On 02/19/2013 07:27 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 the entry belongs to other domains. qemuProcessStart will obviously benifit from it on error codepath (which calls qemuProcessStop to do the cleanup). --- src/qemu/qemu_conf.c | 163 +++++++++++++++++++++++++++++++++++++++++------ src/qemu/qemu_conf.h | 26 ++++++- src/qemu/qemu_driver.c | 6 +- src/qemu/qemu_process.c | 4 +- 4 files changed, 171 insertions(+), 28 deletions(-)

diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c index e54227f..a0477b3 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -858,10 +858,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;

/* The only conflicts between shared disk we care about now @@ -881,7 +880,6 @@ qemuCheckSharedDisk(virHashTablePtr sharedDisks, if (!virFileExists(sysfs_path)) goto cleanup;

- if (!(key = qemuGetSharedDiskKey(disk->src))) { ret = -1; goto cleanup; @@ -890,7 +888,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;

if (virGetDeviceUnprivSGIO(disk->src, NULL, &val) < 0) { @@ -916,14 +914,83 @@ 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; +} + +void +qemuSharedDiskEntryFree(void *payload, const void *name ATTRIBUTE_UNUSED) +{ + qemuSharedDiskEntryPtr entry = (qemuSharedDiskEntryPtr)payload; + int i; + + for (i = 0; i < entry->ref; i++) { + VIR_FREE(entry->domains[i]); + } + VIR_FREE(entry->domains);

Do we need to VIR_FREE(entry)? There's a VIR_ALLOC(ret) below, followed by VIR_ALLOC_N(ret->domains), followed by strdup(entry->domains[i]);

+} + +static qemuSharedDiskEntryPtr +qemuSharedDiskEntryCopy(const qemuSharedDiskEntryPtr entry) +{ + qemuSharedDiskEntryPtr ret = NULL; + int i; + + if (VIR_ALLOC(ret) < 0) { + virReportOOMError(); + return NULL; + } + + if (VIR_ALLOC_N(ret->domains, entry->ref) < 0) { + virReportOOMError(); + goto cleanup; + } + + for (i = 0; i < entry->ref; i++) { + if (!(ret->domains[i] = strdup(entry->domains[i]))) { + virReportOOMError(); + goto cleanup; + } + ret->ref++; + } + + return ret; + +cleanup: + qemuSharedDiskEntryFree(ret, NULL); + return NULL; +} + +/* 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; + qemuSharedDiskEntry *new_entry = NULL; char *key = NULL; int ret = -1;

@@ -942,11 +1009,40 @@ 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))) { + /* Nothing to do if the shared disk is already recorded + * in the table. + */ + if (qemuSharedDiskEntryDomainExists(entry, name, NULL)) { + ret = 0; + goto cleanup; + } + + if (!(new_entry = qemuSharedDiskEntryCopy(entry))) + goto cleanup; + + if ((VIR_EXPAND_N(new_entry->domains, new_entry->ref, 1) < 0) || + !(new_entry->domains[new_entry->ref - 1] = strdup(name))) { + qemuSharedDiskEntryFree(new_entry, NULL); + virReportOOMError(); + goto cleanup; + } + + if (virHashUpdateEntry(driver->sharedDisks, key, new_entry) < 0) { + qemuSharedDiskEntryFree(new_entry, NULL); + goto cleanup; + }

If this is successful, then shouldn't the former 'entry' now be DiskEntryFree'd since you've successfully replaced the former 'entry' with 'new_entry' in the hash table (eg, payload = new data structure?

} 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; + + if (virHashAddEntry(driver->sharedDisks, key, entry)) goto cleanup; }

@@ -957,16 +1053,25 @@ 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; + qemuSharedDiskEntryPtr new_entry = NULL; char *key = NULL; int ret = -1; + int idx;

