Re: [PATCH 6/6] qemu: Remove the shared disk entry if the operation is ejecting or updating

[Osier Yang <jyang@xxxxxxxxxx>]

On 2013年02月20日 09:12, John Ferlan wrote:
> On 02/19/2013 07:27 AM, Osier Yang wrote:
> > For both qemuDomainAttachDeviceDiskLive and qemuDomainChangeDiskMediaLive,
> > remove the shared disk entry of original disk src.
> > ---
> >   src/conf/domain_conf.c   |   20 ++++++++++++
> >   src/conf/domain_conf.h   |    3 ++
> >   src/libvirt_private.syms |    1 +
> >   src/qemu/qemu_driver.c   |   76 ++++++++++++++++++++++++++++++++++++++++++++--
> >   src/qemu/qemu_hotplug.c  |   19 +-----------
> >   src/qemu/qemu_hotplug.h  |    1 +
> >   6 files changed, 99 insertions(+), 21 deletions(-)
>
> Soft ACK - it looks OK, but I'm not too familiar with the code and
> what's being done here.  I do think you need to make a better git
> description at least with respect to what's going on!

Okay. I will update it when pushing, but to let you known what
the patch does clearly before that:

For both AttachDevice and UpdateDevice APIs, if the disk device
is 'cdrom' or 'floppy', the operations could be ejecting, updating,
and inserting. For both ejecting and updating, the shared disk
entry of the original disk src has to be removed, because it's
not useful anymore.

And since the original disk def will be changed, new disk def passed
as argument will be free'ed in qemuDomainChangeEjectableMedia, so
we need to copy the orignal disk def before
qemuDomainChangeEjectableMedia, to use it for qemuRemoveSharedDisk.

