Martin Polednik has posted comments on this change.

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

Patch Set 33:

File lib/vdsm/virt/containers/

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

PS33, Line 45:     def __repr__(self):
             :         return "%s: %s" % (
             :   ,
             :             '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 (considering s/_name/name) is already property, this doesn't add any 

PS33, Line 95: # TODO document the purpose
Yes please. This smells like a redundancy to incl. the 
Path class. Most of the module does.
File lib/vdsm/virt/containers/

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.
File lib/vdsm/virt/containers/

PS33, Line 28: body

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, 
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:
As said elsewhere, this is test implementation leak. Shouldn't be mixed w/ 
runtime code.

PS33, Line 98: global root

To view, visit
To unsubscribe, visit

Gerrit-MessageType: comment
Gerrit-Change-Id: I0fb768ea97dd719cde9bd5e57e1b7cabe4b0f0ae
Gerrit-PatchSet: 33
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Francesco Romani <>
Gerrit-Reviewer: Francesco Romani <>
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Polednik <>
Gerrit-Reviewer: gerrit-hooks <>
Gerrit-HasComments: Yes
vdsm-patches mailing list --
To unsubscribe send an email to

Reply via email to