3:32 p.m.
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