> John
> > diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> > index 7a2b012..f2887c6 100644
> > --- a/src/conf/domain_conf.c
> > +++ b/src/conf/domain_conf.c
> > @@ -3359,6 +3359,26 @@ virDomainDiskFindControllerModel(virDomainDefPtr def,
> >       return model;
> >   }
> >
> > +virDomainDiskDefPtr
> > +virDomainDiskFindByBusAndDst(virDomainDefPtr def,
> > +                             int bus,
> > +                             char *dst)
> > +{
> > +    int i;
> > +
> > +    if (!dst)
> > +        return NULL;
> > +
> > +    for (i = 0 ; i<  def->ndisks ; i++) {
> > +        if (def->disks[i]->bus == bus&&
> > +            STREQ(def->disks[i]->dst, dst)) {
> > +            return def->disks[i];
> > +        }
> > +    }
> > +
> > +    return NULL;
> > +}
> > +
> >   int
> >   virDomainDiskDefAssignAddress(virCapsPtr caps, virDomainDiskDefPtr def)
> >   {
> > diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
> > index 9232ff9..4ffa4aa 100644
> > --- a/src/conf/domain_conf.h
> > +++ b/src/conf/domain_conf.h
> > @@ -1936,6 +1936,9 @@ void virDomainDiskHostDefFree(virDomainDiskHostDefPtr def);
> >   int virDomainDiskFindControllerModel(virDomainDefPtr def,
> >                                        virDomainDiskDefPtr disk,
> >                                        int controllerType);
> > +virDomainDiskDefPtr virDomainDiskFindByBusAndDst(virDomainDefPtr def,
> > +                                                 int bus,
> > +                                                 char *dst);
> >   void virDomainControllerDefFree(virDomainControllerDefPtr def);
> >   void virDomainFSDefFree(virDomainFSDefPtr def);
> >   void virDomainActualNetDefFree(virDomainActualNetDefPtr def);
> > diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> > index d8d5877..54ccf95 100644
> > --- a/src/libvirt_private.syms
> > +++ b/src/libvirt_private.syms
> > @@ -360,6 +360,7 @@ virDomainDiskDefGetSecurityLabelDef;
> >   virDomainDiskDeviceTypeToString;
> >   virDomainDiskErrorPolicyTypeFromString;
> >   virDomainDiskErrorPolicyTypeToString;
> > +virDomainDiskFindByBusAndDst;
> >   virDomainDiskFindControllerModel;
> >   virDomainDiskGeometryTransTypeFromString;
> >   virDomainDiskGeometryTransTypeToString;
> > diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> > index 5a3550d..18302e7 100644
> > --- a/src/qemu/qemu_driver.c
> > +++ b/src/qemu/qemu_driver.c
> > @@ -5700,7 +5700,11 @@ qemuDomainAttachDeviceDiskLive(virConnectPtr conn,
> >                                  virDomainDeviceDefPtr dev)
> >   {
> >       virDomainDiskDefPtr disk = dev->data.disk;
> > +    virDomainDiskDefPtr orig_disk = NULL;
> > +    virDomainDeviceDefPtr dev_copy = NULL;
> > +    virDomainDiskDefPtr tmp = NULL;
> >       virCgroupPtr cgroup = NULL;
> > +    virCapsPtr caps = NULL;
> >       int ret = -1;
> >
> >       if (disk->driverName != NULL&&  !STREQ(disk->driverName, "qemu")) {
> > @@ -5733,7 +5737,37 @@ qemuDomainAttachDeviceDiskLive(virConnectPtr conn,
> >       switch (disk->device)  {
> >       case VIR_DOMAIN_DISK_DEVICE_CDROM:
> >       case VIR_DOMAIN_DISK_DEVICE_FLOPPY:
> > -        ret = qemuDomainChangeEjectableMedia(driver, vm, disk, false);
> > +        if (!(orig_disk = virDomainDiskFindByBusAndDst(vm->def,
> > +                                                       disk->bus, disk->dst))) {
> > +            virReportError(VIR_ERR_INTERNAL_ERROR,
> > +                           _("No device with bus '%s' and target '%s'"),
> > +                           virDomainDiskBusTypeToString(disk->bus),
> > +                           disk->dst);
> > +            goto end;
> > +        }
> > +
> > +        if (!(caps = virQEMUDriverGetCapabilities(driver, false)))
> > +            goto end;
> > +
> > +        tmp = dev->data.disk;
> > +        dev->data.disk = orig_disk;
> > +
> > +        if (!(dev_copy = virDomainDeviceDefCopy(caps, vm->def, dev))) {
> > +            dev->data.disk = tmp;
> > +            goto end;
> > +        }
> > +        dev->data.disk = tmp;
> > +
> > +        ret = qemuDomainChangeEjectableMedia(driver, vm, disk, orig_disk, false);
> > +
> > +        /* Need to remove the shared disk entry for the original disk src
> > +         * if the operation is either ejecting or updating.
> > +         */
> > +        if (ret == 0&&
> > +            orig_disk->src&&
> > +            STRNEQ_NULLABLE(orig_disk->src, disk->src))
> > +            ignore_value(qemuRemoveSharedDisk(driver, dev_copy->data.disk,
> > +                                              vm->def->name));
> >           break;
> >       case VIR_DOMAIN_DISK_DEVICE_DISK:
> >       case VIR_DOMAIN_DISK_DEVICE_LUN:
> > @@ -5773,6 +5807,8 @@ end:
> >           ignore_value(qemuRemoveSharedDisk(driver, disk, vm->def->name));
> >       if (cgroup)
> >           virCgroupFree(&cgroup);
> > +    virObjectUnref(caps);
> > +    virDomainDeviceDefFree(dev_copy);
> >       return ret;
> >   }
> >
> > @@ -5951,7 +5987,11 @@ qemuDomainChangeDiskMediaLive(virDomainObjPtr vm,
> >                                 bool force)
> >   {
> >       virDomainDiskDefPtr disk = dev->data.disk;
> > +    virDomainDiskDefPtr orig_disk = NULL;
> > +    virDomainDiskDefPtr tmp = NULL;
> >       virCgroupPtr cgroup = NULL;
> > +    virDomainDeviceDefPtr dev_copy = NULL;
> > +    virCapsPtr caps = NULL;
> >       int ret = -1;
> >
> >       if (qemuDomainDetermineDiskChain(driver, disk, false)<  0)
> > @@ -5972,9 +6012,37 @@ qemuDomainChangeDiskMediaLive(virDomainObjPtr vm,
> >       switch (disk->device) {
> >       case VIR_DOMAIN_DISK_DEVICE_CDROM:
> >       case VIR_DOMAIN_DISK_DEVICE_FLOPPY:
> > -        ret = qemuDomainChangeEjectableMedia(driver, vm, disk, force);
> > -        if (ret == 0)
> > +        if (!(orig_disk = virDomainDiskFindByBusAndDst(vm->def,
> > +                                                       disk->bus, disk->dst))) {
> > +            virReportError(VIR_ERR_INTERNAL_ERROR,
> > +                           _("No device with bus '%s' and target '%s'"),
> > +                           virDomainDiskBusTypeToString(disk->bus),
> > +                           disk->dst);
> > +            goto end;
> > +        }
> > +
> > +        if (!(caps = virQEMUDriverGetCapabilities(driver, false)))
> > +            goto end;
> > +
> > +        tmp = dev->data.disk;
> > +        dev->data.disk = orig_disk;
> > +
> > +        if (!(dev_copy = virDomainDeviceDefCopy(caps, vm->def, dev))) {
> > +            dev->data.disk = tmp;
> > +            goto end;
> > +        }
> > +        dev->data.disk = tmp;
> > +
> > +        ret = qemuDomainChangeEjectableMedia(driver, vm, disk, orig_disk, force);
> > +        if (ret == 0) {
> >               dev->data.disk = NULL;
> > +            /* Need to remove the shared disk entry for the original
> > +             * disk src if the operation is either ejecting or updating.
> > +             */
> > +            if (orig_disk->src&&  STRNEQ_NULLABLE(orig_disk->src, disk->src))
> > +                ignore_value(qemuRemoveSharedDisk(driver, dev_copy->data.disk,
> > +                                                  vm->def->name));
> > +        }
> >           break;
> >       default:
> >           virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> > @@ -5991,6 +6059,8 @@ qemuDomainChangeDiskMediaLive(virDomainObjPtr vm,
> >   end:
> >       if (cgroup)
> >           virCgroupFree(&cgroup);
> > +    virObjectUnref(caps);
> > +    virDomainDeviceDefFree(dev_copy);
> >       return ret;
> >   }
> >
> > diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
> > index 488a440..e631587 100644
> > --- a/src/qemu/qemu_hotplug.c
> > +++ b/src/qemu/qemu_hotplug.c
> > @@ -53,32 +53,15 @@
> >   int qemuDomainChangeEjectableMedia(virQEMUDriverPtr driver,
> >                                      virDomainObjPtr vm,
> >                                      virDomainDiskDefPtr disk,
> > +                                   virDomainDiskDefPtr origdisk,
> >                                      bool force)
> >   {
> > -    virDomainDiskDefPtr origdisk = NULL;
> > -    int i;
> >       int ret = -1;
> >       char *driveAlias = NULL;
> >       qemuDomainObjPrivatePtr priv = vm->privateData;
> >       int retries = CHANGE_MEDIA_RETRIES;
> >       virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
> >
> > -    for (i = 0 ; i<  vm->def->ndisks ; i++) {
> > -        if (vm->def->disks[i]->bus == disk->bus&&
> > -            STREQ(vm->def->disks[i]->dst, disk->dst)) {
> > -            origdisk = vm->def->disks[i];
> > -            break;
> > -        }
> > -    }
> > -
> > -    if (!origdisk) {
> > -        virReportError(VIR_ERR_INTERNAL_ERROR,
> > -                       _("No device with bus '%s' and target '%s'"),
> > -                       virDomainDiskBusTypeToString(disk->bus),
> > -                       disk->dst);
> > -        goto cleanup;
> > -    }
> > -
> >       if (!origdisk->info.alias) {
> >           virReportError(VIR_ERR_INTERNAL_ERROR,
> >                          _("missing disk device alias name for %s"), origdisk->dst);
> > diff --git a/src/qemu/qemu_hotplug.h b/src/qemu/qemu_hotplug.h
> > index 8f01d23..2a146fe 100644
> > --- a/src/qemu/qemu_hotplug.h
> > +++ b/src/qemu/qemu_hotplug.h
> > @@ -31,6 +31,7 @@
> >   int qemuDomainChangeEjectableMedia(virQEMUDriverPtr driver,
> >                                      virDomainObjPtr vm,
> >                                      virDomainDiskDefPtr disk,
> > +                                   virDomainDiskDefPtr orig_disk,
> >                                      bool force);
> >   int qemuDomainCheckEjectableMedia(virQEMUDriverPtr driver,
> >                                     virDomainObjPtr vm,
>
> --
> libvir-list mailing list
> libvir-list@xxxxxxxxxx
> https://www.redhat.com/mailman/listinfo/libvir-list

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list