Francesco Romani has posted comments on this change. Change subject: virt: clientIF: extract vmContainer into a module ......................................................................
Patch Set 6: (7 comments) https://gerrit.ovirt.org/#/c/53101/6/lib/vdsm/virt/vmdict.py File lib/vdsm/virt/vmdict.py: Line 1: # Line 2: # Copyright 2011-2016 Red Hat, Inc. > This is tricky, are we really able to copyright 2011-2015 on a new module? IANAL. But I moved here code which was (c) 2011-2015, so it is new mode, but no new code, hence the range. Line 3: # Line 4: # This program is free software; you can redistribute it and/or modify Line 5: # it under the terms of the GNU General Public License as published by Line 6: # the Free Software Foundation; either version 2 of the License, or Line 29: Line 30: Line 31: _log = logging.getLogger('vdsm') Line 32: _lock = threading.Lock() Line 33: _vms = {} > uppercase? I think not, they are not constants Line 34: Line 35: Line 36: class UnknownVM(Exception): Line 37: pass Line 84: with _lock: Line 85: if vm.id in _vms: Line 86: del _vms[vm.id] Line 87: else: Line 88: raise UnknownVM() > (applies to all of above) this is really just specialized dictionary - is i You have a point here, but we this 'dict' thingy has to be a singleton, and the most pythonic way to do this is to use a module like this. But I'll think about what you suggested, let's see what I can do. Line 89: Line 90: Line 91: @utils.traceback() Line 92: def dispatch_eio_cont(libvirt_vms, sdUUID): Line 103: Line 104: @utils.traceback() Line 105: def dispatch_libvirt_event(conn, dom, *args): Line 106: """ Line 107: conn is unused > why is it there then? because the libvirt API requires it (AFAIK, didn't check too deeply. Will do) Line 108: """ Line 109: vmid = dom.UUIDString() Line 110: with _lock: Line 111: if vmid in _vms: https://gerrit.ovirt.org/#/c/53101/6/vdsm/virt/periodic.py File vdsm/virt/periodic.py: Line 66: scheduler=scheduler) Line 67: _executor.start() Line 68: Line 69: def per_vm_operation(func, period): Line 70: disp = VmDispatcher(vmdict.get_all_items, > (vmdict.py).items seems nicer, let me try Line 71: _executor, func, _timeout_from(period)) Line 72: return Operation(disp, period, scheduler) Line 73: Line 74: _operations = [ Line 93: # thus, does not need dispatching. Line 94: Operation( Line 95: sampling.VMBulkSampler( Line 96: libvirtconnection.get(cif), Line 97: vmdict.get_all_items, > as above ditto Line 98: sampling.stats_cache), Line 99: config.getint('vars', 'vm_sample_interval'), Line 100: scheduler), Line 101: https://gerrit.ovirt.org/#/c/53101/6/vdsm/virt/vm.py File vdsm/virt/vm.py: Line 3902: self.log.exception("Failed to delete VM %s", self.id) Line 3903: else: Line 3904: self._cleanupRecoveryFile() Line 3905: self.log.debug("Total desktops after destroy of %s is %d", Line 3906: self.conf['vmId'], vmdict.num_vms()) > I don't like this (see vmdict.py) - we should be able to use len(${vmdict}) let's see how I can move in this direction Line 3907: Line 3908: def destroy(self): Line 3909: self.log.debug('destroy Called') Line 3910: -- To view, visit https://gerrit.ovirt.org/53101 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Iacd2ae6c5e9ca6a73c0fed978c78c9ebb001c46d Gerrit-PatchSet: 6 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Francesco Romani <from...@redhat.com> Gerrit-Reviewer: Dan Kenigsberg <dan...@redhat.com> Gerrit-Reviewer: Francesco Romani <from...@redhat.com> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Martin Polednik <mpoled...@redhat.com> Gerrit-Reviewer: Milan Zamazal <mzama...@redhat.com> Gerrit-Reviewer: gerrit-hooks <automat...@ovirt.org> Gerrit-HasComments: Yes _______________________________________________ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches