[libvirt] [PATCH 01/12] qemu: Add a hash table for the shared disks

[Daniel P. Berrange]

On Tue, Dec 11, 2012 at 09:37:18PM +0800, Osier Yang wrote:
> This introduces a hash table for qemu driver, to store the shared
> disk's info as (@disk_path, {@ref_count, @orig_cdbfilter}). @ref_count
> is the number of domains which shares the disk. @orig_cdbfilter is
> the original cdbfilter setting of the shared disk, it will be used
> to restore the the disk's cdbfilter setting to original value by
> later patches.
> 
> * src/qemu/qemu_conf.h: (Add member 'sharedDisks' of type
>                          virHashTablePtr; New struct qemuSharedDiskEntry;
>                          Declare helpers qemuAddSharedDisk,
>                          qemuRemoveSharedDisk)
> * src/qemu/qemu_conf.c (Implement the two helpers)
> * src/qemu/qemu_process.c (Update 'sharedDisks' when domain
>                            starting and shutdown)
> * src/qemu/qemu_driver.c (Update 'sharedDisks' when attaching
>                           or detaching disk).
> 
> 0 is passed for orig_cdbfilter temporarily, later patches will update
> it.
> ---
>  src/qemu/qemu_conf.c    |   48 +++++++++++++++++++++++++++++++++++++++++++++++
>  src/qemu/qemu_conf.h    |   18 +++++++++++++++++
>  src/qemu/qemu_driver.c  |   17 ++++++++++++++++
>  src/qemu/qemu_process.c |   17 +++++++++++++++-
>  4 files changed, 99 insertions(+), 1 deletions(-)
> 
> diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c
> index 8d380a1..2b21186 100644
> --- a/src/qemu/qemu_conf.c
> +++ b/src/qemu/qemu_conf.c
> @@ -556,3 +556,51 @@ qemuDriverCloseCallbackRunAll(virQEMUDriverPtr driver,
>  
>      virHashForEach(driver->closeCallbacks, qemuDriverCloseCallbackRun, &data);
>  }
> +
> +/* Increase ref count if the entry already exists, otherwise
> + * add a new entry.
> + */
> +int
> +qemuAddSharedDisk(virHashTablePtr sharedDisks,
> +                  const char *disk_path,
> +                  int orig_cdbfilter)
> +{
> +    qemuSharedDiskEntryPtr entry = NULL;
> +
> +    if ((entry = virHashLookup(sharedDisks, disk_path))) {
> +        entry->ref++;
> +    } else {
> +        if (VIR_ALLOC(entry) < 0)
> +            return -1;
> +
> +        entry->ref = 1;
> +        entry->orig_cdbfilter = orig_cdbfilter;
> +
> +        if (virHashAddEntry(sharedDisks, disk_path, entry))
> +            return -1;

Leaking 'entry' in failure path.

> @@ -6035,6 +6039,12 @@ qemuDomainAttachDeviceDiskLive(virConnectPtr conn,
>              VIR_WARN("Failed to teardown cgroup for disk path %s",
>                       NULLSTR(disk->src));
>      }
> +
> +    if (ret == 0 && disk->shared) {
> +        if (qemuAddSharedDisk(driver->sharedDisks, disk->src, 0) < 0)
> +            VIR_WARN("Failed to add disk '%s' to shared disk table",
> +                     disk->src);
> +    }

You are not allowed to reference 'disk->src' unless you've validate
disk->type is FILE or BLOCK.

We only need to track this for TYPE_BLOCK in any case


>  end:
>      if (cgroup)
>          virCgroupFree(&cgroup);
> @@ -6149,6 +6159,13 @@ qemuDomainDetachDeviceDiskLive(virQEMUDriverPtr driver,
>                         virDomainDiskDeviceTypeToString(disk->type));
>          break;
>      }
> +
> +    if (ret == 0 && disk->shared) {
> +        if (qemuRemoveSharedDisk(driver->sharedDisks, disk->src) < 0)
> +             VIR_WARN("Failed to remove disk '%s' from shared disk table",
> +                      disk->src);
> +    }

Same crash problem here


> +
>      return ret;
>  }
>  
> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
> index ab04599..89152b8 100644
> --- a/src/qemu/qemu_process.c
> +++ b/src/qemu/qemu_process.c
> @@ -3706,8 +3706,15 @@ int qemuProcessStart(virConnectPtr conn,
>  
>      /* in case a certain disk is desirous of CAP_SYS_RAWIO, add this */
>      for (i = 0; i < vm->def->ndisks; i++) {
> -        if (vm->def->disks[i]->rawio == 1)
> +        virDomainDiskDefPtr disk = vm->def->disks[i];
> +
> +        if (disk->rawio == 1)
>              virCommandAllowCap(cmd, CAP_SYS_RAWIO);
> +
> +        if (disk->shared) {
> +            if (qemuAddSharedDisk(driver->sharedDisks, disk->src, 0) < 0)
> +                goto cleanup;
> +        }


And here

>      }
>  
>      virCommandSetPreExecHook(cmd, qemuProcessHook, &hookData);
> @@ -4104,6 +4111,14 @@ void qemuProcessStop(virQEMUDriverPtr driver,
>                                            flags & VIR_QEMU_PROCESS_STOP_MIGRATED);
>      virSecurityManagerReleaseLabel(driver->securityManager, vm->def);
>  
> +    for (i = 0; i < vm->def->ndisks; i++) {
> +        virDomainDiskDefPtr disk = vm->def->disks[i];
> +
> +        if (disk->shared) {
> +            ignore_value(qemuRemoveSharedDisk(driver->sharedDisks, disk->src));
> +        }
> +    }

And here

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