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
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list