7:37 a.m.
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
7:37 a.m.
New subject: [libvirt] [PATCH 01/12] qemu: Add a hash table for the shared disks
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
9:35 p.m.
New subject: [libvirt] [PATCH 01/12] qemu: Add a hash table for the shared disks
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).
9:39 p.m.
New subject: [libvirt] [PATCH 01/12] qemu: Add a hash table for the shared disks
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(a)redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
11:09 a.m.
New subject: [libvirt] [PATCH 01/12] qemu: Add a hash table for the shared disks
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 :|
9:13 p.m.
New subject: [libvirt] [PATCH 01/12] qemu: Add a hash table for the shared disks
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
11:16 a.m.
New subject: [libvirt] [PATCH 01/12] qemu: Add a hash table for the shared disks
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 :|
9:34 p.m.
New subject: [libvirt] [PATCH 01/12] qemu: Add a hash table for the shared disks
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.
5:05 a.m.
New subject: [libvirt] [PATCH 01/12] qemu: Add a hash table for the shared disks
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
5:14 a.m.
New subject: [libvirt] [PATCH 01/12] qemu: Add a hash table for the shared disks
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
7:37 a.m.
New subject: [libvirt] [PATCH 02/12] docs: Add docs and rng schema for new XML cdbfilter
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
7:37 a.m.
New subject: [libvirt] [PATCH 03/12] conf: Parse and format the new XML tag cdbfilter
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
7:37 a.m.
New subject: [libvirt] [PATCH 04/12] util: Prepare helpers for unpriv_sgio setting
"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
7:37 a.m.
New subject: [libvirt] [PATCH 05/12] qemu: Manage disk's unpriv_sgio in domain lifecyle
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
11:13 a.m.
New subject: [libvirt] [PATCH 05/12] qemu: Manage disk's unpriv_sgio in domain lifecyle
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 :|
9:23 p.m.
New subject: [libvirt] [PATCH 05/12] qemu: Manage disk's unpriv_sgio in domain lifecyle
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
7:37 a.m.
New subject: [libvirt] [PATCH 06/12] qemu: Do not restore the sysfs unpriv_sgio if the disk is being shared
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
7:37 a.m.
New subject: [libvirt] [PATCH 07/12] qemu: Error out when domain starting if the cdbfilter setting conflicts
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
10:24 a.m.
New subject: [libvirt] [PATCH 07/12] qemu: Error out when domain starting if the cdbfilter setting conflicts
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;)
7:37 a.m.
New subject: [libvirt] [PATCH 08/12] qemu: Set unpriv_sgio when attaching disk
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
7:37 a.m.
New subject: [libvirt] [PATCH 09/12] qemu: Restore unpriv_sgio when detaching disk
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
7:37 a.m.
New subject: [libvirt] [PATCH 10/12] qemu: Error out if the shared disk conf conflicts with others when attaching
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
7:37 a.m.
New subject: [libvirt] [PATCH 11/12] qemu: Do not restore unpriv_sgio when detaching disk if it's still shared
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
7:37 a.m.
New subject: [libvirt] [PATCH 12/12] conf: Save disk's original unpriv_sgio state into status XML
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
11:21 a.m.
New subject: [libvirt] [PATCH 12/12] conf: Save disk's original unpriv_sgio state into status XML
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 :|
11:18 a.m.
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 :|
9:35 p.m.
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
4363
days inactive
4365
days old
26 comments
3 participants
participants (3)
-
Daniel P. Berrange -
Jiri Denemark -
Osier Yang