Francesco Romani has posted comments on this change.
Change subject: vdsm: virt: add optional container support
Patch Set 51:
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.
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
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
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.
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
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 https://gerrit.ovirt.org/53820
To unsubscribe, visit https://gerrit.ovirt.org/settings
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>
vdsm-patches mailing list -- email@example.com
To unsubscribe send an email to vdsm-patches-le...@lists.fedorahosted.org