On 02/13/2013 09:57 AM, Osier Yang wrote:

This moves the various checking into the helpers, to avoid the callers missing the checking. --- src/qemu/qemu_conf.c | 20 ++++++++++++++++---- src/qemu/qemu_conf.h | 4 ++-- src/qemu/qemu_driver.c | 14 +++++--------- src/qemu/qemu_process.c | 22 ++++++++++++---------- 4 files changed, 35 insertions(+), 25 deletions(-)

diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c index 2bd1931..d0ee80b 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -826,13 +826,21 @@ qemuGetSharedDiskKey(const char *disk_path) */ int qemuAddSharedDisk(virQEMUDriverPtr driver, - const char *disk_path) + virDomainDiskDefPtr disk) { size_t *ref = NULL; char *key = NULL; int ret = -1;

- if (!(key = qemuGetSharedDiskKey(disk_path))) + /* Currently the only conflicts we have to care about + * for the shared disk is "sgio" setting, which is only + * valid for block disk. + */ + if (disk->type != VIR_DOMAIN_DISK_TYPE_BLOCK || + !disk->shared || !disk->src) + return 0; + + if (!(key = qemuGetSharedDiskKey(disk->src))) goto cleanup;

if ((ref = virHashLookup(driver->sharedDisks, key))) { @@ -854,13 +862,17 @@ cleanup: */ int qemuRemoveSharedDisk(virQEMUDriverPtr driver, - const char *disk_path) + virDomainDiskDefPtr disk) { size_t *ref = NULL; char *key = NULL; int ret = -1;

- if (!(key = qemuGetSharedDiskKey(disk_path))) + if (disk->type != VIR_DOMAIN_DISK_TYPE_BLOCK || + !disk->shared || !disk->src) + return 0; + + if (!(key = qemuGetSharedDiskKey(disk->src))) goto cleanup;

if (!(ref = virHashLookup(driver->sharedDisks, key))) diff --git a/src/qemu/qemu_conf.h b/src/qemu/qemu_conf.h index 09eacce..1685cf6 100644 --- a/src/qemu/qemu_conf.h +++ b/src/qemu/qemu_conf.h @@ -276,11 +276,11 @@ void qemuDriverCloseCallbackRunAll(virQEMUDriverPtr driver, virConnectPtr conn);

int qemuAddSharedDisk(virQEMUDriverPtr driver, - const char *disk_path) + virDomainDiskDefPtr disk) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2);

int qemuRemoveSharedDisk(virQEMUDriverPtr driver, - const char *disk_path) + virDomainDiskDefPtr disk) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); char * qemuGetSharedDiskKey(const char *disk_path) ATTRIBUTE_NONNULL(1); diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index a4fc0c9..f98bd9b 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -5882,11 +5882,9 @@ qemuDomainAttachDeviceDiskLive(virConnectPtr conn, }

