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.
> > + # 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. > > + qga_pci_info = ",bus=pci.0,addr=%s" % hex(qga_addr) > 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 > > + 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. -- Robin Hugh Johnson Gentoo Linux: Dev, Infra Lead, Foundation Trustee & Treasurer E-Mail : [email protected] GnuPG FP : 11ACBA4F 4778E3F6 E4EDF38E B27B944E 34884E85 GnuPG FP : 7D0B3CEB E9B85B1F 825BCECF EE05E6F6 A48F6136
