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

[Osier Yang]

On 2012年12月15日 05:49, Eric Blake wrote:
> 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.)

Okay.

>
>> +
>> +    /* 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)?

Oh, right, there is problem here. It should be like:

     if (!disk->cdbfilter)
          return 0

in the early beginnng of the function.

As VIR_DOMAIN_DISK_CDB_FILTER_DEFAULT implies it doesn't care
about the SG_IO setting. I.E, It lives with what current setting
is.


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

Okay, the error number is always fuzzy to use. :-)