[libvirt] [PATCH 6/8 v8] qemu: Check if the shared disk's cdbfilter conflicts with others

[Eric Blake]

On 12/14/2012 08:44 AM, Osier Yang wrote:
> This prevents the domain starting if the shared disk's setting
> conflicts with other active domain(s), E.g. A domain with
> "cdbfilter" set as "yes", however, another active domain is using
> it set as "no".
> ---
>  src/qemu/qemu_process.c |   51 +++++++++++++++++++++++++++++++++++++++++++++++
>  src/qemu/qemu_process.h |    2 +
>  2 files changed, 53 insertions(+), 0 deletions(-)
> 
> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
> index bcea0ff..1e2baac 100644
> --- a/src/qemu/qemu_process.c
> +++ b/src/qemu/qemu_process.c
> @@ -3385,6 +3385,54 @@ qemuProcessReconnectAll(virConnectPtr conn, virQEMUDriverPtr driver)
>      virHashForEach(driver->domains.objs, qemuProcessReconnectHelper, &data);
>  }
>  
> +/* Check if a shared disk's setting conflicts with the conf
> + * used by other domain(s). Currently only checks the cdbfilter
> + * setting.

Probably worth documenting that this function should only be called on
disks with a block device source...

> + *
> + * Returns 0 if no conflicts, otherwise returns -1.
> + */
> +int
> +qemuCheckSharedDisk(virHashTablePtr sharedDisks,
> +                    virDomainDiskDefPtr disk)
> +{
> +    int val;
> +    size_t *ref = NULL;
> +    char *key = NULL;
> +    int ret = 0;
> +
> +    if (!(key = qemuGetSharedDiskKey(disk->src)))
> +        return -1;

since you error out on regular files.  (You correctly obey that
restriction for now, but without documentation, it might be possible to
reuse this function from the wrong context in a future patch.)

> +
> +    /* It can't be conflict if no other domain is
> +     * is sharing it.
> +     */
> +    if (!(ref = virHashLookup(sharedDisks, key)))
> +        goto cleanup;
> +
> +    if (ref == (void *)0x1)
> +        goto cleanup;
> +
> +    if (virGetDeviceUnprivSGIO(disk->src, NULL, &val) < 0) {
> +        ret = -1;
> +        goto cleanup;
> +    }
> +
> +    if ((val == 0 &&
> +         disk->cdbfilter == VIR_DOMAIN_DISK_CDB_FILTER_YES) ||

Does this do the right thing if disk->cdbfilter ==
VIR_DOMAIN_DISK_CDB_FILTER_DEFAULT (that is, the user didn't specify the
cdbfilter attribute)?  Again, this area of code may need tweaks if you
take my suggestion of making rawio a three-value enum.

> +        (val == 1 &&
> +         disk->cdbfilter == VIR_DOMAIN_DISK_CDB_FILTER_NO))
> +        goto cleanup;
> +
> +    virReportError(VIR_ERR_INTERNAL_ERROR,
> +                   _("cdbfilter of shared disk '%s' conflicts with other "
> +                     "active domains"), disk->src);

INTERNAL_ERROR is wrong.  I think VIR_ERR_OPERATION_INVALID fits better.

-- 
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/eef5720e/attachment-0001.sig>