7:21 p.m.
Hi,
As a result of RFC [1], this implements the unprivleged SG_IO
support. Though this is a bit rushed, and I still have no chance
to test it yet, but I'd like see an earlier reviewing.
@Paolo, please let me known once you have a kernel build with
the new sysfs knob support. Thanks!
Osier Yang (6):
unpriv_sgio: Add docs and rng schema for new XML unpriv_sgio
unpriv_sgio: Parse and format the new XML
unpriv_sgio: Prepare helpers for unpriv_sgio setting
unpriv_sgio: Manage unpriv_sgio in domain's lifecyle
unpriv_sgio: Do not restore unpriv_sgio if the disk is being used
unpriv_sgio: Error out if the unpriv_sgio setting conflicts with
others
docs/formatdomain.html.in | 10 +-
docs/schemas/domaincommon.rng | 52 ++++--
src/conf/domain_conf.c | 168 ++++++++++++++++++--
src/conf/domain_conf.h | 18 ++
src/libvirt_private.syms | 6 +
src/qemu/qemu_process.c | 54 +++++++
src/util/util.c | 154 ++++++++++++++++++
src/util/util.h | 7 +
...l2argv-disk-scsi-lun-passthrough-upriv-sgio.xml | 32 ++++
tests/qemuxml2xmltest.c | 1 +
10 files changed, 472 insertions(+), 30 deletions(-)
create mode 100644
tests/qemuxml2argvdata/qemuxml2argv-disk-scsi-lun-passthrough-upriv-sgio.xml
[1] https://www.redhat.com/archives/libvir-list/2012-November/msg00988.html
Regards,
Osier
7:21 p.m.
New subject: [libvirt] [PATCH 1/6] unpriv_sgio: Add docs and rng schema for new XML unpriv_sgio
Since "rawio" and "unpriv_sgio" are only valid for "lun",
this
groups them together. And since both of them intend to allow
the unprivledged user to use the SCSI commands, they are must be
exclusive. Actually "unpriv_sgio" supersedes "rawio", as it
confines the capability per-device, unlike "rawio", which gives
the domain process broad capablity.
---
docs/formatdomain.html.in | 10 +++-
docs/schemas/domaincommon.rng | 52 ++++++++++----
src/conf/domain_conf.c | 56 ++++++++++++----
src/conf/domain_conf.h | 11 +++
src/libvirt_private.syms | 4 +
src/qemu/qemu_process.c | 30 ++++++++
src/util/util.c | 153 +++++++++++++++++++++++++++++++++++++++++
src/util/util.h | 7 ++
8 files changed, 293 insertions(+), 30 deletions(-)
diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
index 6a3b976..f3f6a9e 100644
--- a/docs/formatdomain.html.in
+++ b/docs/formatdomain.html.in
@@ -1395,7 +1395,15 @@
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> gives
+ the domain process broad capability, to confined the capability
+ as much as possible, one should use <code>unpriv_sgio</code>
+ instead, which controls the capability per-device.
+ The optional <code>unpriv_sgio</code> attribute
+ (<span class="since">since 1.0.1</span>) indicates whether
the
+ disk will allow unprivileged SG_IO, valid settings are "yes"
+ or "no" (defaults to "no"). Note that it's exclusive
with
+ attribute <code>rawio</code>;
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 02ad477..7da571c 100644
--- a/docs/schemas/domaincommon.rng
+++ b/docs/schemas/domaincommon.rng
@@ -957,24 +957,44 @@
-->
<define name="disk">
<element name="disk">
- <optional>
- <attribute name="device">
- <choice>
- <value>floppy</value>
- <value>disk</value>
- <value>cdrom</value>
- <value>lun</value>
- </choice>
- </attribute>
- </optional>
- <optional>
- <attribute name="rawio">
+ <choice>
+ <group>
+ <optional>
+ <attribute name="device">
+ <choice>
+ <value>floppy</value>
+ <value>disk</value>
+ <value>cdrom</value>
+ </choice>
+ </attribute>
+ </optional>
+ </group>
+ <group>
+ <optional>
+ <attribute name="device">
+ <value>lun</value>
+ </attribute>
+ </optional>
<choice>
- <value>yes</value>
- <value>no</value>
+ <optional>
+ <attribute name="rawio">
+ <choice>
+ <value>yes</value>
+ <value>no</value>
+ </choice>
+ </attribute>
+ </optional>
+ <optional>
+ <attribute name="unpriv_sgio">
+ <choice>
+ <value>yes</value>
+ <value>no</value>
+ </choice>
+ </attribute>
+ </optional>
</choice>
- </attribute>
- </optional>
+ </group>
+ </choice>
<optional>
<ref name="snapshot"/>
</optional>
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index ed8b53f..e57dbd0 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -236,6 +236,10 @@ VIR_ENUM_IMPL(virDomainDiskIo, VIR_DOMAIN_DISK_IO_LAST,
"default",
"native",
"threads")
+VIR_ENUM_IMPL(virDomainDiskSGIO, VIR_DOMAIN_DISK_SGIO_LAST,
+ "default",
+ "yes",
+ "no")
VIR_ENUM_IMPL(virDomainIoEventFd, VIR_DOMAIN_IO_EVENT_FD_LAST,
"default",
"on",
@@ -3515,6 +3519,7 @@ virDomainDiskDefParseXML(virCapsPtr caps,
char *device = NULL;
char *snapshot = NULL;
char *rawio = NULL;
+ char *sgio = NULL;
char *driverName = NULL;
char *driverType = NULL;
char *source = NULL;
@@ -3576,6 +3581,7 @@ virDomainDiskDefParseXML(virCapsPtr caps,
snapshot = virXMLPropString(node, "snapshot");
rawio = virXMLPropString(node, "rawio");
+ sgio = virXMLPropString(node, "sgio");
cur = node->children;
while (cur != NULL) {
@@ -3966,24 +3972,44 @@ virDomainDiskDefParseXML(virCapsPtr caps,
def->snapshot = VIR_DOMAIN_SNAPSHOT_LOCATION_NONE;
}
+ if (rawio && sgio) {
+ virReportError(VIR_ERR_XML_ERROR, "%s",
+ _("rawio and sgio are exclusive"));
+ goto error;
+ }
+
+ if ((rawio || sgio) &&
+ (def->device != VIR_DOMAIN_DISK_DEVICE_LUN)) {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+ _("rawio 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 (sgio) {
+ int sgioVal = 0;
+
+ if ((sgioVal = virDomainDiskSGIOTypeFromString(sgio)) < 0) {
+ virReportError(VIR_ERR_XML_ERROR,
+ _("unknown disk sgio setting '%s'"),
+ sgio);
goto error;
}
+
+ def->sgio = sgioVal;
}
if (bus) {
@@ -4220,6 +4246,7 @@ cleanup:
VIR_FREE(type);
VIR_FREE(snapshot);
VIR_FREE(rawio);
+ VIR_FREE(sgio);
VIR_FREE(target);
VIR_FREE(source);
VIR_FREE(tray);
@@ -11897,6 +11924,9 @@ virDomainDiskDefFormat(virBufferPtr buf,
virBufferAddLit(buf, " rawio='no'");
}
}
+ if (def->sgio)
+ virBufferAsprintf(buf, " sgio='%s'",
+ virDomainDiskSGIOTypeToString(def->sgio));
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 c3e8c16..9e1a9bb 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -496,6 +496,14 @@ enum virDomainDiskIo {
VIR_DOMAIN_DISK_IO_LAST
};
+enum virDomainDiskSGIO {
+ VIR_DOMAIN_DISK_SGIO_DEFAULT = 0,
+ VIR_DOMAIN_DISK_SGIO_YES,
+ VIR_DOMAIN_DISK_SGIO_NO,
+
+ VIR_DOMAIN_DISK_SGIO_LAST
+};
+
enum virDomainIoEventFd {
VIR_DOMAIN_IO_EVENT_FD_DEFAULT = 0,
VIR_DOMAIN_IO_EVENT_FD_ON,
@@ -607,6 +615,8 @@ struct _virDomainDiskDef {
virStorageEncryptionPtr encryption;
bool rawio_specified;
int rawio; /* no = 0, yes = 1 */
+ int sgio; /* no = 0, yes = 1 */
+ int old_sgio; /* To record the old unpriv_sgio value, internally */
size_t nseclabels;
virSecurityDeviceLabelDefPtr *seclabels;
@@ -2197,6 +2207,7 @@ VIR_ENUM_DECL(virDomainDiskCache)
VIR_ENUM_DECL(virDomainDiskErrorPolicy)
VIR_ENUM_DECL(virDomainDiskProtocol)
VIR_ENUM_DECL(virDomainDiskIo)
+VIR_ENUM_DECL(virDomainDiskSGIO)
VIR_ENUM_DECL(virDomainDiskSecretType)
VIR_ENUM_DECL(virDomainDiskTray)
VIR_ENUM_DECL(virDomainIoEventFd)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 0115db1..7abaf27 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -1257,6 +1257,9 @@ virFileWaitForDevices;
virFileWriteStr;
virFindFileInPath;
virFormatIntDecimal;
+virGetDeviceMajor;
+virGetDeviceMinor;
+virGetDeviceSGIO;
virGetGroupID;
virGetGroupName;
virGetHostname;
@@ -1275,6 +1278,7 @@ virPipeReadUntilEOF;
virScaleInteger;
virSetBlocking;
virSetCloseExec;
+virSetDeviceSGIO;
virSetInherit;
virSetNonBlock;
virSetUIDGID;
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index 3d7a5a0..98096c5 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -3703,6 +3703,25 @@ int qemuProcessStart(virConnectPtr conn,
virCommandAllowCap(cmd, CAP_SYS_RAWIO);
}
+ /* Set unpriv_sgio for disks */
+ for (i = 0; i < vm->def->ndisks; i++) {
+ virDomainDiskDefPtr disk = vm->def->disks[i];
+ int old_sgio;
+
+ if (!disk->sgio)
+ continue;
+
+ if (virGetDeviceSGIO(disk->src, &old_sgio) < 0)
+ goto cleanup;
+
+ disk->old_sgio = old_sgio;
+
+ if (virSetDeviceSGIO(disk->src,
+ (disk->sgio == VIR_DOMAIN_DISK_SGIO_YES)
+ ? 1 : 0) < 0)
+ goto cleanup;
+ }
+
virCommandSetPreExecHook(cmd, qemuProcessHook, &hookData);
virCommandSetOutputFD(cmd, &logfile);
@@ -4093,6 +4112,17 @@ void qemuProcessStop(struct qemud_driver *driver,
flags & VIR_QEMU_PROCESS_STOP_MIGRATED);
virSecurityManagerReleaseLabel(driver->securityManager, vm->def);
+ /* Restore disk's unpriv_sgio */
+ for (i = 0; i < vm->def->ndisks; i++) {
+ virDomainDiskDefPtr disk = vm->def->disks[i];
+
+ if (!disk->sgio)
+ continue;
+
+ if (virSetDeviceSGIO(disk->src, disk->old_sgio) < 0)
+ VIR_WARN("Unable to restore unpriv_sgio for disk '%s'",
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) {
diff --git a/src/util/util.c b/src/util/util.c
index 2fd0f2c..746f3e9 100644
--- a/src/util/util.c
+++ b/src/util/util.c
@@ -3113,3 +3113,156 @@ virValidateWWN(const char *wwn) {
return true;
}
+
+#if defined(major) && defined(minor)
+int
+virGetDeviceMajor(const char *path)
+{
+ struct stat sb;
+
+ if (stat(path, &sb) < 0)
+ return -errno;
+
+ if (!S_ISBLK(sb.st_mode))
+ return -EINVAL;
+
+ return major(sb.st_rdev);
+}
+
+int
+virGetDeviceMinor(const char *path)
+{
+ struct stat sb;
+
+ if (stat(path, &sb) < 0)
+ return -errno;
+
+ if (!S_ISBLK(sb.st_mode))
+ return -EINVAL;
+
+ return minor(sb.st_rdev);
+}
+#else
+int
+virGetDeviceMajor(const char *path)
+{
+ return -ENOSYS;
+}
+
+int
+virGetDeviceMinor(const char *path)
+{
+ return -ENOSYS;
+}
+#endif
+
+int
+virSetDeviceSGIO(const char *path,
+ int sgio)
+{
+ char *sysfs_path = NULL;
+ char *val = NULL;
+ int major;
+ int minor;
+ int ret = -1;
+ int rc = -1;
+
+ if ((major = virGetDeviceMajor(path)) < 0) {
+ virReportSystemError(-major,
+ _("Unable to get major number of device
'%s'"),
+ path);
+ return -1;
+ }
+
+ if ((minor = virGetDeviceMinor(path)) < 0) {
+ virReportSystemError(-minor,
+ _("Unable to get minor number of device
'%s'"),
+ path);
+ return -1;
+ }
+
+ if (virAsprintf(&sysfs_path, "/sys/dev/block/%d:%d/queue/unpriv_sgio",
+ major, minor) < 0) {
+ virReportOOMError();
+ 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", 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
+virGetDeviceSGIO(const char *path,
+ int *sgio)
+{
+ char *sysfs_path = NULL;
+ char *buf = NULL;
+ int major;
+ int minor;
+ int ret = -1;
+ int rc = -1;
+
+ if ((major = virGetDeviceMajor(path)) < 0) {
+ virReportSystemError(-major,
+ _("Unable to get major number of device
'%s'"),
+ path);
+ return -1;
+ }
+
+ if ((minor = virGetDeviceMinor(path)) < 0) {
+ virReportSystemError(-minor,
+ _("Unable to get minor number of device
'%s'"),
+ path);
+ return -1;
+ }
+
+ if (virAsprintf(&sysfs_path, "/sys/dev/block/%d:%d/queue/unpriv_sgio",
+ major, minor) < 0) {
+ virReportOOMError();
+ 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,
+ sgio)) < 0) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("failed to parse value of %s"), sysfs_path);
+ goto cleanup;
+ }
+
+ ret = 0;
+cleanup:
+ VIR_FREE(sysfs_path);
+ VIR_FREE(buf);
+ return ret;
+}
diff --git a/src/util/util.h b/src/util/util.h
index 4316ab1..0bd9f79 100644
--- a/src/util/util.h
+++ b/src/util/util.h
@@ -280,4 +280,11 @@ bool virIsDevMapperDevice(const char *dev_name)
ATTRIBUTE_NONNULL(1);
bool virValidateWWN(const char *wwn);
+int virGetDeviceMajor(const char *path);
+int virGetDeviceMinor(const char *path);
+int virSetDeviceSGIO(const char *path,
+ int sgio);
+int virGetDeviceSGIO(const char *path,
+ int *sgio);
+
#endif /* __VIR_UTIL_H__ */
--
1.7.7.6
7:38 p.m.
New subject: [libvirt] [PATCH 1/6] unpriv_sgio: Add docs and rng schema for new XML unpriv_sgio
On Mon, Nov 26, 2012 at 06:21:37PM +0800, Osier Yang wrote:
Since "rawio" and "unpriv_sgio" are only valid
for "lun", this
I think that's a ugly attribute name, just use 'sgio'.
I'd tend to say that the rawio behaviour you describe is
really just a QEMU implementation detail. The XML itself
is written on the basis that the 'rawio' flag is specific
to one device. eg it would be possible to make the 'rawio'
flag work on just a single device if we really wanted to
do the kernel work.
As above, I think this needs re-wording to clearly differentiate
between the intended meaning of the 'rawio' attribute in general,
vs the current QEMU specific impl of that attribute.
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 :|
8:26 p.m.
New subject: [libvirt] [PATCH 1/6] unpriv_sgio: Add docs and rng schema for new XML unpriv_sgio
Il 26/11/2012 11:38, Daniel P. Berrange ha scritto:
> > Since "rawio" and "unpriv_sgio" are only
valid for "lun", this
I think that's a ugly attribute name, just use 'sgio'.
That doesn't explain what it is for. Some possibilities:
- filter-scsi-commands=yes/no
- cdb-filter=yes/no
- privileged-scsi-commands=allow/deny
- cdb=allow-all/filter
Paolo
4:08 p.m.
New subject: [libvirt] [PATCH 1/6] unpriv_sgio: Add docs and rng schema for new XML unpriv_sgio
On 2012年11月26日 19:26, Paolo Bonzini wrote:
Yeah, I used "sgio" in the first drafts, but changed
to "unpriv_sgio" afterwards, for realizing it can't
tell what the tag does internally.
Some possibilities:
As far as I get from your kernel patches, the unpriv_sgio
allows any SG_IO commands for unprivileged users, without
filtering.
So I'm wondering if using the term "CBD filter" matches
the exact meaning.
Regards,
Osier
6:16 p.m.
New subject: [libvirt] [PATCH 1/6] unpriv_sgio: Add docs and rng schema for new XML unpriv_sgio
Il 27/11/2012 08:08, Osier Yang ha scritto:
The question is: "will the guest be subject to filtering of some CDBs?"
It's an implementation detail that the kernel does that rather than QEMU...
Paolo
7:21 p.m.
New subject: [libvirt] [PATCH 2/6] unpriv_sgio: Parse and format the new XML
Setting "unpriv_sgio" to "yes" to enable the unprivleged SG_IO,
and "no" to disable it. Later patch will do the actual setting.
---
src/conf/domain_conf.c | 33 ++++++++++---------
src/conf/domain_conf.h | 15 ++++-----
...l2argv-disk-scsi-lun-passthrough-upriv-sgio.xml | 32 +++++++++++++++++++
tests/qemuxml2xmltest.c | 1 +
4 files changed, 57 insertions(+), 24 deletions(-)
create mode 100644
tests/qemuxml2argvdata/qemuxml2argv-disk-scsi-lun-passthrough-upriv-sgio.xml
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index e57dbd0..b25229a 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -236,7 +236,7 @@ VIR_ENUM_IMPL(virDomainDiskIo, VIR_DOMAIN_DISK_IO_LAST,
"default",
"native",
"threads")
-VIR_ENUM_IMPL(virDomainDiskSGIO, VIR_DOMAIN_DISK_SGIO_LAST,
+VIR_ENUM_IMPL(virDomainDiskUnprivSGIO, VIR_DOMAIN_DISK_UNPRIV_SGIO_LAST,
"default",
"yes",
"no")
@@ -3519,7 +3519,7 @@ virDomainDiskDefParseXML(virCapsPtr caps,
char *device = NULL;
char *snapshot = NULL;
char *rawio = NULL;
- char *sgio = NULL;
+ char *unpriv_sgio = NULL;
char *driverName = NULL;
char *driverType = NULL;
char *source = NULL;
@@ -3581,7 +3581,7 @@ virDomainDiskDefParseXML(virCapsPtr caps,
snapshot = virXMLPropString(node, "snapshot");
rawio = virXMLPropString(node, "rawio");
- sgio = virXMLPropString(node, "sgio");
+ unpriv_sgio = virXMLPropString(node, "unpriv_sgio");
cur = node->children;
while (cur != NULL) {
@@ -3972,13 +3972,13 @@ virDomainDiskDefParseXML(virCapsPtr caps,
def->snapshot = VIR_DOMAIN_SNAPSHOT_LOCATION_NONE;
}
- if (rawio && sgio) {
+ if (rawio && unpriv_sgio) {
virReportError(VIR_ERR_XML_ERROR, "%s",
- _("rawio and sgio are exclusive"));
+ _("rawio and unpriv_sgio are exclusive"));
goto error;
}
- if ((rawio || sgio) &&
+ if ((rawio || unpriv_sgio) &&
(def->device != VIR_DOMAIN_DISK_DEVICE_LUN)) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
_("rawio can be used only with device='lun'"));
@@ -3999,17 +3999,18 @@ virDomainDiskDefParseXML(virCapsPtr caps,
}
}
- if (sgio) {
- int sgioVal = 0;
+ if (unpriv_sgio) {
+ int unpriv_sgio_val = 0;
- if ((sgioVal = virDomainDiskSGIOTypeFromString(sgio)) < 0) {
+ if ((unpriv_sgio_val =
+ virDomainDiskUnprivSGIOTypeFromString(unpriv_sgio)) < 0) {
virReportError(VIR_ERR_XML_ERROR,
- _("unknown disk sgio setting '%s'"),
- sgio);
+ _("unknown disk unpriv_sgio setting '%s'"),
+ unpriv_sgio);
goto error;
}
- def->sgio = sgioVal;
+ def->unpriv_sgio = unpriv_sgio_val;
}
if (bus) {
@@ -4246,7 +4247,7 @@ cleanup:
VIR_FREE(type);
VIR_FREE(snapshot);
VIR_FREE(rawio);
- VIR_FREE(sgio);
+ VIR_FREE(unpriv_sgio);
VIR_FREE(target);
VIR_FREE(source);
VIR_FREE(tray);
@@ -11924,9 +11925,9 @@ virDomainDiskDefFormat(virBufferPtr buf,
virBufferAddLit(buf, " rawio='no'");
}
}
- if (def->sgio)
- virBufferAsprintf(buf, " sgio='%s'",
- virDomainDiskSGIOTypeToString(def->sgio));
+ if (def->unpriv_sgio)
+ virBufferAsprintf(buf, " unpriv_sgio='%s'",
+ virDomainDiskUnprivSGIOTypeToString(def->unpriv_sgio));
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 9e1a9bb..105fb7d 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -496,12 +496,12 @@ enum virDomainDiskIo {
VIR_DOMAIN_DISK_IO_LAST
};
-enum virDomainDiskSGIO {
- VIR_DOMAIN_DISK_SGIO_DEFAULT = 0,
- VIR_DOMAIN_DISK_SGIO_YES,
- VIR_DOMAIN_DISK_SGIO_NO,
+enum virDomainDiskUnprivSGIO {
+ VIR_DOMAIN_DISK_UNPRIV_SGIO_DEFAULT = 0,
+ VIR_DOMAIN_DISK_UNPRIV_SGIO_YES,
+ VIR_DOMAIN_DISK_UNPRIV_SGIO_NO,
- VIR_DOMAIN_DISK_SGIO_LAST
+ VIR_DOMAIN_DISK_UNPRIV_SGIO_LAST
};
enum virDomainIoEventFd {
@@ -615,8 +615,7 @@ struct _virDomainDiskDef {
virStorageEncryptionPtr encryption;
bool rawio_specified;
int rawio; /* no = 0, yes = 1 */
- int sgio; /* no = 0, yes = 1 */
- int old_sgio; /* To record the old unpriv_sgio value, internally */
+ int unpriv_sgio; /* no = 0, yes = 1 */
size_t nseclabels;
virSecurityDeviceLabelDefPtr *seclabels;
@@ -2207,7 +2206,7 @@ VIR_ENUM_DECL(virDomainDiskCache)
VIR_ENUM_DECL(virDomainDiskErrorPolicy)
VIR_ENUM_DECL(virDomainDiskProtocol)
VIR_ENUM_DECL(virDomainDiskIo)
-VIR_ENUM_DECL(virDomainDiskSGIO)
+VIR_ENUM_DECL(virDomainDiskUnprivSGIO)
VIR_ENUM_DECL(virDomainDiskSecretType)
VIR_ENUM_DECL(virDomainDiskTray)
VIR_ENUM_DECL(virDomainIoEventFd)
diff --git a/tests/qemuxml2argvdata/qemuxml2argv-disk-scsi-lun-passthrough-upriv-sgio.xml
b/tests/qemuxml2argvdata/qemuxml2argv-disk-scsi-lun-passthrough-upriv-sgio.xml
new file mode 100644
index 0000000..d8f9d12
--- /dev/null
+++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-scsi-lun-passthrough-upriv-sgio.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' unpriv_sgio='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' unpriv_sgio='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 1d366f1..9cb4d88 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-upriv-sgio");
/* These tests generate different XML */
DO_TEST_DIFFERENT("balloon-device-auto");
--
1.7.7.6
7:21 p.m.
New subject: [libvirt] [PATCH 3/6] unpriv_sgio: 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 | 13 +++++++------
src/util/util.h | 8 ++++----
3 files changed, 13 insertions(+), 12 deletions(-)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 7abaf27..c756130 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -1259,7 +1259,7 @@ virFindFileInPath;
virFormatIntDecimal;
virGetDeviceMajor;
virGetDeviceMinor;
-virGetDeviceSGIO;
+virGetDeviceUnprivSGIO;
virGetGroupID;
virGetGroupName;
virGetHostname;
@@ -1278,7 +1278,7 @@ virPipeReadUntilEOF;
virScaleInteger;
virSetBlocking;
virSetCloseExec;
-virSetDeviceSGIO;
+virSetDeviceUnprivSGIO;
virSetInherit;
virSetNonBlock;
virSetUIDGID;
diff --git a/src/util/util.c b/src/util/util.c
index 746f3e9..49619d0 100644
--- a/src/util/util.c
+++ b/src/util/util.c
@@ -3157,8 +3157,8 @@ virGetDeviceMinor(const char *path)
#endif
int
-virSetDeviceSGIO(const char *path,
- int sgio)
+virSetDeviceUnprivSGIO(const char *path,
+ int unpriv_sgio)
{
char *sysfs_path = NULL;
char *val = NULL;
@@ -3193,7 +3193,7 @@ virSetDeviceSGIO(const char *path,
goto cleanup;
}
- if (virAsprintf(&val, "%d", sgio) < 0) {
+ if (virAsprintf(&val, "%d", unpriv_sgio) < 0) {
virReportOOMError();
goto cleanup;
}
@@ -3211,11 +3211,12 @@ cleanup:
}
int
-virGetDeviceSGIO(const char *path,
- int *sgio)
+virGetDeviceUnprivSGIO(const char *path,
+ int *unpriv_sgio)
{
char *sysfs_path = NULL;
char *buf = NULL;
+ char *tmp = NULL;
int major;
int minor;
int ret = -1;
@@ -3254,7 +3255,7 @@ virGetDeviceSGIO(const char *path,
*tmp = '\0';
if ((rc = virStrToLong_i(buf, NULL, 10,
- sgio)) < 0) {
+ unpriv_sgio)) < 0) {
virReportError(VIR_ERR_INTERNAL_ERROR,
_("failed to parse value of %s"), sysfs_path);
goto cleanup;
diff --git a/src/util/util.h b/src/util/util.h
index 0bd9f79..41146c1 100644
--- a/src/util/util.h
+++ b/src/util/util.h
@@ -282,9 +282,9 @@ bool virValidateWWN(const char *wwn);
int virGetDeviceMajor(const char *path);
int virGetDeviceMinor(const char *path);
-int virSetDeviceSGIO(const char *path,
- int sgio);
-int virGetDeviceSGIO(const char *path,
- int *sgio);
+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:21 p.m.
New subject: [libvirt] [PATCH 4/6] unpriv_sgio: Manage unpriv_sgio in domain's lifecyle
To record the original "unpriv_sgio" value, this introduces
"old_unpriv_sgio" for disk def. When the domain is starting,
the disk's "unpriv_sgio" is set with regards to the config
in domain XML. And when the domain is being destroyed, it's
restored to the original value ("old_unpriv_sgio").
---
src/conf/domain_conf.h | 1 +
src/qemu/qemu_process.c | 19 ++++++++++---------
2 files changed, 11 insertions(+), 9 deletions(-)
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index 105fb7d..1a8de71 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -616,6 +616,7 @@ struct _virDomainDiskDef {
bool rawio_specified;
int rawio; /* no = 0, yes = 1 */
int unpriv_sgio; /* no = 0, yes = 1 */
+ int old_unpriv_sgio; /* To record the old unpriv_sgio value, internally */
size_t nseclabels;
virSecurityDeviceLabelDefPtr *seclabels;
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index 98096c5..e48eed0 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -3706,19 +3706,20 @@ int qemuProcessStart(virConnectPtr conn,
/* Set unpriv_sgio for disks */
for (i = 0; i < vm->def->ndisks; i++) {
virDomainDiskDefPtr disk = vm->def->disks[i];
- int old_sgio;
+ int old_unpriv_sgio;
- if (!disk->sgio)
+ if (!disk->unpriv_sgio)
continue;
- if (virGetDeviceSGIO(disk->src, &old_sgio) < 0)
+ if (virGetDeviceUnprivSGIO(disk->src, &old_unpriv_sgio) < 0)
goto cleanup;
- disk->old_sgio = old_sgio;
+ disk->old_unpriv_sgio = old_unpriv_sgio;
- if (virSetDeviceSGIO(disk->src,
- (disk->sgio == VIR_DOMAIN_DISK_SGIO_YES)
- ? 1 : 0) < 0)
+ if (virSetDeviceUnprivSGIO(disk->src,
+ (disk->unpriv_sgio ==
+ VIR_DOMAIN_DISK_UNPRIV_SGIO_YES)
+ ? 1 : 0) < 0)
goto cleanup;
}
@@ -4116,10 +4117,10 @@ void qemuProcessStop(struct qemud_driver *driver,
for (i = 0; i < vm->def->ndisks; i++) {
virDomainDiskDefPtr disk = vm->def->disks[i];
- if (!disk->sgio)
+ if (!disk->unpriv_sgio)
continue;
- if (virSetDeviceSGIO(disk->src, disk->old_sgio) < 0)
+ if (virSetDeviceUnprivSGIO(disk->src, disk->old_unpriv_sgio) < 0)
VIR_WARN("Unable to restore unpriv_sgio for disk '%s'",
disk->src);
}
--
1.7.7.6
7:21 p.m.
New subject: [libvirt] [PATCH 5/6] unpriv_sgio: Do not restore unpriv_sgio if the disk is being used
This prevents restoring the unpriv_sgio if the disk is shared,
and is being used by other active domain. Because we don't want
to fall into the corruption situation.
* src/conf/domain_conf.h (Declare virDomainDiskIsUsed, which is to
detect if a disk is using by domain)
* src/conf/domain_conf.c (Implement virDomainDiskIsUsed)
* src/libvirt_private.syms: (Export virDomainDIskIsUsed)
* src/qemu/qemu_process.c (Don't restore unpriv_sgio if the disk
is shared, and is being used by other
active domain).
---
src/conf/domain_conf.c | 66 ++++++++++++++++++++++++++++++++++++++++++++++
src/conf/domain_conf.h | 4 +++
src/libvirt_private.syms | 1 +
src/qemu/qemu_process.c | 11 +++++++
4 files changed, 82 insertions(+), 0 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index b25229a..02df96e 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -15697,3 +15697,69 @@ virDomainDefAddSecurityLabelDef(virDomainDefPtr def, const char
*model)
return seclabel;
}
+
+struct virDomainDiskIsUsedData {
+ char *diskSrc;
+ char *name; /* Want to exclude some domain? */
+ bool active; /* Want to only iterate the active domains? */
+};
+
+static int
+virDomainObjListSearchDiskPath(const void *payload,
+ const void *name ATTRIBUTE_UNUSED,
+ const void *opaque)
+{
+ virDomainObjPtr obj = (virDomainObjPtr)payload;
+ const struct virDomainDiskIsUsedData *data = opaque;
+ int i;
+ int ret = 0;
+
+ virDomainObjLock(obj);
+
+ if (data->active &&
+ !virDomainObjIsActive(obj))
+ goto cleanup;
+
+ if (data->name &&
+ STREQ(data->name, obj->def->name))
+ goto cleanup;
+
+ for (i = 0; i < obj->def->ndisks; i++) {
+ virDomainDiskDefPtr disk = obj->def->disks[i];
+
+ if (STREQ(disk->src, data->diskSrc)) {
+ ret = 1;
+ goto cleanup;
+ }
+ }
+
+cleanup:
+ virDomainObjUnlock(obj);
+ return ret;
+}
+
+/**
+ * virDomainDiskIsUsed:
+ * @doms: List of domain objects
+ * @name: The domain name want to exclude
+ * @diskSrc: The disk path
+ * @active: Whether to exclude inactive domains
+ *
+ * Returns true if the disk is being used. Otherwise returns false.
+ */
+bool
+virDomainDiskIsUsed(const virDomainObjList doms,
+ char *name,
+ char *diskSrc,
+ bool active)
+{
+ virDomainObjPtr obj;
+
+ struct virDomainDiskIsUsedData data = { diskSrc, name, active };
+
+ obj = virHashSearch(doms.objs, virDomainObjListSearchDiskPath, &data);
+ if (obj)
+ return true;
+
+ return false;
+}
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index 1a8de71..3105e05 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -1874,6 +1874,10 @@ virDomainObjPtr virDomainFindByUUID(const virDomainObjListPtr
doms,
const unsigned char *uuid);
virDomainObjPtr virDomainFindByName(const virDomainObjListPtr doms,
const char *name);
+bool virDomainDiskIsUsed(const virDomainObjList doms,
+ char *name,
+ char *diskSrc,
+ bool active);
bool virDomainObjTaint(virDomainObjPtr obj,
enum virDomainTaintFlags taint);
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index c756130..b9019b7 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -319,6 +319,7 @@ virDomainDefFormatInternal;
virDomainDefFree;
virDomainDefGetSecurityLabelDef;
virDomainDiskDefGetSecurityLabelDef;
+virDomainDiskIsUsed;
virDomainDefAddSecurityLabelDef;
virDomainDefParseFile;
virDomainDefParseNode;
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index e48eed0..eeaaea0 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -4120,6 +4120,17 @@ void qemuProcessStop(struct qemud_driver *driver,
if (!disk->unpriv_sgio)
continue;
+ /* Don't try to restore the unpriv_sgio if the disk is shared
+ * by other active domain(s). We don't want to fall into the
+ * corruptions.
+ */
+ if (disk->shared &&
+ virDomainDiskIsUsed(driver->domains,
+ vm->def->name,
+ disk->src,
+ true))
+ continue;
+
if (virSetDeviceUnprivSGIO(disk->src, disk->old_unpriv_sgio) < 0)
VIR_WARN("Unable to restore unpriv_sgio for disk '%s'",
disk->src);
}
--
1.7.7.6
7:21 p.m.
New subject: [libvirt] [PATCH 6/6] unpriv_sgio: Error out if the unpriv_sgio setting conflicts with others
This prevent the domain starting if the shared disk's setting conflicts
with other active domain(s), E.g. A domain with unpriv_sgio set as
"yes", however, another active domain is using it set as "no".
---
src/conf/domain_conf.c | 45 +++++++++++++++++++++++++++++++++++++++++++++
src/conf/domain_conf.h | 3 +++
src/libvirt_private.syms | 1 +
src/qemu/qemu_process.c | 14 +++++++++++++-
4 files changed, 62 insertions(+), 1 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 02df96e..fb7bb1f 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -15763,3 +15763,48 @@ virDomainDiskIsUsed(const virDomainObjList doms,
return false;
}
+
+static int
+virDomainSharedDiskCompare(const void *payload,
+ const void *name ATTRIBUTE_UNUSED,
+ const void *opaque)
+{
+ virDomainObjPtr obj = (virDomainObjPtr)payload;
+ const virDomainDiskDefPtr disk = (virDomainDiskDefPtr)opaque;
+ int i;
+
+ virDomainObjLock(obj);
+
+ /* Ingores the inactive ones */
+ if (!virDomainObjIsActive(obj))
+ return 0;
+
+ for (i = 0; i < obj->def->ndisks; i++) {
+ if (STRNEQ(obj->def->disks[i]->src, disk->src))
+ continue;
+
+ if (obj->def->disks[i]->unpriv_sgio != disk->unpriv_sgio) {
+ virDomainObjUnlock(obj);
+ return 1;
+ }
+ }
+
+ virDomainObjUnlock(obj);
+ return 0;
+}
+
+/* Validate if the shared disk conf is conflicted with other domains,
+ * currently only validates the unpriv_sgio setting
+ */
+bool
+virDomainSharedDiskValidate(const virDomainObjList doms,
+ const virDomainDiskDefPtr disk)
+{
+ virDomainObjPtr obj;
+
+ obj = virHashSearch(doms.objs, virDomainSharedDiskCompare, disk);
+ if (obj)
+ return false;
+
+ return true;
+}
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index 3105e05..35f46d2 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -1878,6 +1878,9 @@ bool virDomainDiskIsUsed(const virDomainObjList doms,
char *name,
char *diskSrc,
bool active);
+bool
+virDomainSharedDiskValidate(const virDomainObjList doms,
+ const virDomainDiskDefPtr disk);
bool virDomainObjTaint(virDomainObjPtr obj,
enum virDomainTaintFlags taint);
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index b9019b7..b14eb40 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -489,6 +489,7 @@ virDomainSaveStatus;
virDomainSaveXML;
virDomainSeclabelTypeFromString;
virDomainSeclabelTypeToString;
+virDomainSharedDiskValidate;
virDomainShutdownReasonTypeFromString;
virDomainShutdownReasonTypeToString;
virDomainShutoffReasonTypeFromString;
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index eeaaea0..b5676e0 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -3711,6 +3711,18 @@ int qemuProcessStart(virConnectPtr conn,
if (!disk->unpriv_sgio)
continue;
+ /* Error out if the disk is shared, but the unpriv_sgio conflicts
+ * with other active domain(s).
+ */
+ if (disk->shared &&
+ !virDomainSharedDiskValidate(driver->domains,
+ disk)) {
+ virReportError(VIR_ERR_XML_ERROR,
+ _("Setting of shared disk '%s' conflicts with
"
+ "other active domains"), disk->src);
+ goto cleanup;
+ }
+
if (virGetDeviceUnprivSGIO(disk->src, &old_unpriv_sgio) < 0)
goto cleanup;
@@ -3719,7 +3731,7 @@ int qemuProcessStart(virConnectPtr conn,
if (virSetDeviceUnprivSGIO(disk->src,
(disk->unpriv_sgio ==
VIR_DOMAIN_DISK_UNPRIV_SGIO_YES)
- ? 1 : 0) < 0)
+ ? 1 : 0) < 0)
goto cleanup;
}
--
1.7.7.6
7:40 p.m.
New subject: [libvirt] [PATCH 6/6] unpriv_sgio: Error out if the unpriv_sgio setting conflicts with others
On Mon, Nov 26, 2012 at 06:21:42PM +0800, Osier Yang wrote:
This code is rather undesirable since it requires locking every
single VM known to the QEMU driver. Better to use the model we
have for PCI/USB devices where the driver maintains a central
list of the devices
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 :|
4517
days inactive
4518
days old
13 comments
3 participants
participants (3)
-
Daniel P. Berrange -
Osier Yang -
Paolo Bonzini