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

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.