Martin Polednik has posted comments on this change.

Change subject: core: containers: add the container support module
......................................................................


Patch Set 33:

(11 comments)

https://gerrit.ovirt.org/#/c/59824/33/lib/vdsm/virt/containers/command.py
File lib/vdsm/virt/containers/command.py:

PS33, Line 2: 2008-
That doesn't seem right to me.


PS33, Line 45:     def __repr__(self):
             :         return "%s: %s" % (
             :             self.name,
             :             'not found in %r' % self._paths
             :             if self._cmd is None else self._cmd,
             :         )
Format is much nicer.


PS33, Line 52:     @property
             :     def name(self):
             :         return self._name
cls.name (considering s/_name/name) is already property, this doesn't add any 
value.


PS33, Line 95: # TODO document the purpose
Yes please. This smells like a redundancy to 
https://gerrit.ovirt.org/#/c/56491/31/vdsm/supervdsm_api/systemd.py incl. the 
Path class. Most of the module does.


https://gerrit.ovirt.org/#/c/59824/33/lib/vdsm/virt/containers/doms.py
File lib/vdsm/virt/containers/doms.py:

PS33, Line 45: return list(_doms.values())  # py3 compat
Love this compared to six.iteritems.


PS33, Line 63: # use only for testing
             : def clear():
             :     with _lock:
             :         _doms.clear()
Same as every other test leak. Please move the functionality to tests.


https://gerrit.ovirt.org/#/c/59824/33/lib/vdsm/virt/containers/events.py
File lib/vdsm/virt/containers/events.py:

PS33, Line 28: body
func?


PS33, Line 31: def _null_cb(*args, **kwargs):
             :     pass
             : 
             : 
             : _NULL = Callback(None, None, _null_cb, tuple())
I'd move this to get_callbacks unless it has any reason to exist elsewhere.


PS33, Line 80: self._log.warning('[%s] unhandled event %r', self._name, 
event_id)
I find this confusing. I'd expect "unhandled event" to be logged when the event 
is triggered, not when looking up it's callbacks.


PS33, Line 88:     # for testing purposes
             :     def clear(self):
             :         with self._lock:
             :             self.events.clear()
As said elsewhere, this is test implementation leak. Shouldn't be mixed w/ 
runtime code.


PS33, Line 98: global root
??


-- 
To view, visit https://gerrit.ovirt.org/59824
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I0fb768ea97dd719cde9bd5e57e1b7cabe4b0f0ae
Gerrit-PatchSet: 33
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Francesco Romani <from...@redhat.com>
Gerrit-Reviewer: Francesco Romani <from...@redhat.com>
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Polednik <mpoled...@redhat.com>
Gerrit-Reviewer: gerrit-hooks <automat...@ovirt.org>
Gerrit-HasComments: Yes
_______________________________________________
vdsm-patches mailing list -- vdsm-patches@lists.fedorahosted.org
To unsubscribe send an email to vdsm-patches-le...@lists.fedorahosted.org

Reply via email to