A single entry of the list is constructed by the _disk path_ and a sub-list of domain names which are using the disk. * src/qemu/qemu_conf.h (New struct qemuSharedDisk, qemuSharedDiskList; New type qemuSharedDiskPtr, qemuSharedDiskListPtr; Declare the helpers qemuSharedDiskListFree, qemuSharedDiskListAdd, qemuSharedDiskListFind, qemuSharedDiskListDel) * src/qemu/qemu_conf.c (Implement the helpers) --- src/qemu/qemu_conf.c | 170 ++++++++++++++++++++++++++++++++++++++++++++++++++ src/qemu/qemu_conf.h | 30 +++++++++ 2 files changed, 200 insertions(+), 0 deletions(-) diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c index 8d380a1..4fdfb8c 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -556,3 +556,173 @@ qemuDriverCloseCallbackRunAll(virQEMUDriverPtr driver, virHashForEach(driver->closeCallbacks, qemuDriverCloseCallbackRun, &data); } + +void +qemuSharedDiskListFree(qemuSharedDiskListPtr list) +{ + int i; + int j; + + for (i = 0; i < list->ndisks; i++) { + VIR_FREE(list->disks[i]->path); + + for (j = 0; j < list->disks[i]->ndomains; j++) + VIR_FREE(list->disks[i]->domains[j]); + VIR_FREE(list->disks[i]->domains); + VIR_FREE(list->disks[i]); + } + VIR_FREE(list); +} + +/* Return the matched entry on success, with @idx set as + * the index of the matched entry. Or NULL on failure. + * + * If @domain_name is passed as NULL, it simply returns + * the entry which matches the @disk_path. + */ +qemuSharedDiskPtr +qemuSharedDiskListFind(qemuSharedDiskListPtr list, + const char *disk_path, + const char *domain_name, + int *idx) +{ + int i; + int j; + + for (i = 0; i < list->ndisks; i++) { + if (STREQ(disk_path, list->disks[i]->path)) { + if (!domain_name) + return list->disks[i]; + + for (j = 0; j < list->disks[i]->ndomains; j++) { + if (STREQ(domain_name, list->disks[i]->domains[j])) { + *idx = i; + return list->disks[i]; + } + } + } + } + + return NULL; +} + +int +qemuSharedDiskListAdd(qemuSharedDiskListPtr list, + const char *disk_path, + const char *domain_name) +{ + int i; + bool existed = false; + + if (qemuSharedDiskListFind(list, disk_path, domain_name, NULL)) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("disk '%s' of domain '%s' is already " + "maintained"), disk_path, domain_name); + return -1; + } + + for (i = 0; i < list->ndisks; i++) { + qemuSharedDiskPtr disk = list->disks[i]; + + /* Append the domain name to the existed entry */ + if (STREQ(disk->path, disk_path)) { + if (VIR_REALLOC_N(disk->domains, + disk->ndomains + 1) < 0) { + virReportOOMError(); + return -1; + } + + disk->domains[disk->ndomains++] = strdup(domain_name); + existed = true; + break; + } + } + + /* No existed entry for the @disk_path yet */ + if (!existed) { + if (VIR_REALLOC_N(list->disks, list->ndisks + 1) < 0) { + virReportOOMError(); + return -1; + } + + qemuSharedDiskPtr disk = NULL; + + if ((VIR_ALLOC(disk) < 0) || + (VIR_ALLOC_N(disk->domains, 1) < 0)) { + virReportOOMError(); + return -1; + } + + disk->path = strdup(disk_path); + disk->ndomains = 1; + disk->domains[0] = strdup(domain_name); + + list->disks[list->ndisks] = disk; + list->ndisks++; + } + + return 0; +} + +int +qemuSharedDiskListDel(qemuSharedDiskListPtr list, + const char *disk_path, + const char *domain_name) +{ + qemuSharedDiskPtr disk = NULL; + int i; + int idx; + + if (!qemuSharedDiskListFind(list, disk_path, domain_name, &idx)) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("disk '%s' of domain '%s' is not " + "maintained yet"), disk_path, domain_name); + return -1; + } + + disk = list->disks[idx]; + + if (disk->ndomains == 1) { + /* Free the disk entry if there is only one domain using it */ + if (idx != list->ndisks - 1) + memmove(&list->disks[idx], + &list->disks[idx + 1], + sizeof(*list->disks) * (list->ndisks - idx - 1)); + + if (VIR_REALLOC_N(list->disks, list->ndisks - 1) < 0) { + virReportOOMError(); + return -1; + } + + VIR_FREE(disk->domains[0]); + VIR_FREE(disk->domains); + VIR_FREE(disk->path); + VIR_FREE(disk); + list->ndisks--; + } else { + /* Otherwise just remove the domain name from the sub-list + * list->disk[i]->domains. + */ + for (i = 0; i < disk->ndomains; i++) { + if (STREQ(disk->domains[i], domain_name)) { + char *name = disk->domains[i]; + + if (i != disk->ndomains - 1) + memmove(&disk->domains[i], + &disk->domains[i + 1], + sizeof(*disk->domains) * (disk->ndomains - i - 1)); + + if (VIR_REALLOC_N(disk->domains, disk->ndomains - 1) < 0) { + virReportOOMError(); + return -1; + } + + disk->ndomains--; + VIR_FREE(name); + break; + } + } + } + + return 0; +} diff --git a/src/qemu/qemu_conf.h b/src/qemu/qemu_conf.h index d0d25ce..01768f7 100644 --- a/src/qemu/qemu_conf.h +++ b/src/qemu/qemu_conf.h @@ -53,6 +53,22 @@ typedef qemuDriverCloseDef *qemuDriverCloseDefPtr; typedef struct _virQEMUDriver virQEMUDriver; typedef virQEMUDriver *virQEMUDriverPtr; +typedef struct _qemuSharedDisk qemuSharedDisk; +typedef struct _qemuSharedDiskList qemuSharedDiskList; +typedef qemuSharedDiskList *qemuSharedDiskListPtr; +typedef qemuSharedDisk *qemuSharedDiskPtr; + +struct _qemuSharedDisk { + char *path; /* Disk path */ + char **domains; /* List of domain names which share the disk */ + int ndomains; +}; + +struct _qemuSharedDiskList { + int ndisks; + qemuSharedDiskPtr *disks; +}; + /* Main driver state */ struct _virQEMUDriver { virMutex lock; @@ -147,6 +163,8 @@ struct _virQEMUDriver { /* The devices which is are not in use by the host or any guest. */ pciDeviceList *inactivePciHostdevs; + qemuSharedDiskListPtr sharedDisks; + virBitmapPtr reservedRemotePorts; virSysinfoDefPtr hostsysinfo; @@ -211,4 +229,16 @@ qemuDriverCloseCallback qemuDriverCloseCallbackGet(virQEMUDriverPtr driver, void qemuDriverCloseCallbackRunAll(virQEMUDriverPtr driver, virConnectPtr conn); +void qemuSharedDiskListFree(qemuSharedDiskListPtr list); +int qemuSharedDiskListAdd(qemuSharedDiskListPtr list, + const char *disk_path, + const char *domain_name); +qemuSharedDiskPtr qemuSharedDiskListFind(qemuSharedDiskListPtr list, + const char *disk_path, + const char *domain_name, + int *idx); +int qemuSharedDiskListDel(qemuSharedDiskListPtr list, + const char *disk_path, + const char *domain_name); + #endif /* __QEMUD_CONF_H */ -- 1.7.7.6

Lifecyle here only means starting and shutdown. --- src/qemu/qemu_process.c | 20 ++++++++++++++++++++ 1 files changed, 20 insertions(+), 0 deletions(-) diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index ab04599..a44cca1 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -3708,6 +3708,13 @@ int qemuProcessStart(virConnectPtr conn, for (i = 0; i < vm->def->ndisks; i++) { if (vm->def->disks[i]->rawio == 1) virCommandAllowCap(cmd, CAP_SYS_RAWIO); + + if (vm->def->disks[i]->shared && + (qemuSharedDiskListAdd(driver->sharedDisks, + vm->def->disks[i]->src, + vm->def->name) < 0)) { + goto cleanup; + } } virCommandSetPreExecHook(cmd, qemuProcessHook, &hookData); @@ -4104,6 +4111,19 @@ void qemuProcessStop(virQEMUDriverPtr driver, flags & VIR_QEMU_PROCESS_STOP_MIGRATED); virSecurityManagerReleaseLabel(driver->securityManager, vm->def); + /* Remove the shared disk entry from qemud_driver->sharedDisks */ + for (i = 0; i < vm->def->ndisks; i++) { + if (vm->def->disks[i]->shared && + (qemuSharedDiskListDel(driver->sharedDisks, + vm->def->disks[i]->src, + vm->def->name) < 0)) { + VIR_WARN("Unable to remove shared disk entry for " + "disk = '%s', domain = '%s'", + vm->def->disks[i]->src, + vm->def->name); + } + } + /* Clear out dynamically assigned labels */ for (i = 0; i < vm->def->nseclabels; i++) { if (vm->def->seclabels[i]->type == VIR_DOMAIN_SECLABEL_DYNAMIC) { -- 1.7.7.6

Since "rawio" and "cdbfilter" are only valid for "lun", this groups them together; And since both of them intend to allow the unprivledged user to use the SG_IO commands, they must be exclusive. --- docs/formatdomain.html.in | 13 +++++++++- docs/schemas/domaincommon.rng | 52 ++++++++++++++++++++++++++++------------ 2 files changed, 48 insertions(+), 17 deletions(-) diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index 0574e68..cab6952 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -1395,7 +1395,18 @@ rawio='yes', rawio capability will be enabled for all disks in the domain (because, in the case of QEMU, this capability can only be set on a per-process basis). This attribute is only - valid when device is "lun". + valid when device is "lun". NB, <code>rawio</code> intends to + confine the capability per-device, however, current QEMU + implementation gives the domain process broader capability + than that (per-process basis, affects all the domain disks). + To confine the capability as much as possible for QEMU driver + as this stage, <code>cdbfilter</code> is recommended. + The optional <code>cdbfilter</code> attribute + (<span class="since">since 1.0.1</span>) indicates whether the + kernel will filter unprivileged SG_IO for the disk, valid settings + are "yes" or "no" (defaults to "yes"). Note that it's exclusive + with attribute <code>rawio</code>; Same with <code>rawio</code>, + <code>cdbfilter</code> is only valid for device 'lun'. The optional <code>snapshot</code> attribute indicates the default behavior of the disk during disk snapshots: "internal" requires a file format such as qcow2 that can store both the diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index 0e85739..4113521 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -957,24 +957,44 @@ --> <define name="disk"> <element name="disk"> - <optional> - <attribute name="device"> - <choice> - <value>floppy</value> - <value>disk</value> - <value>cdrom</value> - <value>lun</value> - </choice> - </attribute> - </optional> - <optional> - <attribute name="rawio"> + <choice> + <group> + <optional> + <attribute name="device"> + <choice> + <value>floppy</value> + <value>disk</value> + <value>cdrom</value> + </choice> + </attribute> + </optional> + </group> + <group> + <optional> + <attribute name="device"> + <value>lun</value> + </attribute> + </optional> <choice> - <value>yes</value> - <value>no</value> + <optional> + <attribute name="rawio"> + <choice> + <value>yes</value> + <value>no</value> + </choice> + </attribute> + </optional> + <optional> + <attribute name="cdbfilter"> + <choice> + <value>yes</value> + <value>no</value> + </choice> + </attribute> + </optional> </choice> - </attribute> - </optional> + </group> + </choice> <optional> <ref name="snapshot"/> </optional> -- 1.7.7.6

"virGetDevice{Major,Minor}" could be used across the sources, but it doesn't relate with this series, and could be done later. * src/util/util.h: (Declare virGetDevice{Major,Minor}, and vir{Get,Set}DeviceUnprivSGIO) * src/util/util.c: (Implement virGetDevice{Major,Minor} and vir{Get,Set}DeviceUnprivSGIO) * src/libvirt_private.syms: Export private symbols of upper helpers --- src/libvirt_private.syms | 4 + src/util/util.c | 145 ++++++++++++++++++++++++++++++++++++++++++++++ src/util/util.h | 7 ++ 3 files changed, 156 insertions(+), 0 deletions(-) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index df54f1e..78e4a5c 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1271,6 +1271,9 @@ virFileWaitForDevices; virFileWriteStr; virFindFileInPath; virFormatIntDecimal; +virGetDeviceMajor; +virGetDeviceMinor; +virGetDeviceUnprivSGIO; virGetGroupID; virGetGroupName; virGetHostname; @@ -1289,6 +1292,7 @@ virPipeReadUntilEOF; virScaleInteger; virSetBlocking; virSetCloseExec; +virSetDeviceUnprivSGIO; virSetInherit; virSetNonBlock; virSetUIDGID; diff --git a/src/util/util.c b/src/util/util.c index 2fd0f2c..4dab6ac 100644 --- a/src/util/util.c +++ b/src/util/util.c @@ -3113,3 +3113,148 @@ virValidateWWN(const char *wwn) { return true; } + +#if defined(major) && defined(minor) +int +virGetDeviceMajor(const char *path) +{ + struct stat sb; + + if (stat(path, &sb) < 0) + return -errno; + + if (!S_ISBLK(sb.st_mode)) + return -EINVAL; + + return major(sb.st_rdev); +} + +int +virGetDeviceMinor(const char *path) +{ + struct stat sb; + + if (stat(path, &sb) < 0) + return -errno; + + if (!S_ISBLK(sb.st_mode)) + return -EINVAL; + + return minor(sb.st_rdev); +} +#else +int +virGetDeviceMajor(const char *path) +{ + return -ENOSYS; +} + +int +virGetDeviceMinor(const char *path) +{ + return -ENOSYS; +} +#endif + +static char * +virGetUnprivSGIOSysfsPath(const char *path) +{ + int major, minor; + char *sysfs_path = NULL; + + if ((major = virGetDeviceMajor(path)) < 0) { + virReportSystemError(-major, + _("Unable to get major number of device '%s'"), + path); + return NULL; + } + + if ((minor = virGetDeviceMinor(path)) < 0) { + virReportSystemError(-minor, + _("Unable to get minor number of device '%s'"), + path); + return NULL; + } + + if (virAsprintf(&sysfs_path, "/sys/dev/block/%d:%d/queue/unpriv_sgio", + major, minor) < 0) { + virReportOOMError(); + return NULL; + } + + return sysfs_path; +} + +int +virSetDeviceUnprivSGIO(const char *path, + int unpriv_sgio) +{ + char *sysfs_path = NULL; + char *val = NULL; + int ret = -1; + int rc = -1; + + if (!(sysfs_path = virGetUnprivSGIOSysfsPath(path))) + return -1; + + if (!virFileExists(sysfs_path)) { + virReportError(VIR_ERR_OPERATION_INVALID, "%s", + _("unpriv_sgio is not supported by this kernel")); + goto cleanup; + } + + if (virAsprintf(&val, "%d", unpriv_sgio) < 0) { + virReportOOMError(); + goto cleanup; + } + + if ((rc = virFileWriteStr(sysfs_path, val, 0)) < 0) { + virReportSystemError(-rc, _("failed to set %s"), sysfs_path); + goto cleanup; + } + + ret = 0; +cleanup: + VIR_FREE(sysfs_path); + VIR_FREE(val); + return ret; +} + +int +virGetDeviceUnprivSGIO(const char *path, + int *unpriv_sgio) +{ + char *sysfs_path = NULL; + char *buf = NULL; + char *tmp = NULL; + int ret = -1; + int rc = -1; + + if (!(sysfs_path = virGetUnprivSGIOSysfsPath(path))) + return -1; + + if (!virFileExists(sysfs_path)) { + virReportError(VIR_ERR_OPERATION_INVALID, "%s", + _("unpriv_sgio is not supported by this kernel")); + goto cleanup; + } + + if (virFileReadAll(sysfs_path, 1024, &buf) < 0) + goto cleanup; + + if ((tmp = strchr(buf, '\n'))) + *tmp = '\0'; + + if ((rc = virStrToLong_i(buf, NULL, 10, + unpriv_sgio)) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("failed to parse value of %s"), sysfs_path); + goto cleanup; + } + + ret = 0; +cleanup: + VIR_FREE(sysfs_path); + VIR_FREE(buf); + return ret; +} diff --git a/src/util/util.h b/src/util/util.h index 4316ab1..41146c1 100644 --- a/src/util/util.h +++ b/src/util/util.h @@ -280,4 +280,11 @@ bool virIsDevMapperDevice(const char *dev_name) ATTRIBUTE_NONNULL(1); bool virValidateWWN(const char *wwn); +int virGetDeviceMajor(const char *path); +int virGetDeviceMinor(const char *path); +int virSetDeviceUnprivSGIO(const char *path, + int unpriv_sgio); +int virGetDeviceUnprivSGIO(const char *path, + int *unpriv_sgio); + #endif /* __VIR_UTIL_H__ */ -- 1.7.7.6

This prevents restoring the unpriv_sgio if the disk is shared, and is being used by other active domain. Because we don't want to fall into the corruption situation. --- src/qemu/qemu_process.c | 11 +++++++++++ 1 files changed, 11 insertions(+), 0 deletions(-) diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 3923272..05f8622 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -4151,6 +4151,17 @@ void qemuProcessStop(virQEMUDriverPtr driver, if (!disk->cdbfilter) continue; + /* Don't try to restore the unpriv_sgio if the disk is shared + * by other active domain(s). We don't want to fall into the + * corruptions. + */ + if (disk->shared && + qemuSharedDiskListFind(driver->sharedDisks, + disk->src, + NULL, + NULL)) + continue; + /* Restore sysfs unpriv_sgio for the disk */ if (disk->old_cdbfilter == VIR_DOMAIN_DISK_CDB_FILTER_YES) val = 0; -- 1.7.7.6

On Wed, Dec 05, 2012 at 17:25:11 +0800, Osier Yang wrote: ... 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. IIUC, all domains in entry-domains has to have exactly the same cdbfilter otherwise libvirt would refuse to start them. 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. Jirka

On 2012年12月05日 18:50, Osier Yang wrote: So the point of the argument is: the trade between the flexibility and the uncomfortable locks. Regards, Osier

Just like for domain starting, the original unpriv_sgio state is recorded for restoring when detaching. --- src/qemu/qemu_driver.c | 48 +++++++++++++++++++++++++++++++++++++----------- 1 files changed, 37 insertions(+), 11 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 2c36935..b75b6fe 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -6042,17 +6042,43 @@ qemuDomainAttachDeviceDiskLive(virConnectPtr conn, NULLSTR(disk->src)); } - /* If the attached disk is shareable, add it to the list - * qemud_driver->sharedDisks if attachment succeeded - */ - if (ret == 0 && disk->shared) { - if (qemuSharedDiskListAdd(driver->sharedDisks, - disk->src, - vm->def->name) < 0) { - VIR_DEBUG("Failed to add the disk '%s' of domain '%s' to " - "sharedDisks list", disk->src, vm->def->name); - } - } + if (ret == 0) { + if (disk->cdbfilter) { + int val; + + /* Store the original unpriv_sgio state */ + if (virGetDeviceUnprivSGIO(disk->src, &val) < 0) { + VIR_DEBUG("Failed to get unpriv_sgio of disk '%s'", disk->src); + goto end; + } + + if (val == 0) + disk->old_cdbfilter = VIR_DOMAIN_DISK_CDB_FILTER_YES; + else + disk->old_cdbfilter = VIR_DOMAIN_DISK_CDB_FILTER_NO; + + /* Set unpriv_sgio */ + if (virSetDeviceUnprivSGIO(disk->src, + (disk->cdbfilter == + VIR_DOMAIN_DISK_CDB_FILTER_NO) + ? 1 : 0) < 0) { + VIR_DEBUG("Failed to set unpriv_sgio of disk '%s'", disk->src); + goto end; + } + } + + /* If the attached disk is shareable, add it to the list + * qemud_driver->sharedDisks if attachment succeeded + */ + if (disk->shared) { + if (qemuSharedDiskListAdd(driver->sharedDisks, + disk->src, + vm->def->name) < 0) { + VIR_DEBUG("Failed to add the disk '%s' of domain '%s' to " + "sharedDisks list", disk->src, vm->def->name); + } + } + } end: if (cgroup) virCgroupFree(&cgroup); -- 1.7.7.6

Just like for domain starting, this checks if the shared disk's conf conflicts with others which are in use. Currently it only checks the setting of cdbfilter. * src/qemu/qemu_process.h (Abstract a helper function qemuCheckSharedDisk) * src/qemu/qemu_process.c (Implement the helper) * src/qemu/qemu_driver.c (Check the shared disk's conf) --- src/qemu/qemu_driver.c | 4 ++ src/qemu/qemu_process.c | 105 +++++++++++++++++++++++++++++------------------ src/qemu/qemu_process.h | 4 ++ 3 files changed, 73 insertions(+), 40 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index cedf636..7288ad6 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -5991,6 +5991,10 @@ qemuDomainAttachDeviceDiskLive(virConnectPtr conn, goto end; } + if (disk->shared && + (qemuCheckSharedDisk(driver, disk, vm) < 0)) + goto end; + if (qemuDomainDetermineDiskChain(driver, disk, false) < 0) goto end; diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 2938a65..b38fc7b 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -3380,6 +3380,68 @@ qemuProcessReconnectAll(virConnectPtr conn, virQEMUDriverPtr driver) virHashForEach(driver->domains.objs, qemuProcessReconnectHelper, &data); } +/* + * Check if the shared disk's configraution conflicts with others. + * + * Return 0 if there is no conflict, otherwise return -1. + */ +int +qemuCheckSharedDisk(virQEMUDriverPtr driver, + virDomainDiskDefPtr disk, + virDomainObjPtr vm) +{ + qemuSharedDiskPtr entry = NULL; + int i; + + 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]))) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Failed to look up domain '%s'"), + entry->domains[i]); + virDomainObjLock(vm); + return -1; + } + + if (!(diskdef = virDomainDiskFindByPath(domobj->def, + disk->src))) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Failed to find disk '%s' of domain '%s'"), + disk->src, domobj->def->name); + virDomainObjUnlock(domobj); + virDomainObjLock(vm); + return -1; + } + + /* 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); + return -1; + } + virDomainObjUnlock(domobj); + } + virDomainObjLock(vm); + } + + return 0; +} + + int qemuProcessStart(virConnectPtr conn, virQEMUDriverPtr driver, virDomainObjPtr vm, @@ -3716,46 +3778,9 @@ int qemuProcessStart(virConnectPtr conn, /* 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); - } + + if (qemuCheckSharedDisk(driver, disk, vm) < 0) + goto cleanup; /* Add to qemud_driver->sharedDisks list if the disk is shared */ if (qemuSharedDiskListAdd(driver->sharedDisks, diff --git a/src/qemu/qemu_process.h b/src/qemu/qemu_process.h index c12df32..501f206 100644 --- a/src/qemu/qemu_process.h +++ b/src/qemu/qemu_process.h @@ -99,4 +99,8 @@ bool qemuProcessAutoDestroyActive(virQEMUDriverPtr driver, virBitmapPtr qemuPrepareCpumap(virQEMUDriverPtr driver, virBitmapPtr nodemask); +int qemuCheckSharedDisk(virQEMUDriverPtr driver, + virDomainDiskDefPtr disk, + virDomainObjPtr vm); + #endif /* __QEMU_PROCESS_H__ */ -- 1.7.7.6

This allows the disk's original_unpriv value is not lost after restarting or reloading libvirtd. --- src/conf/domain_conf.c | 35 +++++++++++++++++++++++++++++++---- 1 files changed, 31 insertions(+), 4 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 1d6bb1f..bc77429 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -70,6 +70,7 @@ typedef enum { VIR_DOMAIN_XML_INTERNAL_PCI_ORIG_STATES = (1<<18), VIR_DOMAIN_XML_INTERNAL_ALLOW_ROM = (1<<19), VIR_DOMAIN_XML_INTERNAL_ALLOW_BOOT = (1<<20), + VIR_DOMAIN_XML_INTERNAL_OLD_DISK_CDBFILTER = (1<<21), } virDomainXMLInternalFlags; VIR_ENUM_IMPL(virDomainTaint, VIR_DOMAIN_TAINT_LAST, @@ -3544,6 +3545,7 @@ virDomainDiskDefParseXML(virCapsPtr caps, char *snapshot = NULL; char *rawio = NULL; char *cdbfilter = NULL; + char *old_cdbfilter = NULL; char *driverName = NULL; char *driverType = NULL; char *source = NULL; @@ -3607,6 +3609,8 @@ virDomainDiskDefParseXML(virCapsPtr caps, rawio = virXMLPropString(node, "rawio"); cdbfilter = virXMLPropString(node, "cdbfilter"); + if (flags & VIR_DOMAIN_XML_INTERNAL_OLD_DISK_CDBFILTER) + old_cdbfilter = virXMLPropString(node, "old_cdbfilter"); cur = node->children; while (cur != NULL) { @@ -4069,6 +4073,19 @@ virDomainDiskDefParseXML(virCapsPtr caps, def->cdbfilter = cdbfilter_val; } + if (old_cdbfilter) { + int old_cdbfilter_val = 0; + + if ((old_cdbfilter_val = + virDomainDiskCDBFilterTypeFromString(old_cdbfilter)) < 0) { + virReportError(VIR_ERR_XML_ERROR, + _("unknown disk cdbfilter setting '%s'"), + old_cdbfilter); + goto error; + } + def->old_cdbfilter = old_cdbfilter_val; + } + if (bus) { if ((def->bus = virDomainDiskBusTypeFromString(bus)) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, @@ -11985,6 +12002,12 @@ virDomainDiskDefFormat(virBufferPtr buf, if (def->cdbfilter) virBufferAsprintf(buf, " cdbfilter='%s'", virDomainDiskCDBFilterTypeToString(def->cdbfilter)); + + if ((flags & VIR_DOMAIN_XML_INTERNAL_OLD_DISK_CDBFILTER) && + def->old_cdbfilter) + virBufferAsprintf(buf, " old_cdbfilter='%s'", + virDomainDiskCDBFilterTypeToString(def->old_cdbfilter)); + if (def->snapshot && !(def->snapshot == VIR_DOMAIN_SNAPSHOT_LOCATION_NONE && def->readonly)) virBufferAsprintf(buf, " snapshot='%s'", @@ -13654,7 +13677,8 @@ virDomainIsAllVcpupinInherited(virDomainDefPtr def) verify(((VIR_DOMAIN_XML_INTERNAL_STATUS | VIR_DOMAIN_XML_INTERNAL_ACTUAL_NET | - VIR_DOMAIN_XML_INTERNAL_PCI_ORIG_STATES) + VIR_DOMAIN_XML_INTERNAL_PCI_ORIG_STATES | + VIR_DOMAIN_XML_INTERNAL_OLD_DISK_CDBFILTER) & DUMPXML_FLAGS) == 0); /* This internal version can accept VIR_DOMAIN_XML_INTERNAL_*, @@ -13676,7 +13700,8 @@ virDomainDefFormatInternal(virDomainDefPtr def, virCheckFlags(DUMPXML_FLAGS | VIR_DOMAIN_XML_INTERNAL_STATUS | VIR_DOMAIN_XML_INTERNAL_ACTUAL_NET | - VIR_DOMAIN_XML_INTERNAL_PCI_ORIG_STATES, + VIR_DOMAIN_XML_INTERNAL_PCI_ORIG_STATES | + VIR_DOMAIN_XML_INTERNAL_OLD_DISK_CDBFILTER, -1); if (!(type = virDomainVirtTypeToString(def->virtType))) { @@ -14434,7 +14459,8 @@ int virDomainSaveStatus(virCapsPtr caps, unsigned int flags = (VIR_DOMAIN_XML_SECURE | VIR_DOMAIN_XML_INTERNAL_STATUS | VIR_DOMAIN_XML_INTERNAL_ACTUAL_NET | - VIR_DOMAIN_XML_INTERNAL_PCI_ORIG_STATES); + VIR_DOMAIN_XML_INTERNAL_PCI_ORIG_STATES | + VIR_DOMAIN_XML_INTERNAL_OLD_DISK_CDBFILTER); int ret = -1; char *xml; @@ -14534,7 +14560,8 @@ static virDomainObjPtr virDomainLoadStatus(virCapsPtr caps, if (!(obj = virDomainObjParseFile(caps, statusFile, expectedVirtTypes, VIR_DOMAIN_XML_INTERNAL_STATUS | VIR_DOMAIN_XML_INTERNAL_ACTUAL_NET | - VIR_DOMAIN_XML_INTERNAL_PCI_ORIG_STATES))) + VIR_DOMAIN_XML_INTERNAL_PCI_ORIG_STATES | + VIR_DOMAIN_XML_INTERNAL_OLD_DISK_CDBFILTER))) goto error; virUUIDFormat(obj->def->uuid, uuidstr); -- 1.7.7.6