if (ret == 0) { - if (disk->type == VIR_DOMAIN_DISK_TYPE_BLOCK && disk->shared) { - if (qemuAddSharedDisk(driver, disk->src) < 0) - VIR_WARN("Failed to add disk '%s' to shared disk table", - disk->src); - } + if (qemuAddSharedDisk(driver, disk) < 0) + VIR_WARN("Failed to add disk '%s' to shared disk table", + disk->src);

Are we assuming disk->src is not NULL here? IOW, do we need a NULLSTR() around this? I cannot remember from prior review, but I do see in the code just above this set of lines a NULLSTR(disk->src).

if (qemuSetUnprivSGIO(disk) < 0) VIR_WARN("Failed to set unpriv_sgio of disk '%s'", disk->src); @@ -6007,10 +6005,8 @@ qemuDomainDetachDeviceDiskLive(virQEMUDriverPtr driver, break; }

- if (ret == 0 && - disk->type == VIR_DOMAIN_DISK_TYPE_BLOCK && - disk->shared) { - if (qemuRemoveSharedDisk(driver, disk->src) < 0) + if (ret == 0) { + if (qemuRemoveSharedDisk(driver, disk) < 0) VIR_WARN("Failed to remove disk '%s' from shared disk table", disk->src);

Same comment

} diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 9759332..8425cbb 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -3526,6 +3526,13 @@ qemuSetUnprivSGIO(virDomainDiskDefPtr disk) { int val = -1;

+ /* "sgio" is only valid for block disk; cdrom + * and floopy disk can have empty source. + */ + if (disk->type != VIR_DOMAIN_DISK_TYPE_BLOCK || + !disk->src) + return 0; + if (disk->sgio) val = (disk->sgio == VIR_DOMAIN_DISK_SGIO_UNFILTERED);

@@ -3952,13 +3959,11 @@ int qemuProcessStart(virConnectPtr conn, _("Raw I/O is not supported on this platform")); #endif

- if (disk->type == VIR_DOMAIN_DISK_TYPE_BLOCK && disk->shared) { - if (qemuAddSharedDisk(driver, disk->src) < 0) - goto cleanup; + if (qemuAddSharedDisk(driver, disk) < 0) + goto cleanup;

- if (qemuCheckSharedDisk(driver, disk) < 0) - goto cleanup; - } + if (qemuCheckSharedDisk(driver, disk) < 0) + goto cleanup;

if (qemuSetUnprivSGIO(disk) < 0) goto cleanup; @@ -4366,10 +4371,7 @@ void qemuProcessStop(virQEMUDriverPtr driver,

for (i = 0; i < vm->def->ndisks; i++) { virDomainDiskDefPtr disk = vm->def->disks[i]; - - if (disk->type == VIR_DOMAIN_DISK_TYPE_BLOCK && disk->shared) { - ignore_value(qemuRemoveSharedDisk(driver, disk->src)); - } + ignore_value(qemuRemoveSharedDisk(driver, disk)); }

/* Clear out dynamically assigned labels */

On 02/13/2013 09:57 AM, Osier Yang wrote:

Based on moving various checking into qemuAddSharedDisk, this avoids the caller using it in wrong ways. --- src/qemu/qemu_conf.c | 66 +++++++++++++++++++++++++++++++++++++++++++++++ src/qemu/qemu_driver.c | 5 --- src/qemu/qemu_process.c | 53 ------------------------------------- src/qemu/qemu_process.h | 3 -- 4 files changed, 66 insertions(+), 61 deletions(-)

diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c index d0ee80b..6e735c6 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -821,6 +821,69 @@ qemuGetSharedDiskKey(const char *disk_path) return key; }

+/* Check if a shared disk's setting conflicts with the conf + * used by other domain(s). Currently only checks the sgio + * setting. Note that this should only be called for disk with + * block source. + * + * Returns 0 if no conflicts, otherwise returns -1. + */ +static int +qemuCheckSharedDisk(virHashTablePtr sharedDisks, + virDomainDiskDefPtr disk) +{ + int val; + size_t *ref = NULL; + char *sysfs_path = NULL; + char *key = NULL; + int ret = 0; + + if (!(sysfs_path = virGetUnprivSGIOSysfsPath(disk->src, NULL))) { + ret = -1; + goto cleanup; + } + + /* It can't be conflict if unpriv_sgio is not supported + * by kernel. + */ + if (!virFileExists(sysfs_path)) + goto cleanup; + + + if (!(key = qemuGetSharedDiskKey(disk->src))) { + ret = -1; + goto cleanup; + } + + /* It can't be conflict if no other domain is + * is sharing it. + */ + if (!(ref = virHashLookup(sharedDisks, key))) + goto cleanup;

The code you deleted below checked if (ref == (void)0x1) - so that's not necessary now? "ref" isn't used as far as I can see (oh and of course the next patch removes that).

+ + if (virGetDeviceUnprivSGIO(disk->src, NULL, &val) < 0) { + ret = -1; + goto cleanup; + } + + if ((val == 0 && + (disk->sgio == VIR_DOMAIN_DISK_SGIO_FILTERED || + disk->sgio == VIR_DOMAIN_DISK_SGIO_DEFAULT)) || + (val == 1 && + disk->sgio == VIR_DOMAIN_DISK_SGIO_UNFILTERED)) + goto cleanup; + + virReportError(VIR_ERR_OPERATION_INVALID, + _("sgio of shared disk '%s' conflicts with other " + "active domains"), disk->src); + ret = -1; + +cleanup: + VIR_FREE(sysfs_path); + VIR_FREE(key); + return ret; +} + /* Increase ref count if the entry already exists, otherwise * add a new entry. */ @@ -840,6 +903,9 @@ qemuAddSharedDisk(virQEMUDriverPtr driver, !disk->shared || !disk->src) return 0;

+ if (qemuCheckSharedDisk(driver->sharedDisks, disk) < 0) + return -1; + if (!(key = qemuGetSharedDiskKey(disk->src))) goto cleanup;

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index f98bd9b..86f9674 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -5825,11 +5825,6 @@ qemuDomainAttachDeviceDiskLive(virConnectPtr conn, goto end; }

- if (disk->type == VIR_DOMAIN_DISK_TYPE_BLOCK && - disk->shared && - (qemuCheckSharedDisk(driver, disk) < 0)) - goto end; -

There's a lot of work going on in this module that all becomes for naught if it's not a shared disk. In fact if we fail to add the disk in this module, I also note that this code path would still call qemuSetUnprivSGIO() (in the live case, if AddShared) - so if it's not a sharable disk, then we're setting something we may expect to set, right?

if (qemuDomainDetermineDiskChain(driver, disk, false) < 0) goto end;

diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 8425cbb..6ffb680 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -3556,56 +3556,6 @@ qemuSetUnprivSGIO(virDomainDiskDefPtr disk) return 0; }

-/* Check if a shared disk's setting conflicts with the conf - * used by other domain(s). Currently only checks the sgio - * setting. Note that this should only be called for disk with - * block source. - * - * Returns 0 if no conflicts, otherwise returns -1. - */ -int -qemuCheckSharedDisk(virQEMUDriverPtr driver, - virDomainDiskDefPtr disk) -{ - int val; - size_t *ref = NULL; - char *key = NULL; - int ret = 0; - - if (!(key = qemuGetSharedDiskKey(disk->src))) - return -1; - - /* It can't be conflict if no other domain is - * is sharing it. - */ - if (!(ref = virHashLookup(driver->sharedDisks, key))) - goto cleanup; - - if (ref == (void *)0x1) - goto cleanup; - - if (virGetDeviceUnprivSGIO(disk->src, NULL, &val) < 0) { - ret = -1; - goto cleanup; - } - - if ((val == 0 && - (disk->sgio == VIR_DOMAIN_DISK_SGIO_FILTERED || - disk->sgio == VIR_DOMAIN_DISK_SGIO_DEFAULT)) || - (val == 1 && - disk->sgio == VIR_DOMAIN_DISK_SGIO_UNFILTERED)) - goto cleanup; - - virReportError(VIR_ERR_OPERATION_INVALID, - _("sgio of shared disk '%s' conflicts with other " - "active domains"), disk->src); - ret = -1; - -cleanup: - VIR_FREE(key); - return ret; -} - int qemuProcessStart(virConnectPtr conn, virQEMUDriverPtr driver, virDomainObjPtr vm, @@ -3962,9 +3912,6 @@ int qemuProcessStart(virConnectPtr conn, if (qemuAddSharedDisk(driver, disk) < 0) goto cleanup;

- if (qemuCheckSharedDisk(driver, disk) < 0) - goto cleanup; - if (qemuSetUnprivSGIO(disk) < 0) goto cleanup; } diff --git a/src/qemu/qemu_process.h b/src/qemu/qemu_process.h index cbdab24..7c620d4 100644 --- a/src/qemu/qemu_process.h +++ b/src/qemu/qemu_process.h @@ -100,7 +100,4 @@ virBitmapPtr qemuPrepareCpumap(virQEMUDriverPtr driver, virBitmapPtr nodemask); int qemuSetUnprivSGIO(virDomainDiskDefPtr disk);

-int qemuCheckSharedDisk(virQEMUDriverPtr driver, - virDomainDiskDefPtr disk); - #endif /* __QEMU_PROCESS_H__ */

On 02/13/2013 09:57 AM, Osier Yang wrote:

The hash entry is changed from "ref" to {ref, @domains}. With this, the caller can simply call qemuRemoveSharedDisk, without afraid of removing s/afraid/fear/ the entry belongs to other domains. qemuProcessStart will obviously s/entry belongs/entry if it also belongs/ benifit from it on error codepath (which calls qemuProcessStop to do

s/benifit/benefit/

the cleanup).

--- src/qemu/qemu_conf.c | 111 +++++++++++++++++++++++++++++++++++++++-------- src/qemu/qemu_conf.h | 22 ++++++++-- src/qemu/qemu_driver.c | 18 ++++++- src/qemu/qemu_process.c | 4 +- 4 files changed, 128 insertions(+), 27 deletions(-)

diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c index 6e735c6..790f5a1 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -832,10 +832,9 @@ static int qemuCheckSharedDisk(virHashTablePtr sharedDisks, virDomainDiskDefPtr disk) { - int val; - size_t *ref = NULL; char *sysfs_path = NULL; char *key = NULL; + int val; int ret = 0;

if (!(sysfs_path = virGetUnprivSGIOSysfsPath(disk->src, NULL))) { @@ -858,7 +857,7 @@ qemuCheckSharedDisk(virHashTablePtr sharedDisks, /* It can't be conflict if no other domain is * is sharing it. */ - if (!(ref = virHashLookup(sharedDisks, key))) + if (!(virHashLookup(sharedDisks, key))) goto cleanup;

So this now says, if the entry doesn't exist at all, then there's no conflict. This is still slightly different from before where it could exist by the refcnt had to be one. Since there's 2 paths to check, does this "rule" prove true for both? With this set of changes it's almost as if you want to determine if it exists *and* it's not entered for "this" domain, right? IOW - is there a chance the disk could be in the shared list because this domain put it there? Seems as though after reading more of this that entry->ref could/should be used.

if (virGetDeviceUnprivSGIO(disk->src, NULL, &val) < 0) { @@ -884,14 +883,39 @@ cleanup: return ret; }

-/* Increase ref count if the entry already exists, otherwise - * add a new entry. +bool +qemuSharedDiskEntryDomainExists(qemuSharedDiskEntryPtr entry, + const char *name, + int *idx) +{ + int i; + + for (i = 0; i < entry->ref; i++) { + if (STREQ(entry->domains[i], name)) { + if (idx) + *idx = i; + return true; + } + } + + return false; +} + +/* qemuAddSharedDisk: + * @driver: Pointer to qemu driver struct + * @disk: The disk def + * @name: The domain name + * + * Increase ref count and add the domain name into the list which + * records all the domains that use the shared disk if the entry + * already exists, otherwise add a new entry. */ int qemuAddSharedDisk(virQEMUDriverPtr driver, - virDomainDiskDefPtr disk) + virDomainDiskDefPtr disk, + const char *name) { - size_t *ref = NULL; + qemuSharedDiskEntry *entry = NULL; char *key = NULL; int ret = -1;

@@ -909,11 +933,34 @@ qemuAddSharedDisk(virQEMUDriverPtr driver, if (!(key = qemuGetSharedDiskKey(disk->src))) goto cleanup;

- if ((ref = virHashLookup(driver->sharedDisks, key))) { - if (virHashUpdateEntry(driver->sharedDisks, key, ++ref) < 0) - goto cleanup; + if ((entry = virHashLookup(driver->sharedDisks, key))) {

This succeeds if the disk->src name is found in sharedDisks list, but fails otherwise, correct?

+ /* Nothing to do if the shared disk is already recorded + * in the table. + */ + if (qemuSharedDiskEntryDomainExists(entry, name, NULL)) { + ret = 0; + goto cleanup; + }

So we get here when we add a domain to the disk - thus the entry->ref is the count of domains that this disk is shared in, right?

+ + if ((VIR_REALLOC_N(entry->domains, entry->ref + 1) < 0) || + !(entry->domains[entry->ref++] = strdup(name))) { + virReportOOMError(); + goto cleanup; + } + + if (virHashUpdateEntry(driver->sharedDisks, key, entry) < 0) + goto cleanup;

If the virHashUpdateEntry() fails, then what happens to the recently expanded 'entry->domains' and strdup()d 'entry->domains[entry->ref+1]'?

} else { - if (virHashAddEntry(driver->sharedDisks, key, (void *)0x1)) + if ((VIR_ALLOC(entry) < 0) || + (VIR_ALLOC_N(entry->domains, 1) < 0) || + !(entry->domains[0] = strdup(name))) { + virReportOOMError(); + goto cleanup; + } + + entry->ref = 1;

Here we didn't find the disk in the sharedDisks and we have to create an entry which consists of two allocations - one for the entry and one for the domain name...

+ + if (virHashAddEntry(driver->sharedDisks, key, entry)) goto cleanup;

If the virHashAddEntry fails, then 'entry', 'entry->domains', and 'entry->domains[0]' are "lost". You'll need some sort of VIR_FREE for them.

}

@@ -923,16 +970,24 @@ cleanup: return ret; }

-/* Decrease the ref count if the entry already exists, otherwise - * remove the entry. +/* qemuRemoveSharedDisk: + * @driver: Pointer to qemu driver struct + * @disk: The disk def + * @name: The domain name + * + * Decrease ref count and remove the domain name from the list which + * records all the domains that use the shared disk if ref is not 1, + * otherwise remove the entry. */ int qemuRemoveSharedDisk(virQEMUDriverPtr driver, - virDomainDiskDefPtr disk) + virDomainDiskDefPtr disk, + const char *name) { - size_t *ref = NULL; + qemuSharedDiskEntryPtr entry = NULL; char *key = NULL; int ret = -1; + int idx;

if (disk->type != VIR_DOMAIN_DISK_TYPE_BLOCK || !disk->shared || !disk->src) @@ -941,11 +996,31 @@ qemuRemoveSharedDisk(virQEMUDriverPtr driver, if (!(key = qemuGetSharedDiskKey(disk->src))) goto cleanup;

- if (!(ref = virHashLookup(driver->sharedDisks, key))) + if (!(entry = virHashLookup(driver->sharedDisks, key))) goto cleanup;

- if (ref != (void *)0x1) { - if (virHashUpdateEntry(driver->sharedDisks, key, --ref) < 0) + /* Nothing to do if the shared disk is not recored in

s/recored/recorded/

+ * the table.

s/table./table for this domain./

+ */ + if (!qemuSharedDiskEntryDomainExists(entry, name, &idx)) { + ret = 0; + goto cleanup; + } + + if (entry->ref != 1) { + if (idx != entry->ref - 1) + memmove(&entry->domains[idx], + &entry->domains[idx + 1], + sizeof(*entry->domains) * (entry->ref - idx - 1));

I think this needs {} around the multiline memmove().

+ + if (VIR_REALLOC_N(entry->domains, entry->ref - 1) < 0) { + virReportOOMError(); + goto cleanup; + } + + entry->ref--;

Hmmm... something's not quite right here. Our now domain name is removed from the list, but what free's the memory it was using? Side note - does there need to be any consideration for locking?

+ + if (virHashUpdateEntry(driver->sharedDisks, key, entry) < 0) goto cleanup; } else { if (virHashRemoveEntry(driver->sharedDisks, key) < 0)

Does this know enough to free entry and entry->domains[0]?

diff --git a/src/qemu/qemu_conf.h b/src/qemu/qemu_conf.h index 1685cf6..6240e2c 100644 --- a/src/qemu/qemu_conf.h +++ b/src/qemu/qemu_conf.h @@ -275,13 +275,27 @@ qemuDriverCloseCallback qemuDriverCloseCallbackGet(virQEMUDriverPtr driver, void qemuDriverCloseCallbackRunAll(virQEMUDriverPtr driver, virConnectPtr conn);

-int qemuAddSharedDisk(virQEMUDriverPtr driver, - virDomainDiskDefPtr disk) +typedef struct _qemuSharedDiskEntry qemuSharedDiskEntry; +typedef qemuSharedDiskEntry *qemuSharedDiskEntryPtr; +struct _qemuSharedDiskEntry { + size_t ref; + char **domains; /* array of domain names */ +}; + +bool qemuSharedDiskEntryDomainExists(qemuSharedDiskEntryPtr entry, + const char *name, + int *index) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2);

+int qemuAddSharedDisk(virQEMUDriverPtr driver, + virDomainDiskDefPtr disk, + const char *name) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3); + int qemuRemoveSharedDisk(virQEMUDriverPtr driver, - virDomainDiskDefPtr disk) - ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); + virDomainDiskDefPtr disk, + const char *name) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3); char * qemuGetSharedDiskKey(const char *disk_path) ATTRIBUTE_NONNULL(1);

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 86f9674..527c671 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -577,6 +577,18 @@ qemuDomainFindMaxID(virDomainObjPtr vm, }

+static void +qemuSharedDisksFree(void *payload, const void *name ATTRIBUTE_UNUSED)

qemuSharedDiskEntryFree () - is perhaps a more relevant name.

+{ + qemuSharedDiskEntryPtr entry = (qemuSharedDiskEntryPtr)payload; + int i; + + for (i = 0; i < entry->ref; i++) { + VIR_FREE(entry->domains[i]); + } + VIR_FREE(entry->domains);

and what free's 'entry'

+} + /** * qemuStartup: * @@ -718,7 +730,7 @@ qemuStartup(bool privileged, if ((qemu_driver->inactivePciHostdevs = virPCIDeviceListNew()) == NULL) goto error;

- if (!(qemu_driver->sharedDisks = virHashCreate(30, NULL))) + if (!(qemu_driver->sharedDisks = virHashCreate(30, qemuSharedDisksFree)))

qemuSharedDisksEntryFree()?

goto error;

if (privileged) { @@ -5877,7 +5889,7 @@ qemuDomainAttachDeviceDiskLive(virConnectPtr conn, }

if (ret == 0) { - if (qemuAddSharedDisk(driver, disk) < 0) + if (qemuAddSharedDisk(driver, disk, vm->def->name) < 0) VIR_WARN("Failed to add disk '%s' to shared disk table", disk->src);

@@ -6001,7 +6013,7 @@ qemuDomainDetachDeviceDiskLive(virQEMUDriverPtr driver, }

if (ret == 0) { - if (qemuRemoveSharedDisk(driver, disk) < 0) + if (qemuRemoveSharedDisk(driver, disk, vm->def->name) < 0) VIR_WARN("Failed to remove disk '%s' from shared disk table", disk->src); } diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 6ffb680..f46bc62 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -3909,7 +3909,7 @@ int qemuProcessStart(virConnectPtr conn, _("Raw I/O is not supported on this platform")); #endif

- if (qemuAddSharedDisk(driver, disk) < 0) + if (qemuAddSharedDisk(driver, disk, vm->def->name) < 0) goto cleanup;

if (qemuSetUnprivSGIO(disk) < 0) @@ -4318,7 +4318,7 @@ void qemuProcessStop(virQEMUDriverPtr driver,

for (i = 0; i < vm->def->ndisks; i++) { virDomainDiskDefPtr disk = vm->def->disks[i]; - ignore_value(qemuRemoveSharedDisk(driver, disk)); + ignore_value(qemuRemoveSharedDisk(driver, disk, vm->def->name)); }

/* Clear out dynamically assigned labels */

On 02/13/2013 09:58 AM, Osier Yang wrote:

The disk def could be free'ed by qemuDomainChangeEjectableMedia, which can thus cause crash if we reference the disk pointer. On the other hand, we have to remove the added shared disk entry from the table on error codepath. --- src/qemu/qemu_driver.c | 25 +++++++++++-------------- 1 files changed, 11 insertions(+), 14 deletions(-)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 527c671..48852ad 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -5837,6 +5837,12 @@ qemuDomainAttachDeviceDiskLive(virConnectPtr conn, goto end; }

+ if (qemuAddSharedDisk(driver, disk, vm->def->name) < 0) + goto end; + + if (qemuSetUnprivSGIO(disk) < 0) + goto end; + if (qemuDomainDetermineDiskChain(driver, disk, false) < 0) goto end;

@@ -5850,6 +5856,7 @@ qemuDomainAttachDeviceDiskLive(virConnectPtr conn, if (qemuSetupDiskCgroup(vm, cgroup, disk) < 0) goto end; } + switch (disk->device) { case VIR_DOMAIN_DISK_DEVICE_CDROM: case VIR_DOMAIN_DISK_DEVICE_FLOPPY: @@ -5888,16 +5895,9 @@ qemuDomainAttachDeviceDiskLive(virConnectPtr conn, NULLSTR(disk->src)); }

- if (ret == 0) { - if (qemuAddSharedDisk(driver, disk, vm->def->name) < 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); - } - end: + if (ret != 0) + ignore_value(qemuRemoveSharedDisk(driver, disk, vm->def->name));

I can understand why not to log here - you didn't add and I think we message those cases, so why say we couldn't remove if we couldn't add.

if (cgroup) virCgroupFree(&cgroup); return ret; @@ -6012,11 +6012,8 @@ qemuDomainDetachDeviceDiskLive(virQEMUDriverPtr driver, break; }

- if (ret == 0) { - if (qemuRemoveSharedDisk(driver, disk, vm->def->name) < 0) - VIR_WARN("Failed to remove disk '%s' from shared disk table", - disk->src); - } + if (ret == 0) + ignore_value(qemuRemoveSharedDisk(driver, disk, vm->def->name));

However, it seems we should log this.

return ret; }

On 02/13/2013 09:58 AM, Osier Yang wrote:

For both qemuDomainAttachDeviceDiskLive and qemuDomainChangeDiskMediaLive.

This is a really sparse definition of what's being changed here. In fact this just seems very different from the remainder of this patch series. I'm unclear why you have to get a copy of the DeviceDef

--- src/qemu/qemu_driver.c | 40 +++++++++++++++++++++++++++++++++++----- src/qemu/qemu_hotplug.c | 10 +++++++++- src/qemu/qemu_hotplug.h | 3 ++- 3 files changed, 46 insertions(+), 7 deletions(-)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 48852ad..f362027 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -5827,7 +5827,10 @@ qemuDomainAttachDeviceDiskLive(virConnectPtr conn, virDomainDeviceDefPtr dev) { virDomainDiskDefPtr disk = dev->data.disk; + virDomainDeviceDefPtr dev_copy = NULL; virCgroupPtr cgroup = NULL; + virCapsPtr caps = NULL; + int eject; int ret = -1;

if (disk->driverName != NULL && !STREQ(disk->driverName, "qemu")) { @@ -5860,7 +5863,13 @@ 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 (!(caps = virQEMUDriverGetCapabilities(driver, false))) + goto end; + + if (!(dev_copy = virDomainDeviceDefCopy(caps, vm->def, dev))) + goto end; + + ret = qemuDomainChangeEjectableMedia(driver, vm, disk, false, &eject); break; case VIR_DOMAIN_DISK_DEVICE_DISK: case VIR_DOMAIN_DISK_DEVICE_LUN: @@ -5889,8 +5898,12 @@ qemuDomainAttachDeviceDiskLive(virConnectPtr conn, break; }

- if (ret != 0 && cgroup) { - if (qemuTeardownDiskCgroup(vm, cgroup, disk) < 0) + if (ret == 0) { + if (eject) + ignore_value(qemuRemoveSharedDisk(driver, dev_copy->data.disk, + vm->def->name)); + } else { + if (cgroup && qemuTeardownDiskCgroup(vm, cgroup, disk) < 0) VIR_WARN("Failed to teardown cgroup for disk path %s", NULLSTR(disk->src)); } @@ -5900,6 +5913,8 @@ end: ignore_value(qemuRemoveSharedDisk(driver, disk, vm->def->name)); if (cgroup) virCgroupFree(&cgroup); + virObjectUnref(caps); + virDomainDeviceDefFree(dev_copy); return ret; }

@@ -6079,6 +6094,9 @@ qemuDomainChangeDiskMediaLive(virDomainObjPtr vm, { virDomainDiskDefPtr disk = dev->data.disk; virCgroupPtr cgroup = NULL; + virDomainDeviceDefPtr dev_copy = NULL; + virCapsPtr caps = NULL; + int eject; int ret = -1;

if (qemuDomainDetermineDiskChain(driver, disk, false) < 0) @@ -6099,9 +6117,19 @@ 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 (!(caps = virQEMUDriverGetCapabilities(driver, false))) + goto end; + + if (!(dev_copy = virDomainDeviceDefCopy(caps, vm->def, dev))) + goto end; + + ret = qemuDomainChangeEjectableMedia(driver, vm, disk, force, &eject); + if (ret == 0) { dev->data.disk = NULL; + if (eject) + ignore_value(qemuRemoveSharedDisk(driver, dev_copy->data.disk, + vm->def->name)); + } break; default: virReportError(VIR_ERR_CONFIG_UNSUPPORTED, @@ -6118,6 +6146,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 0c28a6a..e54cf09 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,13 @@ int qemuDomainChangeEjectableMedia(virQEMUDriverPtr driver, goto cleanup; }

+ if (eject) { + if (origdisk->src && !disk->src) + *eject = 1; + else + *eject = 0; + } + 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);