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