[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH 4/4] qemu: Move shared disk entry adding and unpriv_sgio seting



On 02/08/2013 08:08 AM, Osier Yang wrote:
> The disk def could be free'ed by qemuDomainChangeEjectableMedia
> for cdrom or floppy disk. This moves the adding and setting before
> the attaching takes place. And for cdrom floppy disk, if the
> change is ejecting, removing the existed hash entry for it.
> ---
>  src/qemu/qemu_driver.c  |   23 +++++++++++++----------
>  src/qemu/qemu_hotplug.c |    6 +++++-
>  src/qemu/qemu_hotplug.h |    3 ++-
>  3 files changed, 20 insertions(+), 12 deletions(-)
> 
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index 03fe526..4aad42f 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -5789,6 +5789,7 @@ qemuDomainAttachDeviceDiskLive(virConnectPtr conn,
>  {
>      virDomainDiskDefPtr disk = dev->data.disk;
>      virCgroupPtr cgroup = NULL;
> +    int eject, added;

Initialize eject and added

>      int ret = -1;
>  
>      if (disk->driverName != NULL && !STREQ(disk->driverName, "qemu")) {
> @@ -5798,6 +5799,12 @@ qemuDomainAttachDeviceDiskLive(virConnectPtr conn,
>          goto end;
>      }
>  
> +    if (qemuAddSharedDisk(driver->sharedDisks, disk, &added) < 0)
> +        goto end;
> +
> +    if (qemuSetUnprivSGIO(disk) < 0)
> +        goto end;
> +

hrmph.  makes my comment in 2/4 less important now.  Although it does
make me think that qemuSetUnprivSGIO() could be at the tail end of
qemuAddSharedDisk() since both here and qemuProcessStart() call it.

>      if (qemuDomainDetermineDiskChain(driver, disk, false) < 0)
>          goto end;
>  
> @@ -5814,7 +5821,7 @@ qemuDomainAttachDeviceDiskLive(virConnectPtr conn,
>      switch (disk->device)  {
>      case VIR_DOMAIN_DISK_DEVICE_CDROM:
>      case VIR_DOMAIN_DISK_DEVICE_FLOPPY:
> -        ret = qemuDomainChangeEjectableMedia(driver, vm, disk, false);
> +        ret = qemuDomainChangeEjectableMedia(driver, vm, disk, false, &eject);
>          break;
>      case VIR_DOMAIN_DISK_DEVICE_DISK:
>      case VIR_DOMAIN_DISK_DEVICE_LUN:
> @@ -5843,22 +5850,18 @@ qemuDomainAttachDeviceDiskLive(virConnectPtr conn,
>          break;
>      }
>  
> +    if (ret == 0 && eject)
> +        ignore_value(qemuRemoveSharedDisk(driver->sharedDisks, disk));
> +

'ret' could be 0 here, but eject undefined... Also we're going to add it
just to remove it?


>      if (ret != 0 && cgroup) {
>          if (qemuTeardownDiskCgroup(vm, cgroup, disk) < 0)
>              VIR_WARN("Failed to teardown cgroup for disk path %s",
>                       NULLSTR(disk->src));
>      }
>  
> -    if (ret == 0) {
> -        if (qemuAddSharedDisk(driver->sharedDisks, disk, NULL) < 0)
> -            VIR_WARN("Failed to add disk '%s' to shared disk table",
> -                     disk->src);
> -
> -        if (qemuSetUnprivSGIO(disk) < 0)
> -            VIR_WARN("Failed to set unpriv_sgio of disk '%s'", disk->src);
> -    }
> -

Rather than move this up to the top, why not swap it with the cgroup
teardown and set ret = qemuAddSharedDisk();  Then the 'eject' code is
perhaps unnecessary. It seems it only exists because you added an
ejectable media to the shared device list and now need to remove it.

>  end:
> +    if (ret != 0 && added)

You can get here from "if (disk->driverName != NULL &&
!STREQ(disk->driverName, "qemu"))" without added being set and ret == -1

Of course if you take my suggestion to swap cgroup teardown, then this
becomes moot.

> +        ignore_value(qemuRemoveSharedDisk(driver->sharedDisks, disk))
>      if (cgroup)
>          virCgroupFree(&cgroup);
>      return ret;

There are two callers to qemuDomainChangeEjectableMedia() in
qemu_driver.c - you only changed one (unless I forgot/missed a prior
patch removing it!).


> diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
> index 98912bf..18aca47 100644
> --- a/src/qemu/qemu_hotplug.c
> +++ b/src/qemu/qemu_hotplug.c
> @@ -53,7 +53,8 @@
>  int qemuDomainChangeEjectableMedia(virQEMUDriverPtr driver,
>                                     virDomainObjPtr vm,
>                                     virDomainDiskDefPtr disk,
> -                                   bool force)
> +                                   bool force,
> +                                   int *eject)
>  {
>      virDomainDiskDefPtr origdisk = NULL;
>      int i;
> @@ -93,6 +94,9 @@ int qemuDomainChangeEjectableMedia(virQEMUDriverPtr driver,
>          goto cleanup;
>      }
>  
> +    if (eject && origdisk->src && !disk->src)
> +        *eject = 1;
> +
>      if (virDomainLockDiskAttach(driver->lockManager, cfg->uri,
>                                  vm, disk) < 0)
>          goto cleanup;
> diff --git a/src/qemu/qemu_hotplug.h b/src/qemu/qemu_hotplug.h
> index 8f01d23..fc0532a 100644
> --- a/src/qemu/qemu_hotplug.h
> +++ b/src/qemu/qemu_hotplug.h
> @@ -31,7 +31,8 @@
>  int qemuDomainChangeEjectableMedia(virQEMUDriverPtr driver,
>                                     virDomainObjPtr vm,
>                                     virDomainDiskDefPtr disk,
> -                                   bool force);
> +                                   bool force,
> +                                   int *eject);
>  int qemuDomainCheckEjectableMedia(virQEMUDriverPtr driver,
>                                    virDomainObjPtr vm,
>                                    enum qemuDomainAsyncJob asyncJob);
> 

John


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]