Saggi Mizrahi has posted comments on this change.
Change subject: vdsm API and libvdsm
......................................................................
Patch Set 3: I would prefer that you didn't submit this
(14 inline comments)
Didn't reach the end, too much code and I got other things to do. Will continue
tomorrow.
....................................................
File build-aux/pkg-version
Line 24: else if (NF == 3) print 0, $2, "git" substr($3, 2)
Line 25: else if (NF == 4) print $2, $3, "git" substr($4, 2)
Line 26: }'
Line 27:
Line 28: PKG_VERSION=`cat VERSION 2> /dev/null || git describe --always --match
"v[0-9]*"`
Could be extracted and pushed in
Line 29:
Line 30: if test "x$1" = "x--full"; then
Line 31: echo $PKG_VERSION | tr -d '[:space:]'
Line 32: elif test "x$1" = "x--version"; then
....................................................
File vdsm_api/BindingJsonRpc.py
Line 24: __log__ = None
Line 25: __bridge__ = None
Line 26:
Line 27: class BindingJsonRpc:
Line 28: def __init__(self, bridge, log, ip, port):
Why is the logger given to the class?
Line 29: self.bridge = bridge
Line 30: self.log = log
Line 31: self.serverPort = port
Line 32: self.serverIP = ip
Line 51:
Line 52: def prepareForShutdown(self):
Line 53: self.server.shutdown()
Line 54:
Line 55: class JsonRpcTCPHandler(SocketServer.StreamRequestHandler):
This is far from a compliant JsonRPC handler.
Line 56: """
Line 57: The RequestHandler class for our server.
Line 58:
Line 59: It is instantiated once per connection to the server, and must
Line 70: data = self.request.recv(_Size.size)
Line 71: if len(data) == 0:
Line 72: log.debug("Connection closed")
Line 73: return
Line 74: msgLen = _Size.unpack(data)[0]
Add FIXME: msgLen should have an upper limit
Line 75: msg = json.loads(self.request.recv(msgLen))
Line 76: except:
Line 77: log.error("Unexpected exception", exc_info=True)
Line 78: return
Line 75: msg = json.loads(self.request.recv(msgLen))
Line 76: except:
Line 77: log.error("Unexpected exception", exc_info=True)
Line 78: return
Line 79: log.debug('--> %s', msg)
trace instead of debug mabye?
Line 80:
Line 81: try:
Line 82: ret = bridge._dispatch(msg['methodName'],
msg.get('args', {}))
Line 83: except Exception:
....................................................
File vdsm_api/Bridge.py
Line 68:
Line 69: class DynamicBridge(MethodBridge):
Line 70: def __init__(self, schema):
Line 71: self.parse_schema(schema)
Line 72:
Too much space. pep8 is sad :(
Line 73:
Line 74: def parse_schema(self, schema):
Line 75: self.commands = {}
Line 76: self.classes = {}
Line 113: pass
Line 114: return getattr(API, cls)
Line 115:
Line 116: def type_fixup(self, sym_name, sym_type, obj):
Line 117: logging.info("type_fixup: '%s': '%s'" % (sym_name, sym_type))
warning?
Line 118: isList = False
Line 119: if type(sym_type) is list:
Line 120: sym_type = sym_type[0]
Line 121: isList = True
....................................................
File vdsm_api/generate.py
Line 20: sys.path.append("../vdsm_api")
Line 21: import vdsmapi
Line 22:
Line 23:
Line 24: def genindent(count):
It's python :)
return " " * count
Though you might want to keep it in multiplies of 4
return " " * (count * 4)
Line 25: ret = ""
Line 26: for i in range(count):
Line 27: ret += " "
Line 28: return ret
Line 28: return ret
Line 29:
Line 30:
Line 31: def push_indent(indent_amount=4):
Line 32: global indent_level
Objects are free, no need for global variables
Line 33: indent_level += indent_amount
Line 34:
Line 35:
Line 36: def pop_indent(indent_amount=4):
Line 33: indent_level += indent_amount
Line 34:
Line 35:
Line 36: def pop_indent(indent_amount=4):
Line 37: global indent_level
and so on...
Line 38: indent_level -= indent_amount
Line 39:
Line 40:
Line 41: def cgen(code, **kwds):
Line 40:
Line 41: def cgen(code, **kwds):
Line 42: indent = genindent(indent_level)
Line 43: lines = code.split('\n')
Line 44: lines = map(lambda x: indent + x, lines)
Go green. Create and iterator and save memory and CPU.
lines = (indent + line for line in lines)
cgen?
Line 45: return '\n'.join(lines) % kwds + '\n'
Line 46:
Line 47:
Line 48: def mcgen(code, **kwds):
Line 44: lines = map(lambda x: indent + x, lines)
Line 45: return '\n'.join(lines) % kwds + '\n'
Line 46:
Line 47:
Line 48: def mcgen(code, **kwds):
mcgen?
Line 49: return cgen('\n'.join(code.split('\n')[1:-1]), **kwds)
Line 50:
Line 51:
Line 52: def parse_args(typeinfo):
Line 48: def mcgen(code, **kwds):
Line 49: return cgen('\n'.join(code.split('\n')[1:-1]), **kwds)
Line 50:
Line 51:
Line 52: def parse_args(typeinfo):
iterTypeInfoMembers()
Line 53: for member in typeinfo:
Line 54: argname = member
Line 55: argentry = typeinfo[member]
Line 56: optional = False
Line 184: def generate_errors(f, errors):
Line 185: ret = cgen('''
Line 186: public errordomain VdsmError {
Line 187: ''')
Line 188: push_indent()
indent should be a context
Line 189: for error in errors:
Line 190: ret += mcgen('''
Line 191: %(error_name)s = %(error_code)i,
Line 192: ''', error_name=error['name'], error_code=error['code'])
--
To view, visit http://gerrit.ovirt.org/7516
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: If6bd34700b86aa84c7e289f02c0e9f2ac6fcba63
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke <[email protected]>
Gerrit-Reviewer: Adam Litke <[email protected]>
Gerrit-Reviewer: Dan Kenigsberg <[email protected]>
Gerrit-Reviewer: Royce Lv <[email protected]>
Gerrit-Reviewer: Saggi Mizrahi <[email protected]>
Gerrit-Reviewer: oVirt Jenkins CI Server
_______________________________________________
vdsm-patches mailing list
[email protected]
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