if (disk->type != VIR_DOMAIN_DISK_TYPE_BLOCK || !disk->shared || !disk->src) @@ -976,12 +1081,32 @@ qemuRemoveSharedDisk(virQEMUDriverPtr driver, if (!(key = qemuGetSharedDiskKey(disk->src))) goto cleanup;

- if (!(ref = virHashLookup(driver->sharedDisks, key))) + if (!(entry = virHashLookup(driver->sharedDisks, key))) + goto cleanup; + + /* Nothing to do if the shared disk is not recored in + * the table. + */ + if (!qemuSharedDiskEntryDomainExists(entry, name, &idx)) { + ret = 0; goto cleanup; + } + + if (entry->ref != 1) { + if (!(new_entry = qemuSharedDiskEntryCopy(entry))) + goto cleanup; + + if (idx != new_entry->ref - 1) + memmove(&new_entry->domains[idx], + &new_entry->domains[idx + 1], + sizeof(*new_entry->domains) * (new_entry->ref - idx - 1));

- if (ref != (void *)0x1) { - if (virHashUpdateEntry(driver->sharedDisks, key, --ref) < 0) + VIR_SHRINK_N(new_entry->domains, new_entry->ref, 1); + + if (virHashUpdateEntry(driver->sharedDisks, key, new_entry) < 0){ + qemuSharedDiskEntryFree(new_entry, NULL); goto cleanup; + }

Same comment as above. 'new_entry' is now in the hash - the old 'entry' can be DiskEntryFree()'d right?

} else { if (virHashRemoveEntry(driver->sharedDisks, key) < 0) goto cleanup; diff --git a/src/qemu/qemu_conf.h b/src/qemu/qemu_conf.h index 81a001b..d3cd0a1 100644 --- a/src/qemu/qemu_conf.h +++ b/src/qemu/qemu_conf.h @@ -272,16 +272,34 @@ 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);

+void qemuSharedDiskEntryFree(void *payload, const void *name) + ATTRIBUTE_NONNULL(1); + int qemuDriverAllocateID(virQEMUDriverPtr driver);

#endif /* __QEMUD_CONF_H */ diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 5ebe3fa..27d3daf 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -679,7 +679,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, qemuSharedDiskEntryFree))) goto error;

if (privileged) { @@ -5762,7 +5762,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);

@@ -5886,7 +5886,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 e955260..69e4209 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -3831,7 +3831,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) @@ -4237,7 +4237,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 */

The hash entry is changed from "ref" to {ref, @domains}. With this, the caller can simply call qemuRemoveSharedDisk, without afraid of removing the entry belongs to other domains. qemuProcessStart will obviously benifit from it on error codepath (which calls qemuProcessStop to do the cleanup). --- src/qemu/qemu_conf.c | 169 +++++++++++++++++++++++++++++++++++++++++----- src/qemu/qemu_conf.h | 22 +++++- src/qemu/qemu_driver.c | 6 +- src/qemu/qemu_process.c | 4 +- 4 files changed, 173 insertions(+), 28 deletions(-) diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c index e54227f..0bd6be0 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -824,6 +824,11 @@ qemuDriverCloseCallbackRunAll(virQEMUDriverPtr driver, qemuDriverUnlock(driver); } +struct _qemuSharedDiskEntry { + size_t ref; + char **domains; /* array of domain names */ +}; + /* Construct the hash key for sharedDisks as "major:minor" */ char * qemuGetSharedDiskKey(const char *disk_path) @@ -858,10 +863,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; /* The only conflicts between shared disk we care about now @@ -881,7 +885,6 @@ qemuCheckSharedDisk(virHashTablePtr sharedDisks, if (!virFileExists(sysfs_path)) goto cleanup; - if (!(key = qemuGetSharedDiskKey(disk->src))) { ret = -1; goto cleanup; @@ -890,7 +893,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; if (virGetDeviceUnprivSGIO(disk->src, NULL, &val) < 0) { @@ -916,14 +919,84 @@ 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) +{ + size_t i; + + for (i = 0; i < entry->ref; i++) { + if (STREQ(entry->domains[i], name)) { + if (idx) + *idx = i; + return true; + } + } + + return false; +} + +void +qemuSharedDiskEntryFree(void *payload, const void *name ATTRIBUTE_UNUSED) +{ + qemuSharedDiskEntryPtr entry = (qemuSharedDiskEntryPtr)payload; + size_t i; + + for (i = 0; i < entry->ref; i++) { + VIR_FREE(entry->domains[i]); + } + VIR_FREE(entry->domains); + VIR_FREE(entry); +} + +static qemuSharedDiskEntryPtr +qemuSharedDiskEntryCopy(const qemuSharedDiskEntryPtr entry) +{ + qemuSharedDiskEntryPtr ret = NULL; + size_t i; + + if (VIR_ALLOC(ret) < 0) { + virReportOOMError(); + return NULL; + } + + if (VIR_ALLOC_N(ret->domains, entry->ref) < 0) { + virReportOOMError(); + goto cleanup; + } + + for (i = 0; i < entry->ref; i++) { + if (!(ret->domains[i] = strdup(entry->domains[i]))) { + virReportOOMError(); + goto cleanup; + } + ret->ref++; + } + + return ret; + +cleanup: + qemuSharedDiskEntryFree(ret, NULL); + return NULL; +} + +/* 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; + qemuSharedDiskEntry *new_entry = NULL; char *key = NULL; int ret = -1; @@ -942,11 +1015,40 @@ 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))) { + /* Nothing to do if the shared disk is already recorded + * in the table. + */ + if (qemuSharedDiskEntryDomainExists(entry, name, NULL)) { + ret = 0; + goto cleanup; + } + + if (!(new_entry = qemuSharedDiskEntryCopy(entry))) + goto cleanup; + + if ((VIR_EXPAND_N(new_entry->domains, new_entry->ref, 1) < 0) || + !(new_entry->domains[new_entry->ref - 1] = strdup(name))) { + qemuSharedDiskEntryFree(new_entry, NULL); + virReportOOMError(); + goto cleanup; + } + + if (virHashUpdateEntry(driver->sharedDisks, key, new_entry) < 0) { + qemuSharedDiskEntryFree(new_entry, NULL); + goto cleanup; + } } 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; + + if (virHashAddEntry(driver->sharedDisks, key, entry)) goto cleanup; } @@ -957,16 +1059,25 @@ 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; + qemuSharedDiskEntryPtr new_entry = NULL; char *key = NULL; int ret = -1; + int idx; if (disk->type != VIR_DOMAIN_DISK_TYPE_BLOCK || !disk->shared || !disk->src) @@ -976,12 +1087,32 @@ 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 + * the table. + */ + if (!qemuSharedDiskEntryDomainExists(entry, name, &idx)) { + ret = 0; + goto cleanup; + } + + if (entry->ref != 1) { + if (!(new_entry = qemuSharedDiskEntryCopy(entry))) + goto cleanup; + + if (idx != new_entry->ref - 1) + memmove(&new_entry->domains[idx], + &new_entry->domains[idx + 1], + sizeof(*new_entry->domains) * (new_entry->ref - idx - 1)); + + VIR_SHRINK_N(new_entry->domains, new_entry->ref, 1); + + if (virHashUpdateEntry(driver->sharedDisks, key, new_entry) < 0){ + qemuSharedDiskEntryFree(new_entry, NULL); goto cleanup; + } } else { if (virHashRemoveEntry(driver->sharedDisks, key) < 0) goto cleanup; diff --git a/src/qemu/qemu_conf.h b/src/qemu/qemu_conf.h index 6ae33ae..87d8b77 100644 --- a/src/qemu/qemu_conf.h +++ b/src/qemu/qemu_conf.h @@ -272,16 +272,30 @@ qemuDriverCloseCallback qemuDriverCloseCallbackGet(virQEMUDriverPtr driver, void qemuDriverCloseCallbackRunAll(virQEMUDriverPtr driver, virConnectPtr conn); -int qemuAddSharedDisk(virQEMUDriverPtr driver, - virDomainDiskDefPtr disk) +typedef struct _qemuSharedDiskEntry qemuSharedDiskEntry; +typedef qemuSharedDiskEntry *qemuSharedDiskEntryPtr; + +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); +void qemuSharedDiskEntryFree(void *payload, const void *name) + ATTRIBUTE_NONNULL(1); + int qemuDriverAllocateID(virQEMUDriverPtr driver); #endif /* __QEMUD_CONF_H */ diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 813411d..3025d1d 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -681,7 +681,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, qemuSharedDiskEntryFree))) goto error; if (privileged) { @@ -5766,7 +5766,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); @@ -5890,7 +5890,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 a925d61..95ed6c0 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -3830,7 +3830,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) @@ -4235,7 +4235,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 */ -- 1.7.7.6

On 2013年02月19日 21:58, Daniel P. Berrange wrote:

On Tue, Feb 19, 2013 at 08:27:43PM +0800, Osier Yang wrote:

...

--- src/qemu/qemu_process.c | 10 ++++++++++ 1 files changed, 10 insertions(+), 0 deletions(-)

diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 69e4209..6466a79 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -3188,6 +3188,7 @@ qemuProcessReconnect(void *opaque) int reason; virQEMUDriverConfigPtr cfg; virCapsPtr caps = NULL; + int i;

s/int/size_t/

...

memcpy(&oldjob,&data->oldjob, sizeof(oldjob));

@@ -3229,6 +3230,15 @@ qemuProcessReconnect(void *opaque) if (qemuUpdateActiveUsbHostdevs(driver, obj->def)< 0) goto error;

+ /* XXX: Need to change as long as lock is introduced for + * qemu_driver->sharedDisks.

Not sure I understand what this comment is refering to ?

It refers to: /* Immutable pointer. Unsafe APIs. XXX */ virHashTablePtr sharedDisks

...

+ */ + for (i = 0; i< obj->def->ndisks; i++) { + if (qemuAddSharedDisk(driver, obj->def->disks[i], + obj->def->name)< 0) + goto error; + } +

ACK if comment is clarified

Daniel

On Tue, Feb 19, 2013 at 08:27:44PM +0800, 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 27d3daf..5a3550d 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -5710,6 +5710,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;

@@ -5723,6 +5729,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: @@ -5761,16 +5768,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)); if (cgroup) virCgroupFree(&cgroup); return ret; @@ -5885,11 +5885,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))

ACK Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|

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! 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,