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