Martin Polednik has posted comments on this change.

Change subject: vdsm: virt: add optional container support

Patch Set 51: Code-Review-1

File lib/vdsm/

PS51, Line 33: _runtimes = frozenset()
I have inner hate for how this is further "calculated" (also considering the 
followup patch) - do we *have to* use the frozenset? I can't see the benefit of 
overwriting frozenset 3 times over |='ing it.
File lib/vdsm/

PS51, Line 99: def _is_cmd(argv, reqv):
Why this isn't attribute of SuperVdsmCommand class? It's slight implementation 
leak imho.

PS51, Line 113: def _call(ret):
Same as above, I believe it should be __call__ on top of that (internally 
calling self._execute).

PS51, Line 153:  
container connection

PS51, Line 161: ev

PS51, Line 177: For clearing connections during the tests.
Only? Runtime code shouldn't be polluted by test helpers.
File vdsm/virt/

PS51, Line 167: all_vms
s/vms/domains ?

PS51, Line 177: def _all_vms_from_runtime(cif):
s/vms/domains ?

Also, what does runtime mean in this context? We're not in any runtime in 
recovery flow afaik.
File vdsm/virt/

PS51, Line 1657: if is_kvm(self.conf):
My vote for split or helper function remains.

To view, visit
To unsubscribe, visit

Gerrit-MessageType: comment
Gerrit-Change-Id: Id236a30a5c875994c037b8d00c7463bceaab143f
Gerrit-PatchSet: 51
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