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 <[email protected]>
Gerrit-Reviewer: Antoni Segura Puimedon <[email protected]>
Gerrit-Reviewer: Martin Betak <[email protected]>
Gerrit-Reviewer: Martin Sivák <[email protected]>
Gerrit-Reviewer: Michal Skrivanek <[email protected]>
Gerrit-Reviewer: Omer Frenkel <[email protected]>
Gerrit-Reviewer: Peter V. Saveliev <[email protected]>
Gerrit-Reviewer: Vinzenz Feenstra <[email protected]>
Gerrit-Reviewer: oVirt Jenkins CI Server
_______________________________________________
vdsm-patches mailing list
[email protected]
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
