Martin Sivák has posted comments on this change. Change subject: vdsm: Reboot capability for VM ......................................................................
Patch Set 12: I would prefer that you didn't submit this (7 inline comments) I think it is reasonably good, but I have some reservations about your usage of string manipulations to get the method name. I would like to see a better documentation regarding the shutdown/reboot modes and probably a bit more explicit code with better checking (if reboot: ... elif shutdown: ... else: raise notsupported). I know it will duplicate code a small bit, but it will make it much more readable. .................................................... File lib/vdsm/utils.py Line 927: Line 928: To use this class, first subclass it and override at least methods Line 929: _checkSuccess() and _initCallbacks(). Line 930: """ Line 931: def __init__(self, obj): See my comments about obj below in _initCallbacks. Line 932: self.obj = obj Line 933: self.log = obj.log Line 934: Line 935: def start(self): Line 929: _checkSuccess() and _initCallbacks(). Line 930: """ Line 931: def __init__(self, obj): Line 932: self.obj = obj Line 933: self.log = obj.log Please define (empty) self.callbacks in the constructor Line 934: Line 935: def start(self): Line 936: self.callbacks = self._initCallbacks() Line 937: # start with first callback Line 972: calling another callback in the list. Line 973: If timeout is None, no further callback will be invoked. Line 974: Line 975: :return: list of tuples (method, timeout) Line 976: """ Have you considered using a triple (or more :)? I think passing an object + the callback name is better then restricting the whole callback chain to a single instance. Also I usually prefer when I can reuse the same callback but give it additional/different arguments when registering it. So something like (object, "method_name", additional_kwargs, timeout) or without the string version (because is is actually not necessary here) (bound_method_ref, additional_kwargs, timeout) might be better. Line 977: return [] Line 978: Line 979: def _getCallbackArgs(self): Line 980: """ .................................................... File vdsm/guestIF.py Line 273: self._forward('log-off') Line 274: except: Line 275: self.log.error("desktopLogoff failed", exc_info=True) Line 276: Line 277: def desktopShutdown(self, timeout, msg, reboot): You added force argument to all the others. IS is not needed here or did you just forgot? Line 278: try: Line 279: self.log.debug("desktopShutdown called") Line 280: self._forward('shutdown', {'timeout': timeout, 'message': msg, Line 281: 'reboot': reboot}) .................................................... File vdsm/vm.py Line 1654: Line 1655: def _initCallbacks(self): Line 1656: callbacks = [] Line 1657: Line 1658: def _addCallback(method_prefix, timeout): I would prefer this (or something similar) to be in the CallbackChain's code and not here. Line 1659: method = method_prefix + self.info['action'] Line 1660: callbacks.append((method, timeout)) Line 1661: Line 1662: sys_timeout = config.getint('vars', 'sys_shutdown_timeout') Line 1673: _addCallback('_forced', None) Line 1674: Line 1675: return callbacks Line 1676: Line 1677: def _getCallbackArgs(self): See my comment in utils.py. I think this should be part of the callback registration to make it possible to use callbacks with different arguments. Line 1678: return [self.timeout, self.message] Line 1679: Line 1680: def _checkSuccess(self): Line 1681: return getattr(self.vm, '_checkSuccessful' + self.info['action'])() Line 1677: def _getCallbackArgs(self): Line 1678: return [self.timeout, self.message] Line 1679: Line 1680: def _checkSuccess(self): Line 1681: return getattr(self.vm, '_checkSuccessful' + self.info['action'])() Please document this. Or better move it to VM class and export it using a method somehow. Line 1682: Line 1683: Line 1684: class Vm(object): Line 1685: """ -- To view, visit http://gerrit.ovirt.org/15829 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I12737e363a80679ffb1db55f14eaee158312d7da Gerrit-PatchSet: 12 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Martin Betak <mbe...@redhat.com> Gerrit-Reviewer: Antoni Segura Puimedon <asegu...@redhat.com> Gerrit-Reviewer: Martin Betak <mbe...@redhat.com> Gerrit-Reviewer: Martin Sivák <msi...@redhat.com> Gerrit-Reviewer: Michal Skrivanek <michal.skriva...@redhat.com> Gerrit-Reviewer: Omer Frenkel <ofren...@redhat.com> Gerrit-Reviewer: Peter V. Saveliev <p...@redhat.com> Gerrit-Reviewer: Vinzenz Feenstra <vfeen...@redhat.com> Gerrit-Reviewer: oVirt Jenkins CI Server _______________________________________________ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches