On Sat, Oct 01, 2016 at 06:43:46PM +0000, Robin H. Johnson wrote: > On Sat, Oct 01, 2016 at 06:30:02PM +0100, Brian Foley wrote: > > On Fri, Sep 30, 2016 at 12:23:17PM -0700, Robin H. Johnson wrote: > > > Implement the QEMU Guest Agent sockets, so that code/scripts on the > > > hypervisors can communicate with guest operating systems easily. > > Awesome. This mostly looks good to me. See comments inline. > FYI pylint output is unusable here. On straight master (no changes) for > me, pylint generates 242 errors, 53 warnings, ~1k Cnnn style errors etc. > Over ~160k of text.
Oh dear, that doesn't sound good. For me, running pylint in a chroot (jessie-amd64-ghc78 using devel/build_chroot on master) gives only one error, and that's related to a psutils API change. What sort of environment are you building in? Would you mind sending me a copy of the pylint output to look at? (privately if you like.) > > > > + # Add guest agent socket > > > + if hvp[constants.HV_USE_GUEST_AGENT]: > > > + qga_addr = utils.GetFreeSlot(pci_reservations, reserve=True) > > pci_reservations isn't defined. Did you mean to derive this from > > hvp[constants.HV_KVM_PCI_RESERVATIONS]? (NB make pylint would have caught > > this.) > I wrote the code on 2.15 where pci_reservations still exists, and > clearly didn't read the rest of the code closely enough when I ported it > to master. I guess I'll write two separate versions now. > > It doesn't need a specific PCI slot, just not to conflict with other > stuff. Thanks. NB this is a patch we wouldn't be able to apply to 2.15 anyway, because it introduces an RPC change, and for stable releases we want to guarantee that running different point releases on different nodes in a cluster won't cause compatibility problems. Otherwise rolling upgrades or downgrades potentially get a lot more difficult. > > Nit: You could use "%#x" % addr instead of "%s" % hex(addr) > > Nit: Also, I think this this would read a little better inlined into the > > device > > string below. > I took existing code style: > http://git.ganeti.org/?p=ganeti.git;a=blob;f=lib/hypervisor/hv_kvm/__init__.py;h=340ddb19001e623523a21b3a82fb0200f9bffcab;hb=003cd9a8bd8d36b24a481a304492ec49efbd5b6d#l1320 ACK, but the qga_info string doesn't make a huge amount of sense on its own. To understand what it means you need to find where it's used, and at that point since it's only used in the one place, you may as well just inline it. > > > + qga_path = self._InstanceQemuGuestAgentMonitor(instance.name) > > > + logging.info("KVM: Guest Agent available at %s", path) > > Shouldn't this be qga_path? (Again, make pylint would have caught this) > Yes, that should have been qga_path. > > > > + # The 'qga0' identified can change, but the > > > 'org.qemu.guest_agent.0' string is > > Typo: s/identified/identifier/ > Thanks. > > > > + # the default expected by the Guest Agent. > > > + kvm_cmd.extend([ > > > + "-chardev", > > > + "socket,path=%s,server,nowait,id=qga0" % qga_path, > > > + ]) > > > + kvm_cmd.extend([ > > > + "-device", > > > + "virtio-serial,id=qga0,%s" % qga_pci_info, > > > + ]) > > > + kvm_cmd.extend([ > > > + "-device", > > > + "virtserialport,chardev=qga0,name=org.qemu.guest_agent.0", > > > + ]) > > Nit: Could you merge these 3 extends into 1 and maybe put the --opt and arg > > on > > the same line? > Existing style did it this way as well, which I preserved. Looking more closely, that's true, but I don't think the existing code would win any beauty prizes. There are a number of things I'd like to clean up in it. For example, why write foo = "%s,arg=%d" % (foo, bar) when you could say foo += ",arg=%d" % bar I think the following expresses the intent nicely, and much more compactly. kvm_cmd.extend([ "-chardev", "socket,path=%s,server,nowait,id=qga0" % qga_path, "-device", "virtio-serial,id=qga0,bus=pci.0,addr=%#x" % qga_addr, "-device", "virtserialport,chardev=qga0,name=org.qemu.guest_agent.0", ]) Cheers, Brian.
