[libvirt] [PATCH v3 libvirt 0/3] Add support for qemu usb-mtp device
This series adds support for qemu usb-mtp devices. This new version addresses comments for v2: https://www.redhat.com/archives/libvir-list/2014-August/msg00471.html Giuseppe Scrivano (3): conf: add <model> child element to <filesystem> docs: relax <filesystem>/<target> dir element attribute to string qemu: add support for MTP filesystem docs/formatdomain.html.in | 6 +++ docs/schemas/domaincommon.rng | 17 +++++-- src/conf/domain_conf.c | 26 +++++++++++ src/conf/domain_conf.h | 11 +++++ src/qemu/qemu_command.c | 65 ++++++++++++++++---------- tests/domainconfdata/getfilesystem.xml | 5 ++ tests/domainconftest.c | 1 + tests/qemuxml2argvdata/qemuxml2argv-fsmtp.args | 6 +++ tests/qemuxml2argvdata/qemuxml2argv-fsmtp.xml | 30 ++++++++++++ 9 files changed, 140 insertions(+), 27 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-fsmtp.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-fsmtp.xml -- 1.9.3
Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com> --- docs/formatdomain.html.in | 6 ++++++ docs/schemas/domaincommon.rng | 13 +++++++++++++ src/conf/domain_conf.c | 26 ++++++++++++++++++++++++++ src/conf/domain_conf.h | 11 +++++++++++ tests/domainconfdata/getfilesystem.xml | 5 +++++ tests/domainconftest.c | 1 + 6 files changed, 62 insertions(+) diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index ed17389..d0c1ce2 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -2300,6 +2300,8 @@ <driver type='path' wrpolicy='immediate'/> <source dir='/export/to/guest'/> <target dir='/import/from/host'/> + <model type='9p'/> + <model type='mtp'/> <readonly/> </filesystem> <filesystem type='file' accessmode='passthrough'> @@ -2337,6 +2339,10 @@ while the value <code>immediate</code> means that a host writeback is immediately triggered for all pages touched during a guest file write operation <span class="since">(since 0.9.10)</span>. + A "filesystem" element has an optional + attribute <code>model</code><span class="since"> (since + 1.2.8)</span>, which is one of "9p", "mtp" (used by QEMU/KVM), + if this element is not specified the default is "9p". </dd> <dt><code>type='template'</code></dt> <dd> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index 033f2f6..684acec 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -1937,6 +1937,19 @@ </element> </optional> </interleave> + <interleave> + <optional> + <element name="model"> + <attribute name="type"> + <choice> + <value>9p</value> + <value>mtp</value> + </choice> + </attribute> + <empty/> + </element> + </optional> + </interleave> </element> </define> <define name="fsDriver"> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 14338ba..98dbe14 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -344,6 +344,11 @@ VIR_ENUM_IMPL(virDomainFS, VIR_DOMAIN_FS_TYPE_LAST, "ram", "bind") +VIR_ENUM_IMPL(virDomainFSModel, VIR_DOMAIN_FS_MODEL_LAST, + "default", + "9p", + "mtp") + VIR_ENUM_IMPL(virDomainFSDriver, VIR_DOMAIN_FS_DRIVER_TYPE_LAST, "default", "path", @@ -6459,6 +6464,7 @@ virDomainFSDefParseXML(xmlNodePtr node, virDomainFSDefPtr def; xmlNodePtr cur, save_node = ctxt->node; char *type = NULL; + char *model = NULL; char *fsdriver = NULL; char *source = NULL; char *target = NULL; @@ -6536,6 +6542,9 @@ virDomainFSDefParseXML(xmlNodePtr node, wrpolicy = virXMLPropString(cur, "wrpolicy"); if (!format) format = virXMLPropString(cur, "format"); + } else if (!model && + xmlStrEqual(cur->name, BAD_CAST "model")) { + model = virXMLPropString(cur, "type"); } } cur = cur->next; @@ -6557,6 +6566,14 @@ virDomainFSDefParseXML(xmlNodePtr node, } } + if (model) { + if ((def->model = virDomainFSModelTypeFromString(model)) <= 0) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("unknown model value '%s'"), model); + goto error; + } + } + if (wrpolicy) { if ((def->wrpolicy = virDomainFSWrpolicyTypeFromString(wrpolicy)) <= 0) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, @@ -6616,6 +6633,7 @@ virDomainFSDefParseXML(xmlNodePtr node, VIR_FREE(usage); VIR_FREE(units); VIR_FREE(format); + VIR_FREE(model); return def; @@ -15786,6 +15804,14 @@ virDomainFSDefFormat(virBufferPtr buf, switch (def->type) { case VIR_DOMAIN_FS_TYPE_MOUNT: + virBufferEscapeString(buf, "<source dir='%s'/>\n", + def->src); + if (def->model) { + virBufferEscapeString(buf, "<model type='%s'/>\n", + virDomainFSModelTypeToString(def->model)); + } + break; + case VIR_DOMAIN_FS_TYPE_BIND: virBufferEscapeString(buf, "<source dir='%s'/>\n", def->src); diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 4cf56c9..df7d019 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -764,6 +764,15 @@ typedef enum { VIR_DOMAIN_FS_TYPE_LAST } virDomainFSType; +/* Filesystem model */ +typedef enum { + VIR_DOMAIN_FS_MODEL_DEFAULT = 0, + VIR_DOMAIN_FS_MODEL_9P, /* 9P network filesystem */ + VIR_DOMAIN_FS_MODEL_MTP, /* MTP usb filesystem */ + + VIR_DOMAIN_FS_MODEL_LAST +} virDomainFSModel; + /* Filesystem driver type */ typedef enum { VIR_DOMAIN_FS_DRIVER_TYPE_DEFAULT = 0, @@ -808,6 +817,7 @@ struct _virDomainFSDef { virDomainDeviceInfo info; unsigned long long space_hard_limit; /* in bytes */ unsigned long long space_soft_limit; /* in bytes */ + int model; /* enum virDomainFSModel */ }; @@ -2585,6 +2595,7 @@ VIR_ENUM_DECL(virDomainControllerModelPCI) VIR_ENUM_DECL(virDomainControllerModelSCSI) VIR_ENUM_DECL(virDomainControllerModelUSB) VIR_ENUM_DECL(virDomainFS) +VIR_ENUM_DECL(virDomainFSModel) VIR_ENUM_DECL(virDomainFSDriver) VIR_ENUM_DECL(virDomainFSAccessMode) VIR_ENUM_DECL(virDomainFSWrpolicy) diff --git a/tests/domainconfdata/getfilesystem.xml b/tests/domainconfdata/getfilesystem.xml index 2ee78b4..3203666 100644 --- a/tests/domainconfdata/getfilesystem.xml +++ b/tests/domainconfdata/getfilesystem.xml @@ -21,6 +21,11 @@ <source dir='/'/> <target dir='/dev'/> </filesystem> + <filesystem type='mount'> + <model type="mtp"/> + <source dir='/'/> + <target dir='mtp share'/> + </filesystem> <console type='pty'> <target type='lxc' port='0'/> </console> diff --git a/tests/domainconftest.c b/tests/domainconftest.c index 3d6ebe1..6b65f09 100644 --- a/tests/domainconftest.c +++ b/tests/domainconftest.c @@ -111,6 +111,7 @@ mymain(void) DO_TEST_GET_FS("/dev", true); DO_TEST_GET_FS("/dev/pts", false); DO_TEST_GET_FS("/doesnotexist", false); + DO_TEST_GET_FS("mtp share", true); virObjectUnref(caps); virObjectUnref(xmlopt); -- 1.9.3
On Tue, Aug 19, 2014 at 10:11:35PM +0200, Giuseppe Scrivano wrote:
Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com> --- docs/formatdomain.html.in | 6 ++++++ docs/schemas/domaincommon.rng | 13 +++++++++++++ src/conf/domain_conf.c | 26 ++++++++++++++++++++++++++ src/conf/domain_conf.h | 11 +++++++++++ tests/domainconfdata/getfilesystem.xml | 5 +++++ tests/domainconftest.c | 1 + 6 files changed, 62 insertions(+)
diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index ed17389..d0c1ce2 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -2300,6 +2300,8 @@ <driver type='path' wrpolicy='immediate'/> <source dir='/export/to/guest'/> <target dir='/import/from/host'/> + <model type='9p'/> + <model type='mtp'/> <readonly/> </filesystem> <filesystem type='file' accessmode='passthrough'> @@ -2337,6 +2339,10 @@ while the value <code>immediate</code> means that a host writeback is immediately triggered for all pages touched during a guest file write operation <span class="since">(since 0.9.10)</span>. + A "filesystem" element has an optional + attribute <code>model</code><span class="since"> (since + 1.2.8)</span>, which is one of "9p", "mtp" (used by QEMU/KVM), + if this element is not specified the default is "9p". </dd> <dt><code>type='template'</code></dt> <dd> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index 033f2f6..684acec 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -1937,6 +1937,19 @@ </element> </optional> </interleave> + <interleave> + <optional> + <element name="model"> + <attribute name="type"> + <choice> + <value>9p</value> + <value>mtp</value> + </choice> + </attribute> + <empty/> + </element> + </optional> + </interleave> </element> </define> <define name="fsDriver"> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 14338ba..98dbe14 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -344,6 +344,11 @@ VIR_ENUM_IMPL(virDomainFS, VIR_DOMAIN_FS_TYPE_LAST, "ram", "bind")
+VIR_ENUM_IMPL(virDomainFSModel, VIR_DOMAIN_FS_MODEL_LAST, + "default", + "9p", + "mtp") + VIR_ENUM_IMPL(virDomainFSDriver, VIR_DOMAIN_FS_DRIVER_TYPE_LAST, "default", "path", @@ -6459,6 +6464,7 @@ virDomainFSDefParseXML(xmlNodePtr node, virDomainFSDefPtr def; xmlNodePtr cur, save_node = ctxt->node; char *type = NULL; + char *model = NULL; char *fsdriver = NULL; char *source = NULL; char *target = NULL; @@ -6536,6 +6542,9 @@ virDomainFSDefParseXML(xmlNodePtr node, wrpolicy = virXMLPropString(cur, "wrpolicy"); if (!format) format = virXMLPropString(cur, "format"); + } else if (!model && + xmlStrEqual(cur->name, BAD_CAST "model")) { + model = virXMLPropString(cur, "type"); } } cur = cur->next; @@ -6557,6 +6566,14 @@ virDomainFSDefParseXML(xmlNodePtr node, } }
+ if (model) { + if ((def->model = virDomainFSModelTypeFromString(model)) <= 0) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("unknown model value '%s'"), model); + goto error; + } + } + if (wrpolicy) { if ((def->wrpolicy = virDomainFSWrpolicyTypeFromString(wrpolicy)) <= 0) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, @@ -6616,6 +6633,7 @@ virDomainFSDefParseXML(xmlNodePtr node, VIR_FREE(usage); VIR_FREE(units); VIR_FREE(format); + VIR_FREE(model);
return def;
@@ -15786,6 +15804,14 @@ virDomainFSDefFormat(virBufferPtr buf,
switch (def->type) { case VIR_DOMAIN_FS_TYPE_MOUNT: + virBufferEscapeString(buf, "<source dir='%s'/>\n", + def->src); + if (def->model) { + virBufferEscapeString(buf, "<model type='%s'/>\n", + virDomainFSModelTypeToString(def->model)); + }
From the XML pov, we shouldn't restrict use of <model> to only be for FS_TYPE_MOUNT. That is a QEMU driver implementation restriction, so just make the QEMU driver raise VIR_ERR_CONFIG_UNSUPPORTED for the cases we don't want, when building the CLI args
+ break; + case VIR_DOMAIN_FS_TYPE_BIND: virBufferEscapeString(buf, "<source dir='%s'/>\n", def->src);
Regards, 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 :|
"Daniel P. Berrange" <berrange@redhat.com> writes:
From the XML pov, we shouldn't restrict use of <model> to only be for FS_TYPE_MOUNT. That is a QEMU driver implementation restriction, so just make the QEMU driver raise VIR_ERR_CONFIG_UNSUPPORTED for the cases we don't want, when building the CLI args
OK to amend this piece? The QEMU driver supports only FS_TYPE_MOUNT so the additional check seems reduntant there. diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 98dbe14..c897f1b 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -15806,10 +15806,6 @@ virDomainFSDefFormat(virBufferPtr buf, case VIR_DOMAIN_FS_TYPE_MOUNT: virBufferEscapeString(buf, "<source dir='%s'/>\n", def->src); - if (def->model) { - virBufferEscapeString(buf, "<model type='%s'/>\n", - virDomainFSModelTypeToString(def->model)); - } break; case VIR_DOMAIN_FS_TYPE_BIND: @@ -15838,6 +15834,11 @@ virDomainFSDefFormat(virBufferPtr buf, break; } + if (def->model) { + virBufferEscapeString(buf, "<model type='%s'/>\n", + virDomainFSModelTypeToString(def->model)); + } + virBufferEscapeString(buf, "<target dir='%s'/>\n", def->dst); Thanks, Giuseppe
On Wed, Aug 20, 2014 at 11:21:24AM +0200, Giuseppe Scrivano wrote:
"Daniel P. Berrange" <berrange@redhat.com> writes:
From the XML pov, we shouldn't restrict use of <model> to only be for FS_TYPE_MOUNT. That is a QEMU driver implementation restriction, so just make the QEMU driver raise VIR_ERR_CONFIG_UNSUPPORTED for the cases we don't want, when building the CLI args
OK to amend this piece? The QEMU driver supports only FS_TYPE_MOUNT so the additional check seems reduntant there.
Yes, I see QEMU driver already has sufficient checks for this.
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 98dbe14..c897f1b 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -15806,10 +15806,6 @@ virDomainFSDefFormat(virBufferPtr buf, case VIR_DOMAIN_FS_TYPE_MOUNT: virBufferEscapeString(buf, "<source dir='%s'/>\n", def->src); - if (def->model) { - virBufferEscapeString(buf, "<model type='%s'/>\n", - virDomainFSModelTypeToString(def->model)); - } break;
case VIR_DOMAIN_FS_TYPE_BIND: @@ -15838,6 +15834,11 @@ virDomainFSDefFormat(virBufferPtr buf, break; }
+ if (def->model) { + virBufferEscapeString(buf, "<model type='%s'/>\n", + virDomainFSModelTypeToString(def->model)); + } + virBufferEscapeString(buf, "<target dir='%s'/>\n", def->dst);
ACK to the whole patch with this applied. Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On 19.08.2014 22:11, Giuseppe Scrivano wrote:
Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com> --- docs/formatdomain.html.in | 6 ++++++ docs/schemas/domaincommon.rng | 13 +++++++++++++ src/conf/domain_conf.c | 26 ++++++++++++++++++++++++++ src/conf/domain_conf.h | 11 +++++++++++ tests/domainconfdata/getfilesystem.xml | 5 +++++ tests/domainconftest.c | 1 + 6 files changed, 62 insertions(+)
diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index ed17389..d0c1ce2 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -2300,6 +2300,8 @@ <driver type='path' wrpolicy='immediate'/> <source dir='/export/to/guest'/> <target dir='/import/from/host'/> + <model type='9p'/> + <model type='mtp'/>
No. Having two <model/> elements is not supported. Just choose one and describe other in the paragraph below (like you did).
<readonly/> </filesystem> <filesystem type='file' accessmode='passthrough'> @@ -2337,6 +2339,10 @@ while the value <code>immediate</code> means that a host writeback is immediately triggered for all pages touched during a guest file write operation <span class="since">(since 0.9.10)</span>. + A "filesystem" element has an optional + attribute <code>model</code><span class="since"> (since + 1.2.8)</span>, which is one of "9p", "mtp" (used by QEMU/KVM), + if this element is not specified the default is "9p". </dd> <dt><code>type='template'</code></dt> <dd> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index 033f2f6..684acec 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -1937,6 +1937,19 @@ </element> </optional> </interleave> + <interleave> + <optional> + <element name="model"> + <attribute name="type"> + <choice> + <value>9p</value> + <value>mtp</value> + </choice> + </attribute> + <empty/> + </element> + </optional> + </interleave> </element> </define> <define name="fsDriver"> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 14338ba..98dbe14 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -344,6 +344,11 @@ VIR_ENUM_IMPL(virDomainFS, VIR_DOMAIN_FS_TYPE_LAST, "ram", "bind")
+VIR_ENUM_IMPL(virDomainFSModel, VIR_DOMAIN_FS_MODEL_LAST, + "default", + "9p", + "mtp") + VIR_ENUM_IMPL(virDomainFSDriver, VIR_DOMAIN_FS_DRIVER_TYPE_LAST, "default", "path", @@ -6459,6 +6464,7 @@ virDomainFSDefParseXML(xmlNodePtr node, virDomainFSDefPtr def; xmlNodePtr cur, save_node = ctxt->node; char *type = NULL; + char *model = NULL; char *fsdriver = NULL; char *source = NULL; char *target = NULL; @@ -6536,6 +6542,9 @@ virDomainFSDefParseXML(xmlNodePtr node, wrpolicy = virXMLPropString(cur, "wrpolicy"); if (!format) format = virXMLPropString(cur, "format"); + } else if (!model && + xmlStrEqual(cur->name, BAD_CAST "model")) { + model = virXMLPropString(cur, "type"); } } cur = cur->next; @@ -6557,6 +6566,14 @@ virDomainFSDefParseXML(xmlNodePtr node, } }
+ if (model) { + if ((def->model = virDomainFSModelTypeFromString(model)) <= 0) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("unknown model value '%s'"), model); + goto error; + } + } + if (wrpolicy) { if ((def->wrpolicy = virDomainFSWrpolicyTypeFromString(wrpolicy)) <= 0) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, @@ -6616,6 +6633,7 @@ virDomainFSDefParseXML(xmlNodePtr node, VIR_FREE(usage); VIR_FREE(units); VIR_FREE(format); + VIR_FREE(model);
Maybe a nitpick, but I'd love to keep an ordering here. I see two possible ways of doing free over multiple variables. Either the same oder in which they are declared, or the reverse one. So this VIR_FREE() needs to go in between VIR_FREE(type) and VIR_FREE(fsdriver).
return def;
@@ -15786,6 +15804,14 @@ virDomainFSDefFormat(virBufferPtr buf,
switch (def->type) { case VIR_DOMAIN_FS_TYPE_MOUNT: + virBufferEscapeString(buf, "<source dir='%s'/>\n", + def->src); + if (def->model) { + virBufferEscapeString(buf, "<model type='%s'/>\n", + virDomainFSModelTypeToString(def->model)); + } + break; + case VIR_DOMAIN_FS_TYPE_BIND: virBufferEscapeString(buf, "<source dir='%s'/>\n", def->src); diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 4cf56c9..df7d019 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -764,6 +764,15 @@ typedef enum { VIR_DOMAIN_FS_TYPE_LAST } virDomainFSType;
+/* Filesystem model */ +typedef enum { + VIR_DOMAIN_FS_MODEL_DEFAULT = 0, + VIR_DOMAIN_FS_MODEL_9P, /* 9P network filesystem */ + VIR_DOMAIN_FS_MODEL_MTP, /* MTP usb filesystem */ + + VIR_DOMAIN_FS_MODEL_LAST +} virDomainFSModel; + /* Filesystem driver type */ typedef enum { VIR_DOMAIN_FS_DRIVER_TYPE_DEFAULT = 0, @@ -808,6 +817,7 @@ struct _virDomainFSDef { virDomainDeviceInfo info; unsigned long long space_hard_limit; /* in bytes */ unsigned long long space_soft_limit; /* in bytes */ + int model; /* enum virDomainFSModel */ };
@@ -2585,6 +2595,7 @@ VIR_ENUM_DECL(virDomainControllerModelPCI) VIR_ENUM_DECL(virDomainControllerModelSCSI) VIR_ENUM_DECL(virDomainControllerModelUSB) VIR_ENUM_DECL(virDomainFS) +VIR_ENUM_DECL(virDomainFSModel) VIR_ENUM_DECL(virDomainFSDriver) VIR_ENUM_DECL(virDomainFSAccessMode) VIR_ENUM_DECL(virDomainFSWrpolicy) diff --git a/tests/domainconfdata/getfilesystem.xml b/tests/domainconfdata/getfilesystem.xml index 2ee78b4..3203666 100644 --- a/tests/domainconfdata/getfilesystem.xml +++ b/tests/domainconfdata/getfilesystem.xml @@ -21,6 +21,11 @@ <source dir='/'/> <target dir='/dev'/> </filesystem> + <filesystem type='mount'> + <model type="mtp"/> + <source dir='/'/> + <target dir='mtp share'/> + </filesystem> <console type='pty'> <target type='lxc' port='0'/> </console> diff --git a/tests/domainconftest.c b/tests/domainconftest.c index 3d6ebe1..6b65f09 100644 --- a/tests/domainconftest.c +++ b/tests/domainconftest.c @@ -111,6 +111,7 @@ mymain(void) DO_TEST_GET_FS("/dev", true); DO_TEST_GET_FS("/dev/pts", false); DO_TEST_GET_FS("/doesnotexist", false); + DO_TEST_GET_FS("mtp share", true);
virObjectUnref(caps); virObjectUnref(xmlopt);
ACK with this and nits pointed out by Daniel fixed. Michal
It is used by mtp model type to specify the mtp share name. Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com> --- docs/schemas/domaincommon.rng | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index 684acec..4178644 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -1899,9 +1899,7 @@ </choice> <interleave> <element name="target"> - <attribute name="dir"> - <ref name="absDirPath"/> - </attribute> + <attribute name="dir"/> <empty/> </element> <optional> -- 1.9.3
On 19.08.2014 22:11, Giuseppe Scrivano wrote:
It is used by mtp model type to specify the mtp share name.
Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com> --- docs/schemas/domaincommon.rng | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index 684acec..4178644 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -1899,9 +1899,7 @@ </choice> <interleave> <element name="target"> - <attribute name="dir"> - <ref name="absDirPath"/> - </attribute> + <attribute name="dir"/> <empty/> </element> <optional>
See my comment to 3/3 Michal
Generate the qemu command line option: -device 'usb-mtp,root=$SRC,desc=$TARGET' from the definition XML: <filesystem type='mount'> <source dir='$SRC'/> <target dir='$TARGET'/> <model type='mtp'/> </filesystem> Closes: https://bugzilla.redhat.com/show_bug.cgi?id=1121781 Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com> --- src/qemu/qemu_command.c | 65 ++++++++++++++++---------- tests/qemuxml2argvdata/qemuxml2argv-fsmtp.args | 6 +++ tests/qemuxml2argvdata/qemuxml2argv-fsmtp.xml | 30 ++++++++++++ 3 files changed, 77 insertions(+), 24 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-fsmtp.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-fsmtp.xml diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index b68695d..850fd71 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -2060,8 +2060,9 @@ qemuAssignDevicePCISlots(virDomainDefPtr def, if (def->fss[i]->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE) continue; - /* Only support VirtIO-9p-pci so far. If that changes, - * we might need to skip devices here */ + if (def->fss[i]->model == VIR_DOMAIN_FS_MODEL_MTP) + continue; + if (virDomainPCIAddressReserveNextSlot(addrs, &def->fss[i]->info, flags) < 0) goto error; @@ -3956,12 +3957,6 @@ char *qemuBuildFSStr(virDomainFSDefPtr fs, const char *driver = qemuDomainFSDriverTypeToString(fs->fsdriver); const char *wrpolicy = virDomainFSWrpolicyTypeToString(fs->wrpolicy); - if (fs->type != VIR_DOMAIN_FS_TYPE_MOUNT) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("only supports mount filesystem type")); - goto error; - } - if (!driver) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("Filesystem driver type not supported")); @@ -4030,22 +4025,36 @@ qemuBuildFSDevStr(virDomainDefPtr def, { virBuffer opt = VIR_BUFFER_INITIALIZER; - if (fs->type != VIR_DOMAIN_FS_TYPE_MOUNT) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("can only passthrough directories")); - goto error; - } + if (fs->type == VIR_DOMAIN_FS_TYPE_MOUNT) { - virBufferAddLit(&opt, "virtio-9p-pci"); - virBufferAsprintf(&opt, ",id=%s", fs->info.alias); - virBufferAsprintf(&opt, ",fsdev=%s%s", QEMU_FSDEV_HOST_PREFIX, fs->info.alias); - virBufferAsprintf(&opt, ",mount_tag=%s", fs->dst); + switch ((virDomainFSModel) fs->model) { + case VIR_DOMAIN_FS_MODEL_MTP: + virBufferAddLit(&opt, "usb-mtp"); + virBufferAsprintf(&opt, ",root=%s,desc=%s", fs->src, fs->dst); + break; + case VIR_DOMAIN_FS_MODEL_DEFAULT: + case VIR_DOMAIN_FS_MODEL_9P: + virBufferAddLit(&opt, "virtio-9p-pci"); + virBufferAsprintf(&opt, ",id=%s", fs->info.alias); + virBufferAsprintf(&opt, ",fsdev=%s%s", QEMU_FSDEV_HOST_PREFIX, + fs->info.alias); + virBufferAsprintf(&opt, ",mount_tag=%s", fs->dst); + if (qemuBuildDeviceAddressStr(&opt, def, &fs->info, qemuCaps) < 0) + goto error; + break; - if (qemuBuildDeviceAddressStr(&opt, def, &fs->info, qemuCaps) < 0) - goto error; + case VIR_DOMAIN_FS_MODEL_LAST: + /* nada */ + break; + } - if (virBufferCheckError(&opt) < 0) + if (virBufferCheckError(&opt) < 0) + goto error; + } else { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("unsupported filesystem type")); goto error; + } return virBufferContentAndReset(&opt); @@ -8320,11 +8329,19 @@ qemuBuildCommandLine(virConnectPtr conn, char *optstr; virDomainFSDefPtr fs = def->fss[i]; - virCommandAddArg(cmd, "-fsdev"); - if (!(optstr = qemuBuildFSStr(fs, qemuCaps))) + if (fs->type != VIR_DOMAIN_FS_TYPE_MOUNT) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("only supports mount filesystem type")); goto error; - virCommandAddArg(cmd, optstr); - VIR_FREE(optstr); + } + + if (fs->model != VIR_DOMAIN_FS_MODEL_MTP) { + virCommandAddArg(cmd, "-fsdev"); + if (!(optstr = qemuBuildFSStr(fs, qemuCaps))) + goto error; + virCommandAddArg(cmd, optstr); + VIR_FREE(optstr); + } virCommandAddArg(cmd, "-device"); if (!(optstr = qemuBuildFSDevStr(def, fs, qemuCaps))) diff --git a/tests/qemuxml2argvdata/qemuxml2argv-fsmtp.args b/tests/qemuxml2argvdata/qemuxml2argv-fsmtp.args new file mode 100644 index 0000000..8a1dfeb --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-fsmtp.args @@ -0,0 +1,6 @@ +LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test QEMU_AUDIO_DRV=none \ +/usr/bin/qemu -S -M \ +pc -m 214 -smp 1 -nographic -nodefconfig -nodefaults -monitor \ +unix:/tmp/test-monitor,server,nowait -no-acpi -boot c -usb -hda \ +/dev/HostVG/QEMUGuest1 -device virtio-mtp,root=/tmp/mtp,desc=mtpfs \ +-device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x6 diff --git a/tests/qemuxml2argvdata/qemuxml2argv-fsmtp.xml b/tests/qemuxml2argvdata/qemuxml2argv-fsmtp.xml new file mode 100644 index 0000000..e811183 --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-fsmtp.xml @@ -0,0 +1,30 @@ +<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='disk'> + <source dev='/dev/HostVG/QEMUGuest1'/> + <target dev='hda' bus='ide'/> + <address type='drive' controller='0' bus='0' target='0' unit='0'/> + </disk> + <controller type='usb' index='0'/> + <controller type='ide' index='0'/> + <filesystem type='mount'> + <source dir='/tmp/mtp'/> + <target dir='mtpfs'/> + <model type='mtp'/> + </filesystem> + </devices> +</domain> -- 1.9.3
On 19.08.2014 22:11, Giuseppe Scrivano wrote:
Generate the qemu command line option:
-device 'usb-mtp,root=$SRC,desc=$TARGET'
from the definition XML:
<filesystem type='mount'> <source dir='$SRC'/> <target dir='$TARGET'/> <model type='mtp'/> </filesystem>
Closes: https://bugzilla.redhat.com/show_bug.cgi?id=1121781
Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com> --- src/qemu/qemu_command.c | 65 ++++++++++++++++---------- tests/qemuxml2argvdata/qemuxml2argv-fsmtp.args | 6 +++ tests/qemuxml2argvdata/qemuxml2argv-fsmtp.xml | 30 ++++++++++++ 3 files changed, 77 insertions(+), 24 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-fsmtp.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-fsmtp.xml
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index b68695d..850fd71 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -2060,8 +2060,9 @@ qemuAssignDevicePCISlots(virDomainDefPtr def, if (def->fss[i]->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE) continue;
- /* Only support VirtIO-9p-pci so far. If that changes, - * we might need to skip devices here */ + if (def->fss[i]->model == VIR_DOMAIN_FS_MODEL_MTP) + continue; + if (virDomainPCIAddressReserveNextSlot(addrs, &def->fss[i]->info, flags) < 0) goto error; @@ -3956,12 +3957,6 @@ char *qemuBuildFSStr(virDomainFSDefPtr fs, const char *driver = qemuDomainFSDriverTypeToString(fs->fsdriver); const char *wrpolicy = virDomainFSWrpolicyTypeToString(fs->wrpolicy);
- if (fs->type != VIR_DOMAIN_FS_TYPE_MOUNT) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("only supports mount filesystem type")); - goto error; - } - if (!driver) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("Filesystem driver type not supported")); @@ -4030,22 +4025,36 @@ qemuBuildFSDevStr(virDomainDefPtr def, { virBuffer opt = VIR_BUFFER_INITIALIZER;
- if (fs->type != VIR_DOMAIN_FS_TYPE_MOUNT) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("can only passthrough directories")); - goto error; - } + if (fs->type == VIR_DOMAIN_FS_TYPE_MOUNT) {
- virBufferAddLit(&opt, "virtio-9p-pci"); - virBufferAsprintf(&opt, ",id=%s", fs->info.alias); - virBufferAsprintf(&opt, ",fsdev=%s%s", QEMU_FSDEV_HOST_PREFIX, fs->info.alias); - virBufferAsprintf(&opt, ",mount_tag=%s", fs->dst); + switch ((virDomainFSModel) fs->model) { + case VIR_DOMAIN_FS_MODEL_MTP: + virBufferAddLit(&opt, "usb-mtp"); + virBufferAsprintf(&opt, ",root=%s,desc=%s", fs->src, fs->dst);
Reading qemu sources the root property is called 'x-root'. And indeed domain fails to start: virsh # start gentoo error: Failed to start domain gentoo error: internal error: early end of file from monitor: possible problem: 2014-08-20T09:24:26.862418Z qemu-system-x86_64: -device usb-mtp,root=/some/path,desc=mtpfs: Property '.root' not found Which makes me wonder more. So me digging through qemu git blame output I've found this qemu commit: commit cf679caf911aa49a25477b3aa20468ee50ed6c89 Author: Gerd Hoffmann <kraxel@redhat.com> AuthorDate: Tue Jul 22 09:30:12 2014 +0200 Commit: Gerd Hoffmann <kraxel@redhat.com> CommitDate: Wed Jul 23 08:55:40 2014 +0200 usb: mtp: tag root property as experimental Reason: we don't want commit to that interface yet. Possibly the implementation will be switched over to use fsdev. Suggested-by: Paolo Bonzini <pbonzini@redhat.com> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com> diff --git a/hw/usb/dev-mtp.c b/hw/usb/dev-mtp.c index 1b51a90..384d4a5 100644 --- a/hw/usb/dev-mtp.c +++ b/hw/usb/dev-mtp.c @@ -1090,7 +1090,7 @@ static const VMStateDescription vmstate_usb_mtp = { }; static Property mtp_properties[] = { - DEFINE_PROP_STRING("root", MTPState, root), + DEFINE_PROP_STRING("x-root", MTPState, root), DEFINE_PROP_STRING("desc", MTPState, desc), DEFINE_PROP_END_OF_LIST(), }; Question that pops up immediately: do we want to commit to something that even qemu developers don't believe yet? What will happen when qemu decides to switch to 'root' attribute again? Libvirt will have to adapt which won't work with older qemus supporting 'x-root' only. And what's this desc= attribute for anyway? I've put in several different values and it seems to me like nothing changed. Maybe I don't know enough about MTP, but shouldn't the following be enough? <filesystem type='mount' accessmode='passthrough'> <source dir='/some/path'/> <model type='mtp'/> </filesystem> QEMU (and subsequently guest) knows where to fetch files from.
+ break; + case VIR_DOMAIN_FS_MODEL_DEFAULT: + case VIR_DOMAIN_FS_MODEL_9P: + virBufferAddLit(&opt, "virtio-9p-pci"); + virBufferAsprintf(&opt, ",id=%s", fs->info.alias); + virBufferAsprintf(&opt, ",fsdev=%s%s", QEMU_FSDEV_HOST_PREFIX, + fs->info.alias); + virBufferAsprintf(&opt, ",mount_tag=%s", fs->dst); + if (qemuBuildDeviceAddressStr(&opt, def, &fs->info, qemuCaps) < 0) + goto error; + break;
- if (qemuBuildDeviceAddressStr(&opt, def, &fs->info, qemuCaps) < 0) - goto error; + case VIR_DOMAIN_FS_MODEL_LAST: + /* nada */ + break; + }
- if (virBufferCheckError(&opt) < 0) + if (virBufferCheckError(&opt) < 0) + goto error; + } else { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("unsupported filesystem type")); goto error; + }
return virBufferContentAndReset(&opt);
@@ -8320,11 +8329,19 @@ qemuBuildCommandLine(virConnectPtr conn, char *optstr; virDomainFSDefPtr fs = def->fss[i];
- virCommandAddArg(cmd, "-fsdev"); - if (!(optstr = qemuBuildFSStr(fs, qemuCaps))) + if (fs->type != VIR_DOMAIN_FS_TYPE_MOUNT) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("only supports mount filesystem type")); goto error; - virCommandAddArg(cmd, optstr); - VIR_FREE(optstr); + } + + if (fs->model != VIR_DOMAIN_FS_MODEL_MTP) { + virCommandAddArg(cmd, "-fsdev"); + if (!(optstr = qemuBuildFSStr(fs, qemuCaps))) + goto error; + virCommandAddArg(cmd, optstr); + VIR_FREE(optstr); + }
virCommandAddArg(cmd, "-device"); if (!(optstr = qemuBuildFSDevStr(def, fs, qemuCaps))) diff --git a/tests/qemuxml2argvdata/qemuxml2argv-fsmtp.args b/tests/qemuxml2argvdata/qemuxml2argv-fsmtp.args new file mode 100644 index 0000000..8a1dfeb --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-fsmtp.args @@ -0,0 +1,6 @@ +LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test QEMU_AUDIO_DRV=none \ +/usr/bin/qemu -S -M \ +pc -m 214 -smp 1 -nographic -nodefconfig -nodefaults -monitor \ +unix:/tmp/test-monitor,server,nowait -no-acpi -boot c -usb -hda \ +/dev/HostVG/QEMUGuest1 -device virtio-mtp,root=/tmp/mtp,desc=mtpfs \ +-device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x6 diff --git a/tests/qemuxml2argvdata/qemuxml2argv-fsmtp.xml b/tests/qemuxml2argvdata/qemuxml2argv-fsmtp.xml new file mode 100644 index 0000000..e811183 --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-fsmtp.xml @@ -0,0 +1,30 @@ +<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='disk'> + <source dev='/dev/HostVG/QEMUGuest1'/> + <target dev='hda' bus='ide'/> + <address type='drive' controller='0' bus='0' target='0' unit='0'/> + </disk> + <controller type='usb' index='0'/> + <controller type='ide' index='0'/> + <filesystem type='mount'> + <source dir='/tmp/mtp'/> + <target dir='mtpfs'/> + <model type='mtp'/> + </filesystem> + </devices> +</domain>
Michal
Michal Privoznik <mprivozn@redhat.com> writes:
Question that pops up immediately: do we want to commit to something that even qemu developers don't believe yet? What will happen when qemu decides to switch to 'root' attribute again? Libvirt will have to adapt which won't work with older qemus supporting 'x-root' only.
I am not sure how usually libvirt deals with these cases, but to me it seems like that we should use the "stable" attribute name; or just postpone this series/feature.
And what's this desc= attribute for anyway? I've put in several different values and it seems to me like nothing changed.
it is the name given to the MTP share on the guest, I see it displayed in the guest, at least Nautilus shows it. Regards, Giuseppe
On 08/20/2014 06:19 AM, Giuseppe Scrivano wrote:
Michal Privoznik <mprivozn@redhat.com> writes:
Question that pops up immediately: do we want to commit to something that even qemu developers don't believe yet? What will happen when qemu decides to switch to 'root' attribute again? Libvirt will have to adapt which won't work with older qemus supporting 'x-root' only.
I am not sure how usually libvirt deals with these cases, but to me it seems like that we should use the "stable" attribute name; or just postpone this series/feature.
And what's this desc= attribute for anyway? I've put in several different values and it seems to me like nothing changed.
it is the name given to the MTP share on the guest, I see it displayed in the guest, at least Nautilus shows it.
Is the use of 'root'/'x-root' mandatory? Is it something where we can commit to only a stable subset of the interface, with no way to name the root, and extend it later? -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 20.08.2014 14:41, Eric Blake wrote:
On 08/20/2014 06:19 AM, Giuseppe Scrivano wrote:
Michal Privoznik <mprivozn@redhat.com> writes:
Question that pops up immediately: do we want to commit to something that even qemu developers don't believe yet? What will happen when qemu decides to switch to 'root' attribute again? Libvirt will have to adapt which won't work with older qemus supporting 'x-root' only.
I am not sure how usually libvirt deals with these cases, but to me it seems like that we should use the "stable" attribute name; or just postpone this series/feature.
And what's this desc= attribute for anyway? I've put in several different values and it seems to me like nothing changed.
it is the name given to the MTP share on the guest, I see it displayed in the guest, at least Nautilus shows it.
Is the use of 'root'/'x-root' mandatory? Is it something where we can commit to only a stable subset of the interface, with no way to name the root, and extend it later?
In fact, it's the most interesting attribute as it denotes the path that is to be shared via MTP. Without it, the whole MTP device is useless. Having said that, I vote for postponing this patch until the dust on qemu side settles down. Michal
On 08/20/2014 05:46 AM, Michal Privoznik wrote:
On 19.08.2014 22:11, Giuseppe Scrivano wrote:
Generate the qemu command line option:
-device 'usb-mtp,root=$SRC,desc=$TARGET'
from the definition XML:
<filesystem type='mount'> <source dir='$SRC'/> <target dir='$TARGET'/> <model type='mtp'/> </filesystem>
Closes: https://bugzilla.redhat.com/show_bug.cgi?id=1121781
Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com> ---
+ virBufferAddLit(&opt, "usb-mtp"); + virBufferAsprintf(&opt, ",root=%s,desc=%s", fs->src, fs->dst);
Reading qemu sources the root property is called 'x-root'. And indeed domain fails to start:
Ouch. Qemu has explicitly documented that anything starting with x- is unstable, and may change. We probably should NOT be targetting this in libvirt API just yet, in case qemu changes their mind. It's nice that you've done the patch as a proof of concept, but I don't think we want it in the tree yet :(
usb: mtp: tag root property as experimental
Reason: we don't want commit to that interface yet. Possibly the implementation will be switched over to use fsdev.
Suggested-by: Paolo Bonzini <pbonzini@redhat.com> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
diff --git a/hw/usb/dev-mtp.c b/hw/usb/dev-mtp.c index 1b51a90..384d4a5 100644 --- a/hw/usb/dev-mtp.c +++ b/hw/usb/dev-mtp.c @@ -1090,7 +1090,7 @@ static const VMStateDescription vmstate_usb_mtp = { };
static Property mtp_properties[] = { - DEFINE_PROP_STRING("root", MTPState, root), + DEFINE_PROP_STRING("x-root", MTPState, root), DEFINE_PROP_STRING("desc", MTPState, desc), DEFINE_PROP_END_OF_LIST(), };
Question that pops up immediately: do we want to commit to something that even qemu developers don't believe yet? What will happen when qemu decides to switch to 'root' attribute again? Libvirt will have to adapt which won't work with older qemus supporting 'x-root' only.
Yep - anything in qemu with x- is not a candidate for libvirt support (for the longest time, we refused to support RDMA migration because it still had an x- prefix). -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
participants (4)
-
Daniel P. Berrange -
Eric Blake -
Giuseppe Scrivano -
Michal Privoznik