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 4ff912d..da1d1d4 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -852,14 +852,22 @@ 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; + /* 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; + qemuDriverLock(driver); - if (!(key = qemuGetSharedDiskKey(disk_path))) + if (!(key = qemuGetSharedDiskKey(disk->src))) goto cleanup; if ((ref = virHashLookup(driver->sharedDisks, key))) { @@ -882,14 +890,18 @@ cleanup: */ int qemuRemoveSharedDisk(virQEMUDriverPtr driver, - const char *disk_path) + virDomainDiskDefPtr disk) { size_t *ref = NULL; char *key = NULL; int ret = -1; + if (disk->type != VIR_DOMAIN_DISK_TYPE_BLOCK || + !disk->shared || !disk->src) + return 0; + qemuDriverLock(driver); - if (!(key = qemuGetSharedDiskKey(disk_path))) + 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 14f1427..81a001b 100644 --- a/src/qemu/qemu_conf.h +++ b/src/qemu/qemu_conf.h @@ -273,11 +273,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 23499ef..7524c71 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -5767,11 +5767,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); if (qemuSetUnprivSGIO(disk) < 0) VIR_WARN("Failed to set unpriv_sgio of disk '%s'", disk->src); @@ -5892,10 +5890,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); } diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 4251c34..66a1e32 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -3447,6 +3447,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); @@ -3874,13 +3881,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; @@ -4285,10 +4290,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 */ -- 1.7.7.6

Based on moving various checking into qemuAddSharedDisk, this avoids the caller using it in wrong ways. Also this adds two new checking for qemuCheckSharedDisk (disk device not 'lun' and kernel doesn't support unpriv_sgio simply returns 0). --- src/qemu/qemu_conf.c | 72 +++++++++++++++++++++++++++++++++++++++++++++++ src/qemu/qemu_driver.c | 5 --- src/qemu/qemu_process.c | 53 ---------------------------------- src/qemu/qemu_process.h | 3 -- 4 files changed, 72 insertions(+), 61 deletions(-) diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c index da1d1d4..e54227f 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -847,6 +847,75 @@ 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; + + /* The only conflicts between shared disk we care about now + * is sgio setting, which is only valid for device='lun'. + */ + if (disk->device != VIR_DOMAIN_DISK_DEVICE_LUN) + return 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; + + 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. */ @@ -867,6 +936,9 @@ qemuAddSharedDisk(virQEMUDriverPtr driver, return 0; qemuDriverLock(driver); + if (qemuCheckSharedDisk(driver->sharedDisks, disk) < 0) + goto cleanup; + if (!(key = qemuGetSharedDiskKey(disk->src))) goto cleanup; diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 7524c71..5ebe3fa 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -5710,11 +5710,6 @@ qemuDomainAttachDeviceDiskLive(virConnectPtr conn, goto end; } - if (disk->type == VIR_DOMAIN_DISK_TYPE_BLOCK && - disk->shared && - (qemuCheckSharedDisk(driver, disk) < 0)) - goto end; - if (qemuDomainDetermineDiskChain(driver, disk, false) < 0) goto end; diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 66a1e32..e955260 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -3477,56 +3477,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, @@ -3884,9 +3834,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 ce44fe5..bc4d54d 100644 --- a/src/qemu/qemu_process.h +++ b/src/qemu/qemu_process.h @@ -99,7 +99,4 @@ virBitmapPtr qemuPrepareCpumap(virQEMUDriverPtr driver, virBitmapPtr nodemask); int qemuSetUnprivSGIO(virDomainDiskDefPtr disk); -int qemuCheckSharedDisk(virQEMUDriverPtr driver, - virDomainDiskDefPtr disk); - #endif /* __QEMU_PROCESS_H__ */ -- 1.7.7.6

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); +} + +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; + } } 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; + } } 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 */ -- 1.7.7.6

On 02/19/2013 07:27 AM, Osier Yang wrote: 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]); 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? Same comment as above. 'new_entry' is now in the hash - the old 'entry' can be DiskEntryFree()'d right?

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

--- 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; 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. + */ + for (i = 0; i < obj->def->ndisks; i++) { + if (qemuAddSharedDisk(driver, obj->def->disks[i], + obj->def->name) < 0) + goto error; + } + if (qemuProcessUpdateState(driver, obj) < 0) goto error; -- 1.7.7.6

On 2013年02月19日 21:58, Daniel P. Berrange wrote: It refers to: /* Immutable pointer. Unsafe APIs. XXX */ virHashTablePtr sharedDisks

On Tue, Feb 19, 2013 at 08:27:44PM +0800, Osier Yang wrote: 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: 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