[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH 3/6] qemu: Record names of domain which uses the shared disk in hash table



On Tue, Feb 19, 2013 at 08:27:42PM +0800, 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);
> +}
> +
> +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 */
> +};

This shouldn't be in the header file, since nothing outside the
.c file should ever touch the hash table contents directly.

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 :|


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]