[libvirt] [PATCH 10/15] qemu: Error out when domain starting if the cdbfilter setting conflicts

[Osier Yang]

On 2012年12月05日 18:50, Osier Yang wrote:
> On 2012年12月05日 18:20, Jiri Denemark wrote:
>> On Wed, Dec 05, 2012 at 17:25:11 +0800, 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".
>>>
>> ...
>>> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
>>> index 05f8622..2938a65 100644
>>> --- a/src/qemu/qemu_process.c
>>> +++ b/src/qemu/qemu_process.c
>>> @@ -3712,12 +3712,56 @@ int qemuProcessStart(virConnectPtr conn,
>>> if (disk->rawio == 1)
>>> virCommandAllowCap(cmd, CAP_SYS_RAWIO);
>>>
>>> - /* Add to qemud_driver->sharedDisks list if the disk is shared */
>>> - if (disk->shared&&
>>> - (qemuSharedDiskListAdd(driver->sharedDisks,
>>> - disk->src,
>>> - vm->def->name)< 0)) {
>>> - goto cleanup;
>>> + if (disk->shared) {
>>> + /* Error out if the cdbfilter setting is different with what
>>> + * other domain(s) uses.
>>> + */
>>> + qemuSharedDiskPtr entry = NULL;
>>> +
>>> + if ((entry = qemuSharedDiskListFind(driver->sharedDisks,
>>> + disk->src,
>>> + NULL,
>>> + NULL))) {
>>> + virDomainObjUnlock(vm);
>>> + for (i = 0; i< entry->ndomains; i++) {
>>> + virDomainObjPtr domobj = NULL;
>>> + virDomainDiskDefPtr diskdef = NULL;
>>> +
>>> + if (!(domobj = virDomainFindByName(&driver->domains,
>>> + entry->domains[i]))) {
>>> + virDomainObjLock(vm);
>>> + goto cleanup;
>>> + }
>>> +
>>> + if (!(diskdef = virDomainDiskFindByPath(domobj->def,
>>> + disk->src))) {
>>> + virDomainObjUnlock(domobj);
>>> + virDomainObjLock(vm);
>>> + goto cleanup;
>>> + }
>>> +
>>> + /* XXX: Can be abstracted into a function when there
>>> + * are more stuffs to check in future.
>>> + */
>>> + if (diskdef->cdbfilter != disk->cdbfilter) {
>>> + virReportError(VIR_ERR_INTERNAL_ERROR,
>>> + _("cdbfilter of shared disk '%s' "
>>> + "conflicts with other active "
>>> + "domains"), disk->src);
>>> + virDomainObjUnlock(domobj);
>>> + virDomainObjLock(vm);
>>> + goto cleanup;
>>> + }
>>> + virDomainObjUnlock(domobj);
>>> + }
>>> + virDomainObjLock(vm);
>>> + }
>>
>> NACK. This is still doing what Daniel complained about in v1. Although
>> it does
>> not go through all domains it is still locking some other domains. Not to
>> mention that it is absolute overkill to do.
>
> I mentioned in v4 that it's not that comfortable, especially for the
> nest locks, but I don't have better idea yet.
>
>
> IIUC, all domains in entry-domains
>> has to have exactly the same cdbfilter otherwise libvirt would refuse
>> to start
>> them.
>
> For this patch only, yes.
>
> And why do we even need to maintain a list of domains sharing the same
>> disk? Wouldn't just storing the value of cdbfilter there be enough? It
>> seems
>> like it would, at least for this purpose of checking that it matches
>> cdbfilter
>> specified in current domain.
>>
>
> Yes if it's for only this purpose (Prevent domain starting). But the
> truth is that we have to care about restoring the disk's unpriv_sgio
> too. I.E, checking if there is domain still using the disk. But you
> will say having a ref count will solve this problem.
>
> However, the mainly reason I choosed to use a sub-list of domain names
> is for future extenstion, I.E. Assuming there are other disk setting
> (you never known how many they will be), we have to guarantee they are
> same among guests in future. Looking up the disk def with domain and
> disk path gives us much flexibility IMHO.
>

So the point of the argument is: the trade between the flexibility and
the uncomfortable locks.

Regards,
Osier