[libvirt] [PATCH 00/12 v6] Unprivleged SG_IO support
As a result of RFC [1], this implements the unprivleged SG_IO support. v4 - v5: * Per discussion in [2], this set uses a hash table to store the shared disk's info. See 1/12 for more details. Osier Yang (12): qemu: Add a hash table for the shared disks docs: Add docs and rng schema for new XML cdbfilter conf: Parse and format the new XML tag cdbfilter util: Prepare helpers for unpriv_sgio setting qemu: Manage disk's unpriv_sgio in domain lifecyle qemu: Do not restore the sysfs unpriv_sgio if the disk is being shared qemu: Error out when domain starting if the cdbfilter setting conflicts qemu: Set unpriv_sgio when attaching disk qemu: Restore unpriv_sgio when detaching disk qemu: Error out if the shared disk conf conflicts with others when attaching qemu: Do not restore unpriv_sgio when detaching disk if it's still shared conf: Save disk's original unpriv_sgio state into status XML docs/formatdomain.html.in | 13 ++- docs/schemas/domaincommon.rng | 52 +++++-- src/conf/domain_conf.c | 93 ++++++++++--- src/conf/domain_conf.h | 11 ++ src/libvirt_private.syms | 4 + src/qemu/qemu_conf.c | 60 ++++++++ src/qemu/qemu_conf.h | 22 +++ src/qemu/qemu_driver.c | 72 ++++++++++ src/qemu/qemu_process.c | 104 ++++++++++++++- src/qemu/qemu_process.h | 3 + src/util/util.c | 145 ++++++++++++++++++++ src/util/util.h | 8 + ...ml2argv-disk-scsi-lun-passthrough-cdbfilter.xml | 32 +++++ tests/qemuxml2xmltest.c | 1 + 14 files changed, 585 insertions(+), 35 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-scsi-lun-passthrough-cdbfilter.xml [1] https://www.redhat.com/archives/libvir-list/2012-November/msg00988.html [2] https://www.redhat.com/archives/libvir-list/2012-December/msg00325.html Regards, Osier
This introduces a hash table for qemu driver, to store the shared disk's info as (@disk_path, {@ref_count, @orig_cdbfilter}). @ref_count is the number of domains which shares the disk. @orig_cdbfilter is the original cdbfilter setting of the shared disk, it will be used to restore the the disk's cdbfilter setting to original value by later patches. * src/qemu/qemu_conf.h: (Add member 'sharedDisks' of type virHashTablePtr; New struct qemuSharedDiskEntry; Declare helpers qemuAddSharedDisk, qemuRemoveSharedDisk) * src/qemu/qemu_conf.c (Implement the two helpers) * src/qemu/qemu_process.c (Update 'sharedDisks' when domain starting and shutdown) * src/qemu/qemu_driver.c (Update 'sharedDisks' when attaching or detaching disk). 0 is passed for orig_cdbfilter temporarily, later patches will update it. --- src/qemu/qemu_conf.c | 48 +++++++++++++++++++++++++++++++++++++++++++++++ src/qemu/qemu_conf.h | 18 +++++++++++++++++ src/qemu/qemu_driver.c | 17 ++++++++++++++++ src/qemu/qemu_process.c | 17 +++++++++++++++- 4 files changed, 99 insertions(+), 1 deletions(-) diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c index 8d380a1..2b21186 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -556,3 +556,51 @@ qemuDriverCloseCallbackRunAll(virQEMUDriverPtr driver, virHashForEach(driver->closeCallbacks, qemuDriverCloseCallbackRun, &data); } + +/* Increase ref count if the entry already exists, otherwise + * add a new entry. + */ +int +qemuAddSharedDisk(virHashTablePtr sharedDisks, + const char *disk_path, + int orig_cdbfilter) +{ + qemuSharedDiskEntryPtr entry = NULL; + + if ((entry = virHashLookup(sharedDisks, disk_path))) { + entry->ref++; + } else { + if (VIR_ALLOC(entry) < 0) + return -1; + + entry->ref = 1; + entry->orig_cdbfilter = orig_cdbfilter; + + if (virHashAddEntry(sharedDisks, disk_path, entry)) + return -1; + } + + return 0; +} + +/* Decrease the ref count if the entry already exists, otherwise + * remove the entry. + */ +int +qemuRemoveSharedDisk(virHashTablePtr sharedDisks, + const char *disk_path) +{ + qemuSharedDiskEntryPtr entry = NULL; + + if (!(entry = virHashLookup(sharedDisks, disk_path))) + return -1; + + if (entry->ref != 1) { + entry->ref--; + } else { + if (virHashRemoveEntry(sharedDisks, disk_path) < 0) + return -1; + } + + return 0; +} diff --git a/src/qemu/qemu_conf.h b/src/qemu/qemu_conf.h index d0d25ce..df901f6 100644 --- a/src/qemu/qemu_conf.h +++ b/src/qemu/qemu_conf.h @@ -147,6 +147,8 @@ struct _virQEMUDriver { /* The devices which is are not in use by the host or any guest. */ pciDeviceList *inactivePciHostdevs; + virHashTablePtr sharedDisks; + virBitmapPtr reservedRemotePorts; virSysinfoDefPtr hostsysinfo; @@ -193,6 +195,13 @@ struct qemuDomainDiskInfo { int io_status; }; +typedef struct qemuSharedDiskEntry qemuSharedDiskEntry; +typedef qemuSharedDiskEntry *qemuSharedDiskEntryPtr; +struct qemuSharedDiskEntry { + size_t ref; /* ref count of the shared disk */ + int orig_cdbfilter; /* Original disk's cdbfilter setting */ +}; + typedef virDomainObjPtr (*qemuDriverCloseCallback)(virQEMUDriverPtr driver, virDomainObjPtr vm, virConnectPtr conn); @@ -211,4 +220,13 @@ qemuDriverCloseCallback qemuDriverCloseCallbackGet(virQEMUDriverPtr driver, void qemuDriverCloseCallbackRunAll(virQEMUDriverPtr driver, virConnectPtr conn); +int qemuAddSharedDisk(virHashTablePtr sharedDisks, + const char *disk_path, + int orig_cdbfilter) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); + +int qemuRemoveSharedDisk(virHashTablePtr sharedDisks, + const char *disk_path) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); + #endif /* __QEMUD_CONF_H */ diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index e099c5c..fdde74d 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -810,6 +810,9 @@ qemuStartup(bool privileged, if ((qemu_driver->inactivePciHostdevs = pciDeviceListNew()) == NULL) goto error; + if (!(qemu_driver->sharedDisks = virHashCreate(30, NULL))) + goto error; + if (privileged) { if (chown(qemu_driver->libDir, qemu_driver->user, qemu_driver->group) < 0) { virReportSystemError(errno, @@ -1064,6 +1067,7 @@ qemuShutdown(void) { pciDeviceListFree(qemu_driver->activePciHostdevs); pciDeviceListFree(qemu_driver->inactivePciHostdevs); usbDeviceListFree(qemu_driver->activeUsbHostdevs); + virHashFree(qemu_driver->sharedDisks); virCapabilitiesFree(qemu_driver->caps); qemuCapsCacheFree(qemu_driver->capsCache); @@ -6035,6 +6039,12 @@ qemuDomainAttachDeviceDiskLive(virConnectPtr conn, VIR_WARN("Failed to teardown cgroup for disk path %s", NULLSTR(disk->src)); } + + if (ret == 0 && disk->shared) { + if (qemuAddSharedDisk(driver->sharedDisks, disk->src, 0) < 0) + VIR_WARN("Failed to add disk '%s' to shared disk table", + disk->src); + } end: if (cgroup) virCgroupFree(&cgroup); @@ -6149,6 +6159,13 @@ qemuDomainDetachDeviceDiskLive(virQEMUDriverPtr driver, virDomainDiskDeviceTypeToString(disk->type)); break; } + + if (ret == 0 && disk->shared) { + if (qemuRemoveSharedDisk(driver->sharedDisks, disk->src) < 0) + VIR_WARN("Failed to remove disk '%s' from shared disk table", + disk->src); + } + return ret; } diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index ab04599..89152b8 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -3706,8 +3706,15 @@ int qemuProcessStart(virConnectPtr conn, /* in case a certain disk is desirous of CAP_SYS_RAWIO, add this */ for (i = 0; i < vm->def->ndisks; i++) { - if (vm->def->disks[i]->rawio == 1) + virDomainDiskDefPtr disk = vm->def->disks[i]; + + if (disk->rawio == 1) virCommandAllowCap(cmd, CAP_SYS_RAWIO); + + if (disk->shared) { + if (qemuAddSharedDisk(driver->sharedDisks, disk->src, 0) < 0) + goto cleanup; + } } virCommandSetPreExecHook(cmd, qemuProcessHook, &hookData); @@ -4104,6 +4111,14 @@ void qemuProcessStop(virQEMUDriverPtr driver, flags & VIR_QEMU_PROCESS_STOP_MIGRATED); virSecurityManagerReleaseLabel(driver->securityManager, vm->def); + for (i = 0; i < vm->def->ndisks; i++) { + virDomainDiskDefPtr disk = vm->def->disks[i]; + + if (disk->shared) { + ignore_value(qemuRemoveSharedDisk(driver->sharedDisks, disk->src)); + } + } + /* 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
On 2012年12月11日 21:37, Osier Yang wrote:
This introduces a hash table for qemu driver, to store the shared disk's info as (@disk_path, {@ref_count, @orig_cdbfilter}). @ref_count is the number of domains which shares the disk. @orig_cdbfilter is the original cdbfilter setting of the shared disk, it will be used to restore the the disk's cdbfilter setting to original value by later patches.
* src/qemu/qemu_conf.h: (Add member 'sharedDisks' of type virHashTablePtr; New struct qemuSharedDiskEntry; Declare helpers qemuAddSharedDisk, qemuRemoveSharedDisk) * src/qemu/qemu_conf.c (Implement the two helpers) * src/qemu/qemu_process.c (Update 'sharedDisks' when domain starting and shutdown) * src/qemu/qemu_driver.c (Update 'sharedDisks' when attaching or detaching disk).
0 is passed for orig_cdbfilter temporarily, later patches will update it. --- src/qemu/qemu_conf.c | 48 +++++++++++++++++++++++++++++++++++++++++++++++ src/qemu/qemu_conf.h | 18 +++++++++++++++++ src/qemu/qemu_driver.c | 17 ++++++++++++++++ src/qemu/qemu_process.c | 17 +++++++++++++++- 4 files changed, 99 insertions(+), 1 deletions(-)
diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c index 8d380a1..2b21186 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -556,3 +556,51 @@ qemuDriverCloseCallbackRunAll(virQEMUDriverPtr driver,
virHashForEach(driver->closeCallbacks, qemuDriverCloseCallbackRun,&data); } + +/* Increase ref count if the entry already exists, otherwise + * add a new entry. + */ +int +qemuAddSharedDisk(virHashTablePtr sharedDisks, + const char *disk_path, + int orig_cdbfilter) +{ + qemuSharedDiskEntryPtr entry = NULL; + + if ((entry = virHashLookup(sharedDisks, disk_path))) { + entry->ref++; + } else { + if (VIR_ALLOC(entry)< 0) + return -1; + + entry->ref = 1; + entry->orig_cdbfilter = orig_cdbfilter; + + if (virHashAddEntry(sharedDisks, disk_path, entry)) + return -1; + } + + return 0; +} + +/* Decrease the ref count if the entry already exists, otherwise + * remove the entry. + */ +int +qemuRemoveSharedDisk(virHashTablePtr sharedDisks, + const char *disk_path) +{ + qemuSharedDiskEntryPtr entry = NULL; + + if (!(entry = virHashLookup(sharedDisks, disk_path))) + return -1; + + if (entry->ref != 1) { + entry->ref--; + } else { + if (virHashRemoveEntry(sharedDisks, disk_path)< 0) + return -1; + } + + return 0; +} diff --git a/src/qemu/qemu_conf.h b/src/qemu/qemu_conf.h index d0d25ce..df901f6 100644 --- a/src/qemu/qemu_conf.h +++ b/src/qemu/qemu_conf.h @@ -147,6 +147,8 @@ struct _virQEMUDriver { /* The devices which is are not in use by the host or any guest. */ pciDeviceList *inactivePciHostdevs;
+ virHashTablePtr sharedDisks; + virBitmapPtr reservedRemotePorts;
virSysinfoDefPtr hostsysinfo; @@ -193,6 +195,13 @@ struct qemuDomainDiskInfo { int io_status; };
+typedef struct qemuSharedDiskEntry qemuSharedDiskEntry; +typedef qemuSharedDiskEntry *qemuSharedDiskEntryPtr; +struct qemuSharedDiskEntry { + size_t ref; /* ref count of the shared disk */ + int orig_cdbfilter; /* Original disk's cdbfilter setting */ +}; + typedef virDomainObjPtr (*qemuDriverCloseCallback)(virQEMUDriverPtr driver, virDomainObjPtr vm, virConnectPtr conn); @@ -211,4 +220,13 @@ qemuDriverCloseCallback qemuDriverCloseCallbackGet(virQEMUDriverPtr driver, void qemuDriverCloseCallbackRunAll(virQEMUDriverPtr driver, virConnectPtr conn);
+int qemuAddSharedDisk(virHashTablePtr sharedDisks, + const char *disk_path, + int orig_cdbfilter) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); + +int qemuRemoveSharedDisk(virHashTablePtr sharedDisks, + const char *disk_path) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); + #endif /* __QEMUD_CONF_H */ diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index e099c5c..fdde74d 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -810,6 +810,9 @@ qemuStartup(bool privileged, if ((qemu_driver->inactivePciHostdevs = pciDeviceListNew()) == NULL) goto error;
+ if (!(qemu_driver->sharedDisks = virHashCreate(30, NULL))) + goto error; + if (privileged) { if (chown(qemu_driver->libDir, qemu_driver->user, qemu_driver->group)< 0) { virReportSystemError(errno, @@ -1064,6 +1067,7 @@ qemuShutdown(void) { pciDeviceListFree(qemu_driver->activePciHostdevs); pciDeviceListFree(qemu_driver->inactivePciHostdevs); usbDeviceListFree(qemu_driver->activeUsbHostdevs); + virHashFree(qemu_driver->sharedDisks); virCapabilitiesFree(qemu_driver->caps); qemuCapsCacheFree(qemu_driver->capsCache);
@@ -6035,6 +6039,12 @@ qemuDomainAttachDeviceDiskLive(virConnectPtr conn, VIR_WARN("Failed to teardown cgroup for disk path %s", NULLSTR(disk->src)); } + + if (ret == 0&& disk->shared) { + if (qemuAddSharedDisk(driver->sharedDisks, disk->src, 0)< 0) + VIR_WARN("Failed to add disk '%s' to shared disk table", + disk->src); + } end: if (cgroup) virCgroupFree(&cgroup); @@ -6149,6 +6159,13 @@ qemuDomainDetachDeviceDiskLive(virQEMUDriverPtr driver, virDomainDiskDeviceTypeToString(disk->type)); break; } + + if (ret == 0&& disk->shared) { + if (qemuRemoveSharedDisk(driver->sharedDisks, disk->src)< 0) + VIR_WARN("Failed to remove disk '%s' from shared disk table", + disk->src); + } + return ret; }
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index ab04599..89152b8 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -3706,8 +3706,15 @@ int qemuProcessStart(virConnectPtr conn,
/* in case a certain disk is desirous of CAP_SYS_RAWIO, add this */ for (i = 0; i< vm->def->ndisks; i++) { - if (vm->def->disks[i]->rawio == 1) + virDomainDiskDefPtr disk = vm->def->disks[i]; + + if (disk->rawio == 1) virCommandAllowCap(cmd, CAP_SYS_RAWIO); + + if (disk->shared) { + if (qemuAddSharedDisk(driver->sharedDisks, disk->src, 0)< 0) + goto cleanup; + } }
virCommandSetPreExecHook(cmd, qemuProcessHook,&hookData); @@ -4104,6 +4111,14 @@ void qemuProcessStop(virQEMUDriverPtr driver, flags& VIR_QEMU_PROCESS_STOP_MIGRATED); virSecurityManagerReleaseLabel(driver->securityManager, vm->def);
+ for (i = 0; i< vm->def->ndisks; i++) { + virDomainDiskDefPtr disk = vm->def->disks[i]; + + if (disk->shared) { + ignore_value(qemuRemoveSharedDisk(driver->sharedDisks, disk->src)); + } + } + /* Clear out dynamically assigned labels */ for (i = 0; i< vm->def->nseclabels; i++) { if (vm->def->seclabels[i]->type == VIR_DOMAIN_SECLABEL_DYNAMIC) {
With the diff below squashed in: (the dataFree was missed when changing the hash value from only ref count to a struct).
On 2012年12月12日 11:35, Osier Yang wrote:
On 2012年12月11日 21:37, Osier Yang wrote:
This introduces a hash table for qemu driver, to store the shared disk's info as (@disk_path, {@ref_count, @orig_cdbfilter}). @ref_count is the number of domains which shares the disk. @orig_cdbfilter is the original cdbfilter setting of the shared disk, it will be used to restore the the disk's cdbfilter setting to original value by later patches.
* src/qemu/qemu_conf.h: (Add member 'sharedDisks' of type virHashTablePtr; New struct qemuSharedDiskEntry; Declare helpers qemuAddSharedDisk, qemuRemoveSharedDisk) * src/qemu/qemu_conf.c (Implement the two helpers) * src/qemu/qemu_process.c (Update 'sharedDisks' when domain starting and shutdown) * src/qemu/qemu_driver.c (Update 'sharedDisks' when attaching or detaching disk).
0 is passed for orig_cdbfilter temporarily, later patches will update it. --- src/qemu/qemu_conf.c | 48 +++++++++++++++++++++++++++++++++++++++++++++++ src/qemu/qemu_conf.h | 18 +++++++++++++++++ src/qemu/qemu_driver.c | 17 ++++++++++++++++ src/qemu/qemu_process.c | 17 +++++++++++++++- 4 files changed, 99 insertions(+), 1 deletions(-)
diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c index 8d380a1..2b21186 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -556,3 +556,51 @@ qemuDriverCloseCallbackRunAll(virQEMUDriverPtr driver,
virHashForEach(driver->closeCallbacks, qemuDriverCloseCallbackRun,&data); } + +/* Increase ref count if the entry already exists, otherwise + * add a new entry. + */ +int +qemuAddSharedDisk(virHashTablePtr sharedDisks, + const char *disk_path, + int orig_cdbfilter) +{ + qemuSharedDiskEntryPtr entry = NULL; + + if ((entry = virHashLookup(sharedDisks, disk_path))) { + entry->ref++; + } else { + if (VIR_ALLOC(entry)< 0) + return -1; + + entry->ref = 1; + entry->orig_cdbfilter = orig_cdbfilter; + + if (virHashAddEntry(sharedDisks, disk_path, entry)) + return -1; + } + + return 0; +} + +/* Decrease the ref count if the entry already exists, otherwise + * remove the entry. + */ +int +qemuRemoveSharedDisk(virHashTablePtr sharedDisks, + const char *disk_path) +{ + qemuSharedDiskEntryPtr entry = NULL; + + if (!(entry = virHashLookup(sharedDisks, disk_path))) + return -1; + + if (entry->ref != 1) { + entry->ref--; + } else { + if (virHashRemoveEntry(sharedDisks, disk_path)< 0) + return -1; + } + + return 0; +} diff --git a/src/qemu/qemu_conf.h b/src/qemu/qemu_conf.h index d0d25ce..df901f6 100644 --- a/src/qemu/qemu_conf.h +++ b/src/qemu/qemu_conf.h @@ -147,6 +147,8 @@ struct _virQEMUDriver { /* The devices which is are not in use by the host or any guest. */ pciDeviceList *inactivePciHostdevs;
+ virHashTablePtr sharedDisks; + virBitmapPtr reservedRemotePorts;
virSysinfoDefPtr hostsysinfo; @@ -193,6 +195,13 @@ struct qemuDomainDiskInfo { int io_status; };
+typedef struct qemuSharedDiskEntry qemuSharedDiskEntry; +typedef qemuSharedDiskEntry *qemuSharedDiskEntryPtr; +struct qemuSharedDiskEntry { + size_t ref; /* ref count of the shared disk */ + int orig_cdbfilter; /* Original disk's cdbfilter setting */ +}; + typedef virDomainObjPtr (*qemuDriverCloseCallback)(virQEMUDriverPtr driver, virDomainObjPtr vm, virConnectPtr conn); @@ -211,4 +220,13 @@ qemuDriverCloseCallback qemuDriverCloseCallbackGet(virQEMUDriverPtr driver, void qemuDriverCloseCallbackRunAll(virQEMUDriverPtr driver, virConnectPtr conn);
+int qemuAddSharedDisk(virHashTablePtr sharedDisks, + const char *disk_path, + int orig_cdbfilter) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); + +int qemuRemoveSharedDisk(virHashTablePtr sharedDisks, + const char *disk_path) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); + #endif /* __QEMUD_CONF_H */ diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index e099c5c..fdde74d 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -810,6 +810,9 @@ qemuStartup(bool privileged, if ((qemu_driver->inactivePciHostdevs = pciDeviceListNew()) == NULL) goto error;
+ if (!(qemu_driver->sharedDisks = virHashCreate(30, NULL))) + goto error; + if (privileged) { if (chown(qemu_driver->libDir, qemu_driver->user, qemu_driver->group)< 0) { virReportSystemError(errno, @@ -1064,6 +1067,7 @@ qemuShutdown(void) { pciDeviceListFree(qemu_driver->activePciHostdevs); pciDeviceListFree(qemu_driver->inactivePciHostdevs); usbDeviceListFree(qemu_driver->activeUsbHostdevs); + virHashFree(qemu_driver->sharedDisks); virCapabilitiesFree(qemu_driver->caps); qemuCapsCacheFree(qemu_driver->capsCache);
@@ -6035,6 +6039,12 @@ qemuDomainAttachDeviceDiskLive(virConnectPtr conn, VIR_WARN("Failed to teardown cgroup for disk path %s", NULLSTR(disk->src)); } + + if (ret == 0&& disk->shared) { + if (qemuAddSharedDisk(driver->sharedDisks, disk->src, 0)< 0) + VIR_WARN("Failed to add disk '%s' to shared disk table", + disk->src); + } end: if (cgroup) virCgroupFree(&cgroup); @@ -6149,6 +6159,13 @@ qemuDomainDetachDeviceDiskLive(virQEMUDriverPtr driver, virDomainDiskDeviceTypeToString(disk->type)); break; } + + if (ret == 0&& disk->shared) { + if (qemuRemoveSharedDisk(driver->sharedDisks, disk->src)< 0) + VIR_WARN("Failed to remove disk '%s' from shared disk table", + disk->src); + } + return ret; }
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index ab04599..89152b8 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -3706,8 +3706,15 @@ int qemuProcessStart(virConnectPtr conn,
/* in case a certain disk is desirous of CAP_SYS_RAWIO, add this */ for (i = 0; i< vm->def->ndisks; i++) { - if (vm->def->disks[i]->rawio == 1) + virDomainDiskDefPtr disk = vm->def->disks[i]; + + if (disk->rawio == 1) virCommandAllowCap(cmd, CAP_SYS_RAWIO); + + if (disk->shared) { + if (qemuAddSharedDisk(driver->sharedDisks, disk->src, 0)< 0) + goto cleanup; + } }
virCommandSetPreExecHook(cmd, qemuProcessHook,&hookData); @@ -4104,6 +4111,14 @@ void qemuProcessStop(virQEMUDriverPtr driver, flags& VIR_QEMU_PROCESS_STOP_MIGRATED); virSecurityManagerReleaseLabel(driver->securityManager, vm->def);
+ for (i = 0; i< vm->def->ndisks; i++) { + virDomainDiskDefPtr disk = vm->def->disks[i]; + + if (disk->shared) { + ignore_value(qemuRemoveSharedDisk(driver->sharedDisks, disk->src)); + } + } + /* Clear out dynamically assigned labels */ for (i = 0; i< vm->def->nseclabels; i++) { if (vm->def->seclabels[i]->type == VIR_DOMAIN_SECLABEL_DYNAMIC) {
With the diff below squashed in: (the dataFree was missed when changing the hash value from only ref count to a struct).
Ah, sorry, the correct diff is:
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On Tue, Dec 11, 2012 at 09:37:18PM +0800, Osier Yang wrote:
This introduces a hash table for qemu driver, to store the shared disk's info as (@disk_path, {@ref_count, @orig_cdbfilter}). @ref_count is the number of domains which shares the disk. @orig_cdbfilter is the original cdbfilter setting of the shared disk, it will be used to restore the the disk's cdbfilter setting to original value by later patches.
* src/qemu/qemu_conf.h: (Add member 'sharedDisks' of type virHashTablePtr; New struct qemuSharedDiskEntry; Declare helpers qemuAddSharedDisk, qemuRemoveSharedDisk) * src/qemu/qemu_conf.c (Implement the two helpers) * src/qemu/qemu_process.c (Update 'sharedDisks' when domain starting and shutdown) * src/qemu/qemu_driver.c (Update 'sharedDisks' when attaching or detaching disk).
0 is passed for orig_cdbfilter temporarily, later patches will update it. --- src/qemu/qemu_conf.c | 48 +++++++++++++++++++++++++++++++++++++++++++++++ src/qemu/qemu_conf.h | 18 +++++++++++++++++ src/qemu/qemu_driver.c | 17 ++++++++++++++++ src/qemu/qemu_process.c | 17 +++++++++++++++- 4 files changed, 99 insertions(+), 1 deletions(-)
diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c index 8d380a1..2b21186 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -556,3 +556,51 @@ qemuDriverCloseCallbackRunAll(virQEMUDriverPtr driver,
virHashForEach(driver->closeCallbacks, qemuDriverCloseCallbackRun, &data); } + +/* Increase ref count if the entry already exists, otherwise + * add a new entry. + */ +int +qemuAddSharedDisk(virHashTablePtr sharedDisks, + const char *disk_path, + int orig_cdbfilter) +{ + qemuSharedDiskEntryPtr entry = NULL; + + if ((entry = virHashLookup(sharedDisks, disk_path))) { + entry->ref++; + } else { + if (VIR_ALLOC(entry) < 0) + return -1; + + entry->ref = 1; + entry->orig_cdbfilter = orig_cdbfilter; + + if (virHashAddEntry(sharedDisks, disk_path, entry)) + return -1;
Leaking 'entry' in failure path.
@@ -6035,6 +6039,12 @@ qemuDomainAttachDeviceDiskLive(virConnectPtr conn, VIR_WARN("Failed to teardown cgroup for disk path %s", NULLSTR(disk->src)); } + + if (ret == 0 && disk->shared) { + if (qemuAddSharedDisk(driver->sharedDisks, disk->src, 0) < 0) + VIR_WARN("Failed to add disk '%s' to shared disk table", + disk->src); + }
You are not allowed to reference 'disk->src' unless you've validate disk->type is FILE or BLOCK. We only need to track this for TYPE_BLOCK in any case
end: if (cgroup) virCgroupFree(&cgroup); @@ -6149,6 +6159,13 @@ qemuDomainDetachDeviceDiskLive(virQEMUDriverPtr driver, virDomainDiskDeviceTypeToString(disk->type)); break; } + + if (ret == 0 && disk->shared) { + if (qemuRemoveSharedDisk(driver->sharedDisks, disk->src) < 0) + VIR_WARN("Failed to remove disk '%s' from shared disk table", + disk->src); + }
Same crash problem here
+ return ret; }
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index ab04599..89152b8 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -3706,8 +3706,15 @@ int qemuProcessStart(virConnectPtr conn,
/* in case a certain disk is desirous of CAP_SYS_RAWIO, add this */ for (i = 0; i < vm->def->ndisks; i++) { - if (vm->def->disks[i]->rawio == 1) + virDomainDiskDefPtr disk = vm->def->disks[i]; + + if (disk->rawio == 1) virCommandAllowCap(cmd, CAP_SYS_RAWIO); + + if (disk->shared) { + if (qemuAddSharedDisk(driver->sharedDisks, disk->src, 0) < 0) + goto cleanup; + }
And here
}
virCommandSetPreExecHook(cmd, qemuProcessHook, &hookData); @@ -4104,6 +4111,14 @@ void qemuProcessStop(virQEMUDriverPtr driver, flags & VIR_QEMU_PROCESS_STOP_MIGRATED); virSecurityManagerReleaseLabel(driver->securityManager, vm->def);
+ for (i = 0; i < vm->def->ndisks; i++) { + virDomainDiskDefPtr disk = vm->def->disks[i]; + + if (disk->shared) { + ignore_value(qemuRemoveSharedDisk(driver->sharedDisks, disk->src)); + } + }
And here Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On 2012年12月13日 01:09, Daniel P. Berrange wrote:
On Tue, Dec 11, 2012 at 09:37:18PM +0800, Osier Yang wrote:
This introduces a hash table for qemu driver, to store the shared disk's info as (@disk_path, {@ref_count, @orig_cdbfilter}). @ref_count is the number of domains which shares the disk. @orig_cdbfilter is the original cdbfilter setting of the shared disk, it will be used to restore the the disk's cdbfilter setting to original value by later patches.
* src/qemu/qemu_conf.h: (Add member 'sharedDisks' of type virHashTablePtr; New struct qemuSharedDiskEntry; Declare helpers qemuAddSharedDisk, qemuRemoveSharedDisk) * src/qemu/qemu_conf.c (Implement the two helpers) * src/qemu/qemu_process.c (Update 'sharedDisks' when domain starting and shutdown) * src/qemu/qemu_driver.c (Update 'sharedDisks' when attaching or detaching disk).
0 is passed for orig_cdbfilter temporarily, later patches will update it. --- src/qemu/qemu_conf.c | 48 +++++++++++++++++++++++++++++++++++++++++++++++ src/qemu/qemu_conf.h | 18 +++++++++++++++++ src/qemu/qemu_driver.c | 17 ++++++++++++++++ src/qemu/qemu_process.c | 17 +++++++++++++++- 4 files changed, 99 insertions(+), 1 deletions(-)
diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c index 8d380a1..2b21186 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -556,3 +556,51 @@ qemuDriverCloseCallbackRunAll(virQEMUDriverPtr driver,
virHashForEach(driver->closeCallbacks, qemuDriverCloseCallbackRun,&data); } + +/* Increase ref count if the entry already exists, otherwise + * add a new entry. + */ +int +qemuAddSharedDisk(virHashTablePtr sharedDisks, + const char *disk_path, + int orig_cdbfilter) +{ + qemuSharedDiskEntryPtr entry = NULL; + + if ((entry = virHashLookup(sharedDisks, disk_path))) { + entry->ref++; + } else { + if (VIR_ALLOC(entry)< 0) + return -1; + + entry->ref = 1; + entry->orig_cdbfilter = orig_cdbfilter; + + if (virHashAddEntry(sharedDisks, disk_path, entry)) + return -1;
Leaking 'entry' in failure path.
Okay, it's missed when changing the hash value from only 'ref' count to a struct containing the original cdbfilter value.
@@ -6035,6 +6039,12 @@ qemuDomainAttachDeviceDiskLive(virConnectPtr conn, VIR_WARN("Failed to teardown cgroup for disk path %s", NULLSTR(disk->src)); } + + if (ret == 0&& disk->shared) { + if (qemuAddSharedDisk(driver->sharedDisks, disk->src, 0)< 0) + VIR_WARN("Failed to add disk '%s' to shared disk table", + disk->src); + }
You are not allowed to reference 'disk->src' unless you've validate disk->type is FILE or BLOCK.
We only need to track this for TYPE_BLOCK in any case
Oh, right, okay.
end: if (cgroup) virCgroupFree(&cgroup); @@ -6149,6 +6159,13 @@ qemuDomainDetachDeviceDiskLive(virQEMUDriverPtr driver, virDomainDiskDeviceTypeToString(disk->type)); break; } + + if (ret == 0&& disk->shared) { + if (qemuRemoveSharedDisk(driver->sharedDisks, disk->src)< 0) + VIR_WARN("Failed to remove disk '%s' from shared disk table", + disk->src); + }
Same crash problem here
+ return ret; }
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index ab04599..89152b8 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -3706,8 +3706,15 @@ int qemuProcessStart(virConnectPtr conn,
/* in case a certain disk is desirous of CAP_SYS_RAWIO, add this */ for (i = 0; i< vm->def->ndisks; i++) { - if (vm->def->disks[i]->rawio == 1) + virDomainDiskDefPtr disk = vm->def->disks[i]; + + if (disk->rawio == 1) virCommandAllowCap(cmd, CAP_SYS_RAWIO); + + if (disk->shared) { + if (qemuAddSharedDisk(driver->sharedDisks, disk->src, 0)< 0) + goto cleanup; + }
And here
}
virCommandSetPreExecHook(cmd, qemuProcessHook,&hookData); @@ -4104,6 +4111,14 @@ void qemuProcessStop(virQEMUDriverPtr driver, flags& VIR_QEMU_PROCESS_STOP_MIGRATED); virSecurityManagerReleaseLabel(driver->securityManager, vm->def);
+ for (i = 0; i< vm->def->ndisks; i++) { + virDomainDiskDefPtr disk = vm->def->disks[i]; + + if (disk->shared) { + ignore_value(qemuRemoveSharedDisk(driver->sharedDisks, disk->src)); + } + }
And here
Daniel
On Tue, Dec 11, 2012 at 09:37:18PM +0800, Osier Yang wrote:
This introduces a hash table for qemu driver, to store the shared disk's info as (@disk_path, {@ref_count, @orig_cdbfilter}). @ref_count is the number of domains which shares the disk. @orig_cdbfilter is the original cdbfilter setting of the shared disk, it will be used to restore the the disk's cdbfilter setting to original value by later patches.
* src/qemu/qemu_conf.h: (Add member 'sharedDisks' of type virHashTablePtr; New struct qemuSharedDiskEntry; Declare helpers qemuAddSharedDisk, qemuRemoveSharedDisk) * src/qemu/qemu_conf.c (Implement the two helpers) * src/qemu/qemu_process.c (Update 'sharedDisks' when domain starting and shutdown) * src/qemu/qemu_driver.c (Update 'sharedDisks' when attaching or detaching disk).
0 is passed for orig_cdbfilter temporarily, later patches will update it. --- src/qemu/qemu_conf.c | 48 +++++++++++++++++++++++++++++++++++++++++++++++ src/qemu/qemu_conf.h | 18 +++++++++++++++++ src/qemu/qemu_driver.c | 17 ++++++++++++++++ src/qemu/qemu_process.c | 17 +++++++++++++++- 4 files changed, 99 insertions(+), 1 deletions(-)
diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c index 8d380a1..2b21186 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -556,3 +556,51 @@ qemuDriverCloseCallbackRunAll(virQEMUDriverPtr driver,
virHashForEach(driver->closeCallbacks, qemuDriverCloseCallbackRun, &data); } + +/* Increase ref count if the entry already exists, otherwise + * add a new entry. + */ +int +qemuAddSharedDisk(virHashTablePtr sharedDisks, + const char *disk_path, + int orig_cdbfilter) +{ + qemuSharedDiskEntryPtr entry = NULL; + + if ((entry = virHashLookup(sharedDisks, disk_path))) { + entry->ref++; + } else { + if (VIR_ALLOC(entry) < 0) + return -1; + + entry->ref = 1; + entry->orig_cdbfilter = orig_cdbfilter; + + if (virHashAddEntry(sharedDisks, disk_path, entry)) + return -1; + } + + return 0; +} + +/* Decrease the ref count if the entry already exists, otherwise + * remove the entry. + */ +int +qemuRemoveSharedDisk(virHashTablePtr sharedDisks, + const char *disk_path) +{ + qemuSharedDiskEntryPtr entry = NULL; + + if (!(entry = virHashLookup(sharedDisks, disk_path))) + return -1; + + if (entry->ref != 1) { + entry->ref--; + } else { + if (virHashRemoveEntry(sharedDisks, disk_path) < 0) + return -1; + } + + return 0; +} diff --git a/src/qemu/qemu_conf.h b/src/qemu/qemu_conf.h index d0d25ce..df901f6 100644 --- a/src/qemu/qemu_conf.h +++ b/src/qemu/qemu_conf.h @@ -147,6 +147,8 @@ struct _virQEMUDriver { /* The devices which is are not in use by the host or any guest. */ pciDeviceList *inactivePciHostdevs;
+ virHashTablePtr sharedDisks; + virBitmapPtr reservedRemotePorts;
virSysinfoDefPtr hostsysinfo; @@ -193,6 +195,13 @@ struct qemuDomainDiskInfo { int io_status; };
+typedef struct qemuSharedDiskEntry qemuSharedDiskEntry; +typedef qemuSharedDiskEntry *qemuSharedDiskEntryPtr; +struct qemuSharedDiskEntry { + size_t ref; /* ref count of the shared disk */ + int orig_cdbfilter; /* Original disk's cdbfilter setting */ +};
Storing the original value here doesn't work in context of libvirtd restarts, because you've no way to re-populate it Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On 2012年12月13日 01:16, Daniel P. Berrange wrote:
On Tue, Dec 11, 2012 at 09:37:18PM +0800, Osier Yang wrote:
This introduces a hash table for qemu driver, to store the shared disk's info as (@disk_path, {@ref_count, @orig_cdbfilter}). @ref_count is the number of domains which shares the disk. @orig_cdbfilter is the original cdbfilter setting of the shared disk, it will be used to restore the the disk's cdbfilter setting to original value by later patches.
* src/qemu/qemu_conf.h: (Add member 'sharedDisks' of type virHashTablePtr; New struct qemuSharedDiskEntry; Declare helpers qemuAddSharedDisk, qemuRemoveSharedDisk) * src/qemu/qemu_conf.c (Implement the two helpers) * src/qemu/qemu_process.c (Update 'sharedDisks' when domain starting and shutdown) * src/qemu/qemu_driver.c (Update 'sharedDisks' when attaching or detaching disk).
0 is passed for orig_cdbfilter temporarily, later patches will update it. --- src/qemu/qemu_conf.c | 48 +++++++++++++++++++++++++++++++++++++++++++++++ src/qemu/qemu_conf.h | 18 +++++++++++++++++ src/qemu/qemu_driver.c | 17 ++++++++++++++++ src/qemu/qemu_process.c | 17 +++++++++++++++- 4 files changed, 99 insertions(+), 1 deletions(-)
diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c index 8d380a1..2b21186 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -556,3 +556,51 @@ qemuDriverCloseCallbackRunAll(virQEMUDriverPtr driver,
virHashForEach(driver->closeCallbacks, qemuDriverCloseCallbackRun,&data); } + +/* Increase ref count if the entry already exists, otherwise + * add a new entry. + */ +int +qemuAddSharedDisk(virHashTablePtr sharedDisks, + const char *disk_path, + int orig_cdbfilter) +{ + qemuSharedDiskEntryPtr entry = NULL; + + if ((entry = virHashLookup(sharedDisks, disk_path))) { + entry->ref++; + } else { + if (VIR_ALLOC(entry)< 0) + return -1; + + entry->ref = 1; + entry->orig_cdbfilter = orig_cdbfilter; + + if (virHashAddEntry(sharedDisks, disk_path, entry)) + return -1; + } + + return 0; +} + +/* Decrease the ref count if the entry already exists, otherwise + * remove the entry. + */ +int +qemuRemoveSharedDisk(virHashTablePtr sharedDisks, + const char *disk_path) +{ + qemuSharedDiskEntryPtr entry = NULL; + + if (!(entry = virHashLookup(sharedDisks, disk_path))) + return -1; + + if (entry->ref != 1) { + entry->ref--; + } else { + if (virHashRemoveEntry(sharedDisks, disk_path)< 0) + return -1; + } + + return 0; +} diff --git a/src/qemu/qemu_conf.h b/src/qemu/qemu_conf.h index d0d25ce..df901f6 100644 --- a/src/qemu/qemu_conf.h +++ b/src/qemu/qemu_conf.h @@ -147,6 +147,8 @@ struct _virQEMUDriver { /* The devices which is are not in use by the host or any guest. */ pciDeviceList *inactivePciHostdevs;
+ virHashTablePtr sharedDisks; + virBitmapPtr reservedRemotePorts;
virSysinfoDefPtr hostsysinfo; @@ -193,6 +195,13 @@ struct qemuDomainDiskInfo { int io_status; };
+typedef struct qemuSharedDiskEntry qemuSharedDiskEntry; +typedef qemuSharedDiskEntry *qemuSharedDiskEntryPtr; +struct qemuSharedDiskEntry { + size_t ref; /* ref count of the shared disk */ + int orig_cdbfilter; /* Original disk's cdbfilter setting */ +};
Storing the original value here doesn't work in context of libvirtd restarts, because you've no way to re-populate it
Yeah, I thought as long as it works without libvirtd restarting, it should be better.
On Tue, Dec 11, 2012 at 21:37:18 +0800, Osier Yang wrote:
This introduces a hash table for qemu driver, to store the shared disk's info as (@disk_path, {@ref_count, @orig_cdbfilter}). @ref_count is the number of domains which shares the disk. @orig_cdbfilter is the original cdbfilter setting of the shared disk, it will be used to restore the the disk's cdbfilter setting to original value by later patches.
* src/qemu/qemu_conf.h: (Add member 'sharedDisks' of type virHashTablePtr; New struct qemuSharedDiskEntry; Declare helpers qemuAddSharedDisk, qemuRemoveSharedDisk) * src/qemu/qemu_conf.c (Implement the two helpers) * src/qemu/qemu_process.c (Update 'sharedDisks' when domain starting and shutdown) * src/qemu/qemu_driver.c (Update 'sharedDisks' when attaching or detaching disk).
0 is passed for orig_cdbfilter temporarily, later patches will update it. --- src/qemu/qemu_conf.c | 48 +++++++++++++++++++++++++++++++++++++++++++++++ src/qemu/qemu_conf.h | 18 +++++++++++++++++ src/qemu/qemu_driver.c | 17 ++++++++++++++++ src/qemu/qemu_process.c | 17 +++++++++++++++- 4 files changed, 99 insertions(+), 1 deletions(-)
diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c index 8d380a1..2b21186 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -556,3 +556,51 @@ qemuDriverCloseCallbackRunAll(virQEMUDriverPtr driver,
virHashForEach(driver->closeCallbacks, qemuDriverCloseCallbackRun, &data); } + +/* Increase ref count if the entry already exists, otherwise + * add a new entry. + */ +int +qemuAddSharedDisk(virHashTablePtr sharedDisks, + const char *disk_path, + int orig_cdbfilter) +{ + qemuSharedDiskEntryPtr entry = NULL; + + if ((entry = virHashLookup(sharedDisks, disk_path))) { + entry->ref++; + } else { + if (VIR_ALLOC(entry) < 0) + return -1; + + entry->ref = 1; + entry->orig_cdbfilter = orig_cdbfilter; + + if (virHashAddEntry(sharedDisks, disk_path, entry)) + return -1; + }
In addition to Daniel's comments, I think this hash table should contain device number rather than disk path since /dev is full of symlinks. One domain may be configured to use /dev/sdg while another one would use /dev/disk/by-id/..., another one /dev/disk/by-uuid/... and they can all lead to the same block device. Thus you need to canonicalize disk_path first, e.g., into major:minor and store that in the hash table. Jirka
On 2012年12月13日 19:05, Jiri Denemark wrote:
On Tue, Dec 11, 2012 at 21:37:18 +0800, Osier Yang wrote:
This introduces a hash table for qemu driver, to store the shared disk's info as (@disk_path, {@ref_count, @orig_cdbfilter}). @ref_count is the number of domains which shares the disk. @orig_cdbfilter is the original cdbfilter setting of the shared disk, it will be used to restore the the disk's cdbfilter setting to original value by later patches.
* src/qemu/qemu_conf.h: (Add member 'sharedDisks' of type virHashTablePtr; New struct qemuSharedDiskEntry; Declare helpers qemuAddSharedDisk, qemuRemoveSharedDisk) * src/qemu/qemu_conf.c (Implement the two helpers) * src/qemu/qemu_process.c (Update 'sharedDisks' when domain starting and shutdown) * src/qemu/qemu_driver.c (Update 'sharedDisks' when attaching or detaching disk).
0 is passed for orig_cdbfilter temporarily, later patches will update it. --- src/qemu/qemu_conf.c | 48 +++++++++++++++++++++++++++++++++++++++++++++++ src/qemu/qemu_conf.h | 18 +++++++++++++++++ src/qemu/qemu_driver.c | 17 ++++++++++++++++ src/qemu/qemu_process.c | 17 +++++++++++++++- 4 files changed, 99 insertions(+), 1 deletions(-)
diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c index 8d380a1..2b21186 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -556,3 +556,51 @@ qemuDriverCloseCallbackRunAll(virQEMUDriverPtr driver,
virHashForEach(driver->closeCallbacks, qemuDriverCloseCallbackRun,&data); } + +/* Increase ref count if the entry already exists, otherwise + * add a new entry. + */ +int +qemuAddSharedDisk(virHashTablePtr sharedDisks, + const char *disk_path, + int orig_cdbfilter) +{ + qemuSharedDiskEntryPtr entry = NULL; + + if ((entry = virHashLookup(sharedDisks, disk_path))) { + entry->ref++; + } else { + if (VIR_ALLOC(entry)< 0) + return -1; + + entry->ref = 1; + entry->orig_cdbfilter = orig_cdbfilter; + + if (virHashAddEntry(sharedDisks, disk_path, entry)) + return -1; + }
In addition to Daniel's comments, I think this hash table should contain device number rather than disk path since /dev is full of symlinks. One domain may be configured to use /dev/sdg while another one would use /dev/disk/by-id/..., another one /dev/disk/by-uuid/... and they can all lead to the same block device. Thus you need to canonicalize disk_path first, e.g., into major:minor and store that in the hash table.
Hum, agreed, this is really good catch which saves us from the potential bug. Thanks. Regards, Osier
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 c81af8a..a42f0d3 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 14344e2..c8cdfd7 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -971,24 +971,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
Setting "cdbfilter" to "no" to enable the unprivleged SG_IO, I.e. Disable the kernel CDB filtering. And "yes" to enable it. "yes" is the kernel default behaviour. Later patch will do the actual setting. --- src/conf/domain_conf.c | 58 +++++++++++++++----- src/conf/domain_conf.h | 10 ++++ ...ml2argv-disk-scsi-lun-passthrough-cdbfilter.xml | 32 +++++++++++ tests/qemuxml2xmltest.c | 1 + 4 files changed, 88 insertions(+), 13 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-scsi-lun-passthrough-cdbfilter.xml diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 6aa5f79..1edba69 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -242,6 +242,10 @@ VIR_ENUM_IMPL(virDomainDiskIo, VIR_DOMAIN_DISK_IO_LAST, "default", "native", "threads") +VIR_ENUM_IMPL(virDomainDiskCDBFilter, VIR_DOMAIN_DISK_CDB_FILTER_LAST, + "default", + "yes", + "no") VIR_ENUM_IMPL(virDomainIoEventFd, VIR_DOMAIN_IO_EVENT_FD_LAST, "default", "on", @@ -3530,6 +3534,7 @@ virDomainDiskDefParseXML(virCapsPtr caps, char *device = NULL; char *snapshot = NULL; char *rawio = NULL; + char *cdbfilter = NULL; char *driverName = NULL; char *driverType = NULL; char *source = NULL; @@ -3594,6 +3599,7 @@ virDomainDiskDefParseXML(virCapsPtr caps, snapshot = virXMLPropString(node, "snapshot"); rawio = virXMLPropString(node, "rawio"); + cdbfilter = virXMLPropString(node, "cdbfilter"); cur = node->children; while (cur != NULL) { @@ -4044,24 +4050,46 @@ virDomainDiskDefParseXML(virCapsPtr caps, def->snapshot = VIR_DOMAIN_SNAPSHOT_LOCATION_NONE; } + if (rawio && cdbfilter) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("rawio and cdbfilter are exclusive")); + goto error; + } + + if ((rawio || cdbfilter) && + (def->device != VIR_DOMAIN_DISK_DEVICE_LUN)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("rawio or cdbfilter can be used only with " + "device='lun'")); + goto error; + } + if (rawio) { def->rawio_specified = true; - if (def->device == VIR_DOMAIN_DISK_DEVICE_LUN) { - if (STREQ(rawio, "yes")) { - def->rawio = 1; - } else if (STREQ(rawio, "no")) { - def->rawio = 0; - } else { - virReportError(VIR_ERR_XML_ERROR, - _("unknown disk rawio setting '%s'"), - rawio); - goto error; - } + if (STREQ(rawio, "yes")) { + def->rawio = 1; + } else if (STREQ(rawio, "no")) { + def->rawio = 0; } else { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("rawio can be used only with device='lun'")); + virReportError(VIR_ERR_XML_ERROR, + _("unknown disk rawio setting '%s'"), + rawio); + goto error; + } + } + + if (cdbfilter) { + int cdbfilter_val = 0; + + if ((cdbfilter_val = + virDomainDiskCDBFilterTypeFromString(cdbfilter)) < 0) { + virReportError(VIR_ERR_XML_ERROR, + _("unknown disk cdbfilter setting '%s'"), + cdbfilter); goto error; } + + def->cdbfilter = cdbfilter_val; } if (bus) { @@ -4302,6 +4330,7 @@ cleanup: VIR_FREE(type); VIR_FREE(snapshot); VIR_FREE(rawio); + VIR_FREE(cdbfilter); VIR_FREE(target); VIR_FREE(source); VIR_FREE(tray); @@ -11982,6 +12011,9 @@ virDomainDiskDefFormat(virBufferPtr buf, virBufferAddLit(buf, " rawio='no'"); } } + if (def->cdbfilter) + virBufferAsprintf(buf, " cdbfilter='%s'", + virDomainDiskCDBFilterTypeToString(def->cdbfilter)); if (def->snapshot && !(def->snapshot == VIR_DOMAIN_SNAPSHOT_LOCATION_NONE && def->readonly)) virBufferAsprintf(buf, " snapshot='%s'", diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 7ad5377..9586f75 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -507,6 +507,14 @@ enum virDomainDiskIo { VIR_DOMAIN_DISK_IO_LAST }; +enum virDomainDiskCDBFilter { + VIR_DOMAIN_DISK_CDB_FILTER_DEFAULT = 0, + VIR_DOMAIN_DISK_CDB_FILTER_YES, /* reflects to 0 of sysfs unpriv_sgio */ + VIR_DOMAIN_DISK_CDB_FILTER_NO, /* reflects to 1 of sysfs unpriv_sgio */ + + VIR_DOMAIN_DISK_CDB_FILTER_LAST +}; + enum virDomainIoEventFd { VIR_DOMAIN_IO_EVENT_FD_DEFAULT = 0, VIR_DOMAIN_IO_EVENT_FD_ON, @@ -620,6 +628,7 @@ struct _virDomainDiskDef { virStorageEncryptionPtr encryption; bool rawio_specified; int rawio; /* no = 0, yes = 1 */ + int cdbfilter; size_t nseclabels; virSecurityDeviceLabelDefPtr *seclabels; @@ -2211,6 +2220,7 @@ VIR_ENUM_DECL(virDomainDiskErrorPolicy) VIR_ENUM_DECL(virDomainDiskProtocol) VIR_ENUM_DECL(virDomainDiskProtocolTransport) VIR_ENUM_DECL(virDomainDiskIo) +VIR_ENUM_DECL(virDomainDiskCDBFilter) VIR_ENUM_DECL(virDomainDiskSecretType) VIR_ENUM_DECL(virDomainDiskTray) VIR_ENUM_DECL(virDomainIoEventFd) diff --git a/tests/qemuxml2argvdata/qemuxml2argv-disk-scsi-lun-passthrough-cdbfilter.xml b/tests/qemuxml2argvdata/qemuxml2argv-disk-scsi-lun-passthrough-cdbfilter.xml new file mode 100644 index 0000000..731c9bd --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-scsi-lun-passthrough-cdbfilter.xml @@ -0,0 +1,32 @@ +<domain type='qemu'> + <name>QEMUGuest1</name> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> + <memory unit='KiB'>219136</memory> + <currentMemory unit='KiB'>219136</currentMemory> + <vcpu placement='static'>1</vcpu> + <os> + <type arch='i686' machine='pc'>hvm</type> + <boot dev='hd'/> + </os> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/bin/qemu</emulator> + <disk type='block' device='lun' cdbfilter='yes'> + <source dev='/dev/HostVG/QEMUGuest1'/> + <target dev='hda' bus='scsi'/> + <address type='drive' controller='0' bus='0' target='0' unit='0'/> + </disk> + <disk type='block' device='lun' cdbfilter='no'> + <source dev='/dev/HostVG/QEMUGuest2'/> + <target dev='hda' bus='scsi'/> + <address type='drive' controller='0' bus='0' target='1' unit='1'/> + </disk> + <controller type='scsi' index='0' model='virtio-scsi'/> + <controller type='scsi' index='1' model='lsilogic'/> + <controller type='usb' index='0'/> + <memballoon model='virtio'/> + </devices> +</domain> diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c index c2d3dd3..ca2daff 100644 --- a/tests/qemuxml2xmltest.c +++ b/tests/qemuxml2xmltest.c @@ -238,6 +238,7 @@ mymain(void) DO_TEST("seclabel-static"); DO_TEST("seclabel-none"); DO_TEST("numad-static-vcpu-no-numatune"); + DO_TEST("disk-scsi-lun-passthrough-cdbfilter"); DO_TEST("disk-scsi-disk-vpd"); -- 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 | 8 +++ 3 files changed, 157 insertions(+), 0 deletions(-) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 499ab3b..321d839 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1274,6 +1274,9 @@ virFileWaitForDevices; virFileWriteStr; virFindFileInPath; virFormatIntDecimal; +virGetDeviceMajor; +virGetDeviceMinor; +virGetDeviceUnprivSGIO; virGetGroupID; virGetGroupName; virGetHostname; @@ -1292,6 +1295,7 @@ virPipeReadUntilEOF; virScaleInteger; virSetBlocking; virSetCloseExec; +virSetDeviceUnprivSGIO; virSetInherit; virSetNonBlock; virSetUIDGID; diff --git a/src/util/util.c b/src/util/util.c index 3f244f5..f0f76da 100644 --- a/src/util/util.c +++ b/src/util/util.c @@ -3143,3 +3143,148 @@ virStrIsPrint(const char *str) 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 6d5dd03..e00681a 100644 --- a/src/util/util.h +++ b/src/util/util.h @@ -281,4 +281,12 @@ bool virIsDevMapperDevice(const char *dev_name) ATTRIBUTE_NONNULL(1); bool virValidateWWN(const char *wwn); bool virStrIsPrint(const char *str); + +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
Here lifecyle only means starting and shutdown. To record the original "unpriv_sgio" value for non-shared disk, this introduces "orig_cdbfilter" for disk def. On domain starting, the disk's "unpriv_sgio" is set with regards to the config in domain XML. And on domain shutdown, it's restored to the original value ("orig_cdbfilter"). Later patch will prevent restoring if other domain is still using the shared disk. --- src/conf/domain_conf.h | 1 + src/qemu/qemu_process.c | 34 +++++++++++++++++++++++++++++++++- 2 files changed, 34 insertions(+), 1 deletions(-) diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 9586f75..256bea2 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -629,6 +629,7 @@ struct _virDomainDiskDef { bool rawio_specified; int rawio; /* no = 0, yes = 1 */ int cdbfilter; + int orig_cdbfilter; size_t nseclabels; virSecurityDeviceLabelDefPtr *seclabels; diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 89152b8..859c5b0 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -3707,12 +3707,31 @@ int qemuProcessStart(virConnectPtr conn, /* in case a certain disk is desirous of CAP_SYS_RAWIO, add this */ for (i = 0; i < vm->def->ndisks; i++) { virDomainDiskDefPtr disk = vm->def->disks[i]; + int val; if (disk->rawio == 1) virCommandAllowCap(cmd, CAP_SYS_RAWIO); + /* Set sysfs unpriv_sgio if cdbfilter is specified */ + if (disk->cdbfilter) { + if (virGetDeviceUnprivSGIO(disk->src, &val) < 0) + goto cleanup; + + if (val == 0) + disk->orig_cdbfilter = VIR_DOMAIN_DISK_CDB_FILTER_YES; + else + disk->orig_cdbfilter = VIR_DOMAIN_DISK_CDB_FILTER_NO; + + if (virSetDeviceUnprivSGIO(disk->src, + (disk->cdbfilter == + VIR_DOMAIN_DISK_CDB_FILTER_NO) + ? 1 : 0) < 0) + goto cleanup; + } + if (disk->shared) { - if (qemuAddSharedDisk(driver->sharedDisks, disk->src, 0) < 0) + if (qemuAddSharedDisk(driver->sharedDisks, disk->src, + disk->orig_cdbfilter) < 0) goto cleanup; } } @@ -4113,6 +4132,19 @@ void qemuProcessStop(virQEMUDriverPtr driver, for (i = 0; i < vm->def->ndisks; i++) { virDomainDiskDefPtr disk = vm->def->disks[i]; + int val; + + /* Restore sysfs unpriv_sgio for the disk */ + if (disk->cdbfilter) { + if (disk->orig_cdbfilter == VIR_DOMAIN_DISK_CDB_FILTER_YES) + val = 0; + else + val = 1; + + if (virSetDeviceUnprivSGIO(disk->src, val) < 0) + VIR_WARN("Unable to restore unpriv_sgio for disk '%s'", + disk->src); + } if (disk->shared) { ignore_value(qemuRemoveSharedDisk(driver->sharedDisks, disk->src)); -- 1.7.7.6
On Tue, Dec 11, 2012 at 09:37:22PM +0800, Osier Yang wrote:
Here lifecyle only means starting and shutdown.
To record the original "unpriv_sgio" value for non-shared disk, this introduces "orig_cdbfilter" for disk def. On domain starting, the disk's "unpriv_sgio" is set with regards to the config in domain XML. And on domain shutdown, it's restored to the original value ("orig_cdbfilter"). Later patch will prevent restoring if other domain is still using the shared disk. --- src/conf/domain_conf.h | 1 + src/qemu/qemu_process.c | 34 +++++++++++++++++++++++++++++++++- 2 files changed, 34 insertions(+), 1 deletions(-)
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 9586f75..256bea2 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -629,6 +629,7 @@ struct _virDomainDiskDef { bool rawio_specified; int rawio; /* no = 0, yes = 1 */ int cdbfilter; + int orig_cdbfilter;
Huh, why did you go to the bother of saving the original value in the hash table, if you're saving it here too. THis just seems bogus to me, and won't survive a restart of libvirtd in any case. Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On 2012年12月13日 01:13, Daniel P. Berrange wrote:
On Tue, Dec 11, 2012 at 09:37:22PM +0800, Osier Yang wrote:
Here lifecyle only means starting and shutdown.
To record the original "unpriv_sgio" value for non-shared disk, this introduces "orig_cdbfilter" for disk def. On domain starting, the disk's "unpriv_sgio" is set with regards to the config in domain XML. And on domain shutdown, it's restored to the original value ("orig_cdbfilter"). Later patch will prevent restoring if other domain is still using the shared disk. --- src/conf/domain_conf.h | 1 + src/qemu/qemu_process.c | 34 +++++++++++++++++++++++++++++++++- 2 files changed, 34 insertions(+), 1 deletions(-)
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 9586f75..256bea2 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -629,6 +629,7 @@ struct _virDomainDiskDef { bool rawio_specified; int rawio; /* no = 0, yes = 1 */ int cdbfilter; + int orig_cdbfilter;
Huh, why did you go to the bother of saving the original value in the hash table, if you're saving it here too. THis just seems bogus to me, and won't survive a restart of libvirtd in any case.
I should explain more in the commit message. The orig_cdbfilter in qemu driver is for shared disks, and this is for non-shared disks. As the orig_cdbfilter can be changed when starting domains share disks. E.g. % virsh start one % virsh start two the orig_cdbfilter in domain two's disk def will be not the exact original value. Regards, Osier
This prevents restoring the unpriv_sgio if the disk is still being, shared by other active domain. Because we don't want to fall into the corruption situation. And if the disk is not shared, using diskdef->orig_cdbfilter to restore the original disk's unpriv_sgio state. If the disk is shared, but there is no other domain is sharing it, using orig_cdbfilter of sharedDisks' entry to restore it. --- src/qemu/qemu_conf.c | 12 ++++++++++++ src/qemu/qemu_conf.h | 4 ++++ src/qemu/qemu_process.c | 25 ++++++++++++++++++------- 3 files changed, 34 insertions(+), 7 deletions(-) diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c index 2b21186..1bd89f7 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -604,3 +604,15 @@ qemuRemoveSharedDisk(virHashTablePtr sharedDisks, return 0; } + +bool +qemuSharedDiskIsLast(virHashTablePtr sharedDisks, + const char *disk_path) +{ + qemuSharedDiskEntryPtr entry = NULL; + + if (!(entry = virHashLookup(sharedDisks, disk_path))) + return false; + + return entry->ref == 1; +} diff --git a/src/qemu/qemu_conf.h b/src/qemu/qemu_conf.h index df901f6..73bbea8 100644 --- a/src/qemu/qemu_conf.h +++ b/src/qemu/qemu_conf.h @@ -229,4 +229,8 @@ int qemuRemoveSharedDisk(virHashTablePtr sharedDisks, const char *disk_path) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); +bool qemuSharedDiskIsLast(virHashTablePtr sharedDisks, + const char *disk_path) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); + #endif /* __QEMUD_CONF_H */ diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 859c5b0..f39b83f 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -4133,17 +4133,28 @@ void qemuProcessStop(virQEMUDriverPtr driver, for (i = 0; i < vm->def->ndisks; i++) { virDomainDiskDefPtr disk = vm->def->disks[i]; int val; + qemuSharedDiskEntryPtr entry = NULL; /* Restore sysfs unpriv_sgio for the disk */ if (disk->cdbfilter) { - if (disk->orig_cdbfilter == VIR_DOMAIN_DISK_CDB_FILTER_YES) - val = 0; - else - val = 1; + /* Do not restore unpriv_sgio of the disk if it's still + * being shared by some other domain. + */ + if (!disk->shared || + qemuSharedDiskIsLast(driver->sharedDisks, disk->src)) { + if (!disk->shared) { + val = disk->orig_cdbfilter; + } else { + entry = virHashLookup(driver->sharedDisks, disk->src); + val = entry->orig_cdbfilter; + } - if (virSetDeviceUnprivSGIO(disk->src, val) < 0) - VIR_WARN("Unable to restore unpriv_sgio for disk '%s'", - disk->src); + if (virSetDeviceUnprivSGIO(disk->src, + (val == VIR_DOMAIN_DISK_CDB_FILTER_YES) ? + 0 : 1) < 0) + VIR_WARN("Unable to restore unpriv_sgio for disk '%s'", + disk->src); + } } if (disk->shared) { -- 1.7.7.6
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 | 44 ++++++++++++++++++++++++++++++++++++++++++++ src/qemu/qemu_process.h | 3 +++ 2 files changed, 47 insertions(+), 0 deletions(-) diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index f39b83f..ba777e2 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -3380,6 +3380,42 @@ 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). + * + * Returns 0 if no conflicts, otherwise returns -1. + */ +int +qemuCheckSharedDisk(virHashTablePtr sharedDisks, + virDomainDiskDefPtr disk) +{ + int val; + qemuSharedDiskEntryPtr entry = NULL; + + /* It can't be conflict if no other domain is + * is sharing it. + */ + if (!(entry = virHashLookup(sharedDisks, disk->src))) + return 0; + + if (entry->ref == 1) + return 0; + + if (virGetDeviceUnprivSGIO(disk->src, &val) < 0) + return -1; + + if ((val == 0 && + disk->orig_cdbfilter == VIR_DOMAIN_DISK_CDB_FILTER_YES) || + (val == 1 && + disk->orig_cdbfilter == VIR_DOMAIN_DISK_CDB_FILTER_NO)) + return 0; + + virReportError(VIR_ERR_INTERNAL_ERROR, + _("cdbfilter of shared disk '%s' conflicts with other " + "active domains"), disk->src); + return -1; +} + int qemuProcessStart(virConnectPtr conn, virQEMUDriverPtr driver, virDomainObjPtr vm, @@ -3730,9 +3766,17 @@ int qemuProcessStart(virConnectPtr conn, } if (disk->shared) { + /* qemuAddSharedDisk must be called before qemuCheckSharedDisk + * in case of qemuCheckSharedDisk fails and qemuProcessStop + * will restore the disk's unpriv_sgio if there is only + * one domain is using the shared disk. + */ if (qemuAddSharedDisk(driver->sharedDisks, disk->src, disk->orig_cdbfilter) < 0) goto cleanup; + + if (qemuCheckSharedDisk(driver->sharedDisks, disk) < 0) + goto cleanup; } } diff --git a/src/qemu/qemu_process.h b/src/qemu/qemu_process.h index c12df32..6d92368 100644 --- a/src/qemu/qemu_process.h +++ b/src/qemu/qemu_process.h @@ -99,4 +99,7 @@ bool qemuProcessAutoDestroyActive(virQEMUDriverPtr driver, virBitmapPtr qemuPrepareCpumap(virQEMUDriverPtr driver, virBitmapPtr nodemask); +int qemuCheckSharedDisk(virHashTablePtr sharedDisks, + virDomainDiskDefPtr disk); + #endif /* __QEMU_PROCESS_H__ */ -- 1.7.7.6
On 2012年12月11日 21:37, 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 | 44 ++++++++++++++++++++++++++++++++++++++++++++ src/qemu/qemu_process.h | 3 +++ 2 files changed, 47 insertions(+), 0 deletions(-)
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index f39b83f..ba777e2 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -3380,6 +3380,42 @@ 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). + * + * Returns 0 if no conflicts, otherwise returns -1. + */ +int +qemuCheckSharedDisk(virHashTablePtr sharedDisks, + virDomainDiskDefPtr disk) +{ + int val; + qemuSharedDiskEntryPtr entry = NULL; + + /* It can't be conflict if no other domain is + * is sharing it. + */ + if (!(entry = virHashLookup(sharedDisks, disk->src))) + return 0; + + if (entry->ref == 1) + return 0; + + if (virGetDeviceUnprivSGIO(disk->src,&val)< 0) + return -1; + + if ((val == 0&& + disk->orig_cdbfilter == VIR_DOMAIN_DISK_CDB_FILTER_YES) || + (val == 1&& + disk->orig_cdbfilter == VIR_DOMAIN_DISK_CDB_FILTER_NO)) + return 0; + + virReportError(VIR_ERR_INTERNAL_ERROR, + _("cdbfilter of shared disk '%s' conflicts with other " + "active domains"), disk->src); + return -1; +} + int qemuProcessStart(virConnectPtr conn, virQEMUDriverPtr driver, virDomainObjPtr vm, @@ -3730,9 +3766,17 @@ int qemuProcessStart(virConnectPtr conn, }
if (disk->shared) { + /* qemuAddSharedDisk must be called before qemuCheckSharedDisk + * in case of qemuCheckSharedDisk fails and qemuProcessStop + * will restore the disk's unpriv_sgio if there is only + * one domain is using the shared disk. + */ if (qemuAddSharedDisk(driver->sharedDisks, disk->src, disk->orig_cdbfilter)< 0) goto cleanup; + + if (qemuCheckSharedDisk(driver->sharedDisks, disk)< 0) + goto cleanup; } }
diff --git a/src/qemu/qemu_process.h b/src/qemu/qemu_process.h index c12df32..6d92368 100644 --- a/src/qemu/qemu_process.h +++ b/src/qemu/qemu_process.h @@ -99,4 +99,7 @@ bool qemuProcessAutoDestroyActive(virQEMUDriverPtr driver, virBitmapPtr qemuPrepareCpumap(virQEMUDriverPtr driver, virBitmapPtr nodemask);
+int qemuCheckSharedDisk(virHashTablePtr sharedDisks, + virDomainDiskDefPtr disk); + #endif /* __QEMU_PROCESS_H__ */
With the diff below squashed in: (comparing disk->orig_cdbfilter is not right, it's a typo; And don't try to set the unpriv_sgio before conflict checking, otherwise it could affect the active domain if checking fails;)
Just like for domain starting, the original unpriv_sgio state is recorded in disk def and shareDisks entry for restoring when detaching or shutdown. --- src/qemu/qemu_driver.c | 36 ++++++++++++++++++++++++++++++++---- 1 files changed, 32 insertions(+), 4 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index fdde74d..a16e336 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -6040,11 +6040,39 @@ qemuDomainAttachDeviceDiskLive(virConnectPtr conn, NULLSTR(disk->src)); } - if (ret == 0 && disk->shared) { - if (qemuAddSharedDisk(driver->sharedDisks, disk->src, 0) < 0) - VIR_WARN("Failed to add disk '%s' to shared disk table", - disk->src); + if (ret == 0) { + if (disk->cdbfilter) { + int val; + + /* Store the original unpriv_sgio state */ + if (virGetDeviceUnprivSGIO(disk->src, &val) < 0) { + VIR_WARN("Failed to get unpriv_sgio of disk '%s'", disk->src); + goto end; + } + + if (val == 0) + disk->orig_cdbfilter = VIR_DOMAIN_DISK_CDB_FILTER_YES; + else + disk->orig_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_WARN("Failed to set unpriv_sgio of disk '%s'", disk->src); + goto end; + } + } + + if (disk->shared) { + if (qemuAddSharedDisk(driver->sharedDisks, disk->src, + disk->orig_cdbfilter) < 0) + VIR_WARN("Failed to add disk '%s' to shared disk table", + disk->src); + } } + end: if (cgroup) virCgroupFree(&cgroup); -- 1.7.7.6
Later patch will prohibit restoring it the disk is being shared by other domain(s). Note that this only cares about the non-shared disk. Later patch will change the codes to honor the orig_cdbfilter of sharedDisks entry. --- src/qemu/qemu_driver.c | 24 ++++++++++++++++++++---- 1 files changed, 20 insertions(+), 4 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index a16e336..b394eb9 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -6188,10 +6188,26 @@ qemuDomainDetachDeviceDiskLive(virQEMUDriverPtr driver, break; } - if (ret == 0 && disk->shared) { - if (qemuRemoveSharedDisk(driver->sharedDisks, disk->src) < 0) - VIR_WARN("Failed to remove disk '%s' from shared disk table", - disk->src); + if (ret == 0) { + /* Restore the disk's unpriv_sgio */ + if (disk->cdbfilter) { + int val; + + if (disk->orig_cdbfilter == VIR_DOMAIN_DISK_CDB_FILTER_YES) + val = 0; + else + val = 1; + + if (virSetDeviceUnprivSGIO(disk->src, val) < 0) + VIR_DEBUG("Failed to restore unpriv_sgio for disk '%s'", + disk->src); + } + + if (disk->shared) { + if (qemuRemoveSharedDisk(driver->sharedDisks, disk->src) < 0) + VIR_WARN("Failed to remove disk '%s' from shared disk table", + disk->src); + } } return ret; -- 1.7.7.6
Just like for domain starting, this checks if the shared disk's conf conflicts with others. --- src/qemu/qemu_driver.c | 3 +++ 1 files changed, 3 insertions(+), 0 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index b394eb9..c5e45c5 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -5989,6 +5989,9 @@ qemuDomainAttachDeviceDiskLive(virConnectPtr conn, goto end; } + if (disk->shared && (qemuCheckSharedDisk(driver->sharedDisks, disk) < 0)) + goto end; + if (qemuDomainDetermineDiskChain(driver, disk, false) < 0) goto end; -- 1.7.7.6
Just like for domain shutdown, this prevents restoring the disk's unpriv_sgio if it's being shared by other domain(s). --- src/qemu/qemu_driver.c | 22 +++++++++++++++------- 1 files changed, 15 insertions(+), 7 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index c5e45c5..48201c1 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -6195,15 +6195,23 @@ qemuDomainDetachDeviceDiskLive(virQEMUDriverPtr driver, /* Restore the disk's unpriv_sgio */ if (disk->cdbfilter) { int val; + qemuSharedDiskEntryPtr entry = NULL; - if (disk->orig_cdbfilter == VIR_DOMAIN_DISK_CDB_FILTER_YES) - val = 0; - else - val = 1; + if (!disk->shared || + qemuSharedDiskIsLast(driver->sharedDisks, disk->src)) { + if (!disk->shared) { + val = disk->orig_cdbfilter; + } else { + entry = virHashLookup(driver->sharedDisks, disk->src); + val = entry->orig_cdbfilter; + } - if (virSetDeviceUnprivSGIO(disk->src, val) < 0) - VIR_DEBUG("Failed to restore unpriv_sgio for disk '%s'", - disk->src); + if (virSetDeviceUnprivSGIO(disk->src, + (val == VIR_DOMAIN_DISK_CDB_FILTER_YES) ? + 0 : 1) < 0) + VIR_DEBUG("Failed to restore unpriv_sgio for disk '%s'", + disk->src); + } } if (disk->shared) { -- 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 1edba69..599d299 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, @@ -3535,6 +3536,7 @@ virDomainDiskDefParseXML(virCapsPtr caps, char *snapshot = NULL; char *rawio = NULL; char *cdbfilter = NULL; + char *orig_cdbfilter = NULL; char *driverName = NULL; char *driverType = NULL; char *source = NULL; @@ -3600,6 +3602,8 @@ virDomainDiskDefParseXML(virCapsPtr caps, rawio = virXMLPropString(node, "rawio"); cdbfilter = virXMLPropString(node, "cdbfilter"); + if (flags & VIR_DOMAIN_XML_INTERNAL_OLD_DISK_CDBFILTER) + orig_cdbfilter = virXMLPropString(node, "orig_cdbfilter"); cur = node->children; while (cur != NULL) { @@ -4092,6 +4096,19 @@ virDomainDiskDefParseXML(virCapsPtr caps, def->cdbfilter = cdbfilter_val; } + if (orig_cdbfilter) { + int orig_cdbfilter_val = 0; + + if ((orig_cdbfilter_val = + virDomainDiskCDBFilterTypeFromString(orig_cdbfilter)) < 0) { + virReportError(VIR_ERR_XML_ERROR, + _("unknown disk cdbfilter setting '%s'"), + orig_cdbfilter); + goto error; + } + def->orig_cdbfilter = orig_cdbfilter_val; + } + if (bus) { if ((def->bus = virDomainDiskBusTypeFromString(bus)) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, @@ -12014,6 +12031,12 @@ virDomainDiskDefFormat(virBufferPtr buf, if (def->cdbfilter) virBufferAsprintf(buf, " cdbfilter='%s'", virDomainDiskCDBFilterTypeToString(def->cdbfilter)); + + if ((flags & VIR_DOMAIN_XML_INTERNAL_OLD_DISK_CDBFILTER) && + def->orig_cdbfilter) + virBufferAsprintf(buf, " orig_cdbfilter='%s'", + virDomainDiskCDBFilterTypeToString(def->orig_cdbfilter)); + if (def->snapshot && !(def->snapshot == VIR_DOMAIN_SNAPSHOT_LOCATION_NONE && def->readonly)) virBufferAsprintf(buf, " snapshot='%s'", @@ -13685,7 +13708,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_*, @@ -13707,7 +13731,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))) { @@ -14465,7 +14490,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; @@ -14565,7 +14591,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
On Tue, Dec 11, 2012 at 09:37:29PM +0800, Osier Yang wrote:
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 1edba69..599d299 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, @@ -3535,6 +3536,7 @@ virDomainDiskDefParseXML(virCapsPtr caps, char *snapshot = NULL; char *rawio = NULL; char *cdbfilter = NULL; + char *orig_cdbfilter = NULL; char *driverName = NULL; char *driverType = NULL; char *source = NULL; @@ -3600,6 +3602,8 @@ virDomainDiskDefParseXML(virCapsPtr caps,
rawio = virXMLPropString(node, "rawio"); cdbfilter = virXMLPropString(node, "cdbfilter"); + if (flags & VIR_DOMAIN_XML_INTERNAL_OLD_DISK_CDBFILTER) + orig_cdbfilter = virXMLPropString(node, "orig_cdbfilter");
cur = node->children; while (cur != NULL) { @@ -4092,6 +4096,19 @@ virDomainDiskDefParseXML(virCapsPtr caps, def->cdbfilter = cdbfilter_val; }
+ if (orig_cdbfilter) { + int orig_cdbfilter_val = 0; + + if ((orig_cdbfilter_val = + virDomainDiskCDBFilterTypeFromString(orig_cdbfilter)) < 0) { + virReportError(VIR_ERR_XML_ERROR, + _("unknown disk cdbfilter setting '%s'"), + orig_cdbfilter); + goto error; + } + def->orig_cdbfilter = orig_cdbfilter_val; + } + if (bus) { if ((def->bus = virDomainDiskBusTypeFromString(bus)) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, @@ -12014,6 +12031,12 @@ virDomainDiskDefFormat(virBufferPtr buf, if (def->cdbfilter) virBufferAsprintf(buf, " cdbfilter='%s'", virDomainDiskCDBFilterTypeToString(def->cdbfilter)); + + if ((flags & VIR_DOMAIN_XML_INTERNAL_OLD_DISK_CDBFILTER) && + def->orig_cdbfilter) + virBufferAsprintf(buf, " orig_cdbfilter='%s'", + virDomainDiskCDBFilterTypeToString(def->orig_cdbfilter)); + if (def->snapshot && !(def->snapshot == VIR_DOMAIN_SNAPSHOT_LOCATION_NONE && def->readonly)) virBufferAsprintf(buf, " snapshot='%s'", @@ -13685,7 +13708,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_*, @@ -13707,7 +13731,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))) { @@ -14465,7 +14490,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; @@ -14565,7 +14591,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);
I must say I really don't like using virDomainDefPtr to hold driver state information like this. The driver specific domain status XML is really the place for it. Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On Tue, Dec 11, 2012 at 09:37:17PM +0800, Osier Yang wrote:
As a result of RFC [1], this implements the unprivleged SG_IO support.
v4 - v5: * Per discussion in [2], this set uses a hash table to store the shared disk's info. See 1/12 for more details.
Osier Yang (12): qemu: Add a hash table for the shared disks docs: Add docs and rng schema for new XML cdbfilter conf: Parse and format the new XML tag cdbfilter util: Prepare helpers for unpriv_sgio setting qemu: Manage disk's unpriv_sgio in domain lifecyle qemu: Do not restore the sysfs unpriv_sgio if the disk is being shared qemu: Error out when domain starting if the cdbfilter setting conflicts qemu: Set unpriv_sgio when attaching disk qemu: Restore unpriv_sgio when detaching disk qemu: Error out if the shared disk conf conflicts with others when attaching qemu: Do not restore unpriv_sgio when detaching disk if it's still shared conf: Save disk's original unpriv_sgio state into status XML
IMHO this series would be a hell of alot simpler if we just didn't bother restoring the original CBD value. The value only really matters when VMs are actually running in any case Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On 2012年12月13日 01:18, Daniel P. Berrange wrote:
On Tue, Dec 11, 2012 at 09:37:17PM +0800, Osier Yang wrote:
As a result of RFC [1], this implements the unprivleged SG_IO support.
v4 - v5: * Per discussion in [2], this set uses a hash table to store the shared disk's info. See 1/12 for more details.
Osier Yang (12): qemu: Add a hash table for the shared disks docs: Add docs and rng schema for new XML cdbfilter conf: Parse and format the new XML tag cdbfilter util: Prepare helpers for unpriv_sgio setting qemu: Manage disk's unpriv_sgio in domain lifecyle qemu: Do not restore the sysfs unpriv_sgio if the disk is being shared qemu: Error out when domain starting if the cdbfilter setting conflicts qemu: Set unpriv_sgio when attaching disk qemu: Restore unpriv_sgio when detaching disk qemu: Error out if the shared disk conf conflicts with others when attaching qemu: Do not restore unpriv_sgio when detaching disk if it's still shared conf: Save disk's original unpriv_sgio state into status XML
IMHO this series would be a hell of alot simpler if we just didn't bother restoring the original CBD value. The value only really matters when VMs are actually running in any case
Yeah, that will be a lot simpler if we don't care about restoring the orignal unpriv_sgio. And it can avoid many risks. It basicly follows the non-agreement in [1]: <...> When libvirtd starting, set the sysfs knob "unpriv_sgio" of the devices listed to 1, and 0 when libvirtd exists. </...> But now I doubt about this after I went that way forward, especially it makes no sense after a libvirtd restarting. Should we avoid the restoring? Regards, Osier
participants (3)
-
Daniel P. Berrange -
Jiri Denemark -
Osier Yang