[libvirt] [PATCH 2/8 v8] qemu: Add a hash table for the shared disks

[Eric Blake]

On 12/14/2012 08:42 AM, Osier Yang wrote:
> This introduces a hash table for qemu driver, to store the shared
> disk's info as (@disk_path, @ref_count). @ref_count is the number

A bit out of date - the key is now major:minor, not path.

> of domains which shares the disk.
> 
> Since we only care about the disk's cdbfilter (see later patch)
> setting for shared disk currently, and cdbfilter is only valid for
> block disk, this patch only manages (add/remove hash entry) the
> shared disk for block disk.
> 
> * src/qemu/qemu_conf.h: (Add member 'sharedDisks' of type
>                          virHashTablePtr; Declare helpers
>                          qemuGetSharedDiskKey, qemuAddSharedDisk
>                          and qemuRemoveSharedDisk)
> * src/qemu/qemu_conf.c (Implement the 3 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).
> ---
>  src/qemu/qemu_conf.c    |   86 +++++++++++++++++++++++++++++++++++++++++++++++
>  src/qemu/qemu_conf.h    |   12 ++++++
>  src/qemu/qemu_driver.c  |   22 ++++++++++++
>  src/qemu/qemu_process.c |   15 ++++++++
>  4 files changed, 135 insertions(+), 0 deletions(-)
> 
> diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c
> index a1b1d04..e25376d 100644
> --- a/src/qemu/qemu_conf.c
> +++ b/src/qemu/qemu_conf.c
> @@ -553,3 +553,89 @@ qemuDriverCloseCallbackRunAll(virQEMUDriverPtr driver,
>  
>      virHashForEach(driver->closeCallbacks, qemuDriverCloseCallbackRun, &data);
>  }
> +
> +/* Construct the hash key for sharedDisks as "major:minor" */
> +char *
> +qemuGetSharedDiskKey(const char *disk_path)
> +{
> +    int major, minor;
> +    char *key = NULL;
> +    int rc;
> +
> +    if ((rc = virGetDeviceID(disk_path, &major, &minor)) < 0) {
> +        virReportSystemError(-rc,
> +                             _("Unable to get minor number of device '%s'"),
> +                             disk_path);
> +        return NULL;
> +    }

virGetDeviceID fails for non-block devices; does that mean I'm going to
get an error message in the log for regular files?  Or do I have to
pre-filter for block devices before calling this function, which kind of
defeats the point of virGetDeviceID also doing the filtering?

I'm wondering if it would be better to make virGetDeviceID return 0 on
success for a block device, and 1 on success for a regular file, and
reserve negative returns for actual errors (OOM, stat() failed, ...).
Then, you could blindly call this function, and be silent on the common
case of a regular file, while still emitting useful error messages where
they matter.
[edit - read more below before you change anything...]

> +
> +    if (virAsprintf(&key, "%d:%d", major, minor) < 0) {
> +        virReportOOMError();
> +        return NULL;
> +    }
> +
> +    return key;
> +}
> +
> +/* Increase ref count if the entry already exists, otherwise
> + * add a new entry.
> + */
> +int
> +qemuAddSharedDisk(virHashTablePtr sharedDisks,
> +                  const char *disk_path)
> +{
> +    size_t *ref = NULL;
> +    char *key = NULL;
> +
> +    if (!(key = qemuGetSharedDiskKey(disk_path)))
> +        return -1;

Hmm, here you would have to be able to distinguish between a NULL
because an error message was issued, vs. NULL because disk_path was a
regular file rather than a block device so there is nothing to do.  That
may mean rewriting things to:
int qemuGetSharedDiskKey(const char *disk_path, char **result)

> @@ -6041,6 +6045,15 @@ qemuDomainAttachDeviceDiskLive(virConnectPtr conn,
>              VIR_WARN("Failed to teardown cgroup for disk path %s",
>                       NULLSTR(disk->src));
>      }
> +
> +    if (ret == 0 &&
> +        disk->type == VIR_DOMAIN_DISK_TYPE_BLOCK &&
> +        disk->shared) {

Or maybe I should just read more of your patch first.  It looks like you
are indeed pre-filtering and that you expect block devices from the
get-go, and that it is appropriate to error out on regular files.

So in spite of my musings earlier in this patch, it looks like you are
doing everything correctly.  ACK, but again, delay until after 1.0.1.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 619 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20121214/61424878/attachment-0001.sig>