Re: [libvirt] [PATCHv11 2/6] libvirt/qemu - clean up At(De)tachDeviceFlags() for consolidation.

On 04/21/2011 01:23 AM, KAMEZAWA Hiroyuki wrote: > clean up At(De)tachDeviceFlags() for consolidation. > > qemudDomainAttachDeviceFlags()/qemudDomainDetachFlags()/ > qemudDomainUpdateDeviceFlags() has similar logics and copied codes. > > This patch series tries to unify them to use shared code when it can. > At first, clean up At(De)tachDeviceFlags() and devide it into functions. > > By this, this patch pulls out shared components between functions. > Based on patch series by Eric Blake, I added some modification as > switch-case with QEMUD_DEVICE_ATTACH, QEMUD_DEVICE_DETACH, QEMUD_DEVICE_UPDATE I kind of liked my callback function pointers, but your switch statement isn't too many more lines of code and arguably a tiny bit more readable. I want this series in before 0.9.1, so I'm not going to reject the patch just because of a difference in approach :) > > +static int qemudDomainAttachDeviceDiskLive(struct qemud_driver *driver, The prefix qemud is a misnomer (it dates back to when we were running a daemon just for qemu and directly calling into xen; but now that the code has evolved, the daemon is named libvirtd and is independent of qemu, and the qemu code is independent of the daemon and no longer needs the 'd'). New code should use just 'qemu'.

Yes, I know that changing it now means more merge conflicts to resolve later in the series. Oh well. > +static int qemudDomainDetachDeviceDiskLive(struct qemud_driver *driver, > + virDomainObjPtr vm, > + virDomainDeviceDefPtr dev, > + virBitmapPtr qemuCaps) > +{ > + virDomainDiskDefPtr disk = dev->data.disk; > + int ret = -1; > + > + switch (disk->device) { > + case VIR_DOMAIN_DISK_DEVICE_DISK: > + if (disk->bus == VIR_DOMAIN_DISK_BUS_VIRTIO) > + ret = qemuDomainDetachPciDiskDevice(driver, vm, dev, qemuCaps); > + else if (disk->bus == VIR_DOMAIN_DISK_BUS_SCSI) > + ret = qemuDomainDetachDiskDevice(driver, vm, dev, qemuCaps); > + else if (dev->data.disk->bus == VIR_DOMAIN_DISK_BUS_USB) > + ret = qemuDomainDetachDiskDevice(driver, vm, dev, qemuCaps); > + else > + qemuReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", > + _("This type of disk cannot be hot unplugged")); > + break; > + default: > + break; Oops, no error message on that path.

> + > +enum { > + QEMUD_DEVICE_ATTACH, QEMUD_DEVICE_DETACH, QEMUD_DEVICE_UPDATE, I flattened these out to one-per-line as part of renaming to QEMU_*.

> + * update domain status forcibly because the domain status may be changed > * even if we attach the device failed. For example, a new controller may > * be created. > */ > - if (virDomainSaveStatus(driver->caps, driver->stateDir, vm) < 0) > + if (!ret && virDomainSaveStatus(driver->caps, driver->stateDir, vm) < 0) This change violates the comment right before it. I undid it. ACK with those nits fixed, so I pushed a modified version (shoot; I've lost an easy way to have git show my incremental diffs, due to the merge conflict resolution from the renames).