[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 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 */
> 


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