On Thu, 21 Apr 2011 14:27:23 -0600
Eric Blake <eblake@xxxxxxxxxx> wrote:
> 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'.
>
I see.
> 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.
>
Ah, yes.
> > +
> > +enum {
> > + QEMUD_DEVICE_ATTACH, QEMUD_DEVICE_DETACH, QEMUD_DEVICE_UPDATE,
>
> I flattened these out to one-per-line as part of renaming to QEMU_*.
>
Sure.
> > + * 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).
Thank you.
-kame
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list