Francesco Romani has posted comments on this change.

Change subject: vdsm: virt: add optional container support

Patch Set 51:

File lib/vdsm/

PS51, Line 33: _runtimes = frozenset()
> I have inner hate for how this is further "calculated" (also considering th
no, we don't really need a frozenset, we can probably kill it.
File lib/vdsm/

PS51, Line 99: def _is_cmd(argv, reqv):
> Why this isn't attribute of SuperVdsmCommand class? It's slight implementat
true, but OTOH this code doesn't use the SuperVdsmCommand state at all, so 
could also be a free function

PS51, Line 113: def _call(ret):
> Same as above, I believe it should be __call__ on top of that (internally c
SubProcCommand.__call__ already does that!

Again, this is a free function because it doesn't use any of the 
SuperVdsmCommand state.
Anyway it's open for discussion, I'm not entirely happy with both designs. If 
you think it's clearer as method, I'll make this a method (same for _is_cmd 

PS51, Line 153:  
> container connection

PS51, Line 161: ev
> s/ev/event

PS51, Line 177: For clearing connections during the tests.
> Only? Runtime code shouldn't be polluted by test helpers.
And for consistency with libvirtconnection. But we don't really care here, so 
let's kill this.
File vdsm/virt/

PS51, Line 167: all_vms
> s/vms/domains ?
yes, a bit better. Changed.

PS51, Line 177: def _all_vms_from_runtime(cif):
> s/vms/domains ?
_all_running_domains(cif) is probably clearer and a better name
File vdsm/virt/

PS51, Line 1657: if is_kvm(self.conf):
> My vote for split or helper function remains.
Let's retry with a different partiotioning.

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