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
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list