Francesco Romani has posted comments on this change. Change subject: vdsm: virt: add optional container support ......................................................................
Patch Set 51: (9 comments) https://gerrit.ovirt.org/#/c/53820/51/lib/vdsm/containersconnection.py File lib/vdsm/containersconnection.py: 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. https://gerrit.ovirt.org/#/c/53820/51/lib/vdsm/containerslib.py File lib/vdsm/containerslib.py: 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 above) PS51, Line 153: > container connection Done PS51, Line 161: ev > s/ev/event Done 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. https://gerrit.ovirt.org/#/c/53820/51/vdsm/virt/recovery.py File vdsm/virt/recovery.py: 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 https://gerrit.ovirt.org/#/c/53820/51/vdsm/virt/vm.py File vdsm/virt/vm.py: 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-MessageType: comment Gerrit-Change-Id: Id236a30a5c875994c037b8d00c7463bceaab143f Gerrit-PatchSet: 51 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
