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

[KAMEZAWA Hiroyuki]

On Thu, 21 Apr 2011 14:27:23 -0600
Eric Blake <eblake at redhat.com> 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