Change in vdsm[master]: [WIP] Implement python bindings

smizrahi at redhat.com smizrahi at redhat.com
Wed Feb 6 19:55:29 UTC 2013


Saggi Mizrahi has posted comments on this change.

Change subject: [WIP] Implement python bindings
......................................................................


Patch Set 2: I would prefer that you didn't submit this

(9 inline comments)

Some minor comments.
The big problems are:
move everything to $GITROOT/vdsm_api/python/vdsm_client
Explain why you even generate code. Everything can be done dynamically at runtime if you already distribute the schema.

....................................................
File vdsm_api/vdsmapi-schema.json
Line 2341: # @deviceType:  The device type (always @balloon)
Line 2342: #
Line 2343: # @device:      The the type of balloon device
Line 2344: #
Line 2345: # @address:     #optional Device hardware address
unrelated?
Line 2346: #
Line 2347: # @alias:       Alias used to identify this device in commands
Line 2348: #
Line 2349: # @specParams:  #optional Additional device parameters


....................................................
File vdsm_api/vdsm_client/generate.py
Line 119: def generate_file_top(f):
Line 120:     f.write(pygen('''##
Line 121: # AUTOMATICALLY GENERATED, DO NOT MODIFY
Line 122: ##
Line 123: # Copyright (C) 2013 Adam Litke, IBM Corporation
I the generated code really is a copyright of IBM?
IANAL but I wonder who is the copyright holder if the code was generated by a machine,
Line 124: #
Line 125: # This program is free software; you can redistribute it and/or modify
Line 126: # it under the terms of the GNU General Public License as published by
Line 127: # the Free Software Foundation; either version 2 of the License, or


Line 138: #
Line 139: # Refer to the README and COPYING files for full details of the license
Line 140: ##
Line 141: 
Line 142: from modelBase import encode, VdsmRequest, CommandNamespace, BaseType
I'd separate the LICENCE generator from the imports one.
It's quite confusing when to see that you actually do anything useful with this method at a glance. At least it was hard for me :)
Line 143: '''))
Line 144: 
Line 145: 
Line 146: def generate_file_bottom(f):


Line 158: 
Line 159:     for val in enum['data']:
Line 160:         name = val.replace('-', '_').replace(' ', '_').upper()
Line 161:         ret += mpygen('''
Line 162:     %(name)s = '%(value)s'
We assume enums are only strings?
Line 163: ''', name=name, value=val.lower())
Line 164:     f.write(ret)
Line 165: 
Line 166: 


Line 350:     if name in aliased_types:
Line 351:         name = aliased_types[name]
Line 352:     return name
Line 353: 
Line 354: 
if __name__ == "__main__":

Some of our code might want to reuse these methods in the future without causing horrible side effects
Line 355: ###############################################################################
Line 356: indent_level = 0
Line 357: namespaces = {}
Line 358: enum_types = []


Line 361: aliased_types = {}
Line 362: union_members = {}
Line 363: 
Line 364: 
Line 365: errors = [
These already use the proper jsonrpc return codes in my stack.
Line 366:     {'code': 1, 'name': 'PROTOCOL_ERROR', 'message': ''},
Line 367:     {'code': 2, 'name': 'CONNECTION_ERROR', 'message': ''},
Line 368:     {'code': 3, 'name': 'OPERATION_CANCELLED', 'message': ''},
Line 369:     {'code': 4, 'name': 'OPERATION_NOT_SUPPORTED', 'message': ''},


....................................................
File vdsm_api/vdsm_client/modelBase.py
Line 72: class VdsmConnection(object):
Line 73:     def __init__(self, uri):
Line 74:         addr, port = uri.split(':')
Line 75: 
Line 76:         reactor = AsyncoreReactor()
The whole point was to reuse reactor for multiple clients.
But for stage 1 this is fine.
Line 77:         t = threading.Thread(target=reactor.process_requests)
Line 78:         t.setDaemon(True)
Line 79:         t.start()
Line 80:         client = ClientWrapper(reactor.createClient((addr, int(port))))


Line 107:         self.params = params
Line 108: 
Line 109:         # TODO: I think the id should be passed when building the call, not
Line 110:         # when building the request.  In this way, sync calls can completely
Line 111:         # ignore the id and async/batch calls can pass in an array of ids
The call can contain multiple requests.
A call with null ID is a notification which is perfectly valid.
Especially if you use the AMQP reactor and can get a request settlement from the transport so you might use notification a bit more often to save on bandwidth.
Line 112:         self.id = _getReqId()
Line 113: 
Line 114:         self._jsonRequest = JsonRpcRequest(self.method, self.params, self.id)
Line 115: 


....................................................
File vdsm.spec.in
Line 207: %package client-python
Line 208: Summary:        VDSM client library for python
Line 209: BuildArch:      noarch
Line 210: Requires: %{name}-yajsonrpc = %{version}-%{release}
Line 211: Requires: %{name}-jsonrpc = %{version}-%{release}
Why does the client require the Bindings?
If you need anything from that package it has to be split as it will pull in the entire daemon
Line 212: Requires: %{name}-python = %{version}-%{release}
Line 213: 
Line 214: %description client-python
Line 215: A module for accessing the vdsm API from python programs


--
To view, visit http://gerrit.ovirt.org/11755
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I3f22966348c4c459710d79e1d97f98e2280d7022
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke <agl at us.ibm.com>
Gerrit-Reviewer: Adam Litke <agl at us.ibm.com>
Gerrit-Reviewer: Saggi Mizrahi <smizrahi at redhat.com>
Gerrit-Reviewer: oVirt Jenkins CI Server


More information about the vdsm-patches mailing list