[libvirt] [PATCH libvirt 0/6] Add support for qemu usb-mtp device
This series adds support for qemu usb-mtp devices. Closes: https://bugzilla.redhat.com/show_bug.cgi?id=1121781 Giuseppe Scrivano (6): conf: add MTP filesystem support to the parser conf, virDomainFSDefPtr: rename "path" argument to "target" qemu_command: fix block indentation qemu: add support for MTP filesystem docs: add documentation and schema for MTP filesystem type tests: add tests for MTP filesystem docs/formatdomain.html.in | 9 +++++ docs/schemas/domaincommon.rng | 15 ++++++-- src/conf/domain_conf.c | 38 +++++++++++++------ src/conf/domain_conf.h | 3 +- src/qemu/qemu_command.c | 67 +++++++++++++++++++--------------- tests/domainconfdata/getfilesystem.xml | 4 ++ tests/domainconftest.c | 2 + 7 files changed, 93 insertions(+), 45 deletions(-) -- 1.9.3
Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com> --- src/conf/domain_conf.c | 34 +++++++++++++++++++++++++--------- src/conf/domain_conf.h | 1 + 2 files changed, 26 insertions(+), 9 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index c25c74b..3bdf46a 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -342,7 +342,8 @@ VIR_ENUM_IMPL(virDomainFS, VIR_DOMAIN_FS_TYPE_LAST, "file", "template", "ram", - "bind") + "bind", + "mtp") VIR_ENUM_IMPL(virDomainFSDriver, VIR_DOMAIN_FS_DRIVER_TYPE_LAST, "default", @@ -6404,7 +6405,8 @@ virDomainFSDefParseXML(xmlNodePtr node, xmlStrEqual(cur->name, BAD_CAST "source")) { if (def->type == VIR_DOMAIN_FS_TYPE_MOUNT || - def->type == VIR_DOMAIN_FS_TYPE_BIND) + def->type == VIR_DOMAIN_FS_TYPE_BIND || + def->type == VIR_DOMAIN_FS_TYPE_MTP) source = virXMLPropString(cur, "dir"); else if (def->type == VIR_DOMAIN_FS_TYPE_FILE) source = virXMLPropString(cur, "file"); @@ -6418,7 +6420,10 @@ virDomainFSDefParseXML(xmlNodePtr node, } } else if (!target && xmlStrEqual(cur->name, BAD_CAST "target")) { - target = virXMLPropString(cur, "dir"); + if (def->type == VIR_DOMAIN_FS_TYPE_MTP) + target = virXMLPropString(cur, "name"); + else + target = virXMLPropString(cur, "dir"); } else if (xmlStrEqual(cur->name, BAD_CAST "readonly")) { def->readonly = true; } else if (xmlStrEqual(cur->name, BAD_CAST "driver")) { @@ -15668,9 +15673,11 @@ virDomainFSDefFormat(virBufferPtr buf, } - virBufferAsprintf(buf, - "<filesystem type='%s' accessmode='%s'>\n", - type, accessmode); + virBufferAsprintf(buf, "<filesystem type='%s'", type); + if (def->type != VIR_DOMAIN_FS_TYPE_MTP) + virBufferAsprintf(buf, " accessmode='%s'", accessmode); + virBufferAddLit(buf, ">\n"); + virBufferAdjustIndent(buf, 2); if (def->fsdriver) { virBufferAsprintf(buf, "<driver type='%s'", fsdriver); @@ -15712,15 +15719,24 @@ virDomainFSDefFormat(virBufferPtr buf, virBufferAsprintf(buf, "<source usage='%lld' units='KiB'/>\n", def->usage / 1024); break; + case VIR_DOMAIN_FS_TYPE_MTP: + virBufferEscapeString(buf, "<source dir='%s'/>\n", + def->src); + break; } - virBufferEscapeString(buf, "<target dir='%s'/>\n", - def->dst); + if (def->type == VIR_DOMAIN_FS_TYPE_MTP) + virBufferEscapeString(buf, "<target name='%s'/>\n", + def->dst); + else + virBufferEscapeString(buf, "<target dir='%s'/>\n", + def->dst); if (def->readonly) virBufferAddLit(buf, "<readonly/>\n"); - if (virDomainDeviceInfoFormat(buf, &def->info, flags) < 0) + if (def->type != VIR_DOMAIN_FS_TYPE_MTP && + virDomainDeviceInfoFormat(buf, &def->info, flags) < 0) return -1; diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index bffc0a5..a4d8a76 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -760,6 +760,7 @@ typedef enum { VIR_DOMAIN_FS_TYPE_TEMPLATE, /* Expands a OS template to a guest dir */ VIR_DOMAIN_FS_TYPE_RAM, /* Mount a RAM filesystem on a guest dir */ VIR_DOMAIN_FS_TYPE_BIND, /* Binds a guest dir to another guest dir */ + VIR_DOMAIN_FS_TYPE_MTP, /* Binds a host dir to a MTP guest device */ VIR_DOMAIN_FS_TYPE_LAST } virDomainFSType; -- 1.9.3
On Thu, Aug 07, 2014 at 04:10:31PM +0200, Giuseppe Scrivano wrote:
Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com> --- src/conf/domain_conf.c | 34 +++++++++++++++++++++++++--------- src/conf/domain_conf.h | 1 + 2 files changed, 26 insertions(+), 9 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index c25c74b..3bdf46a 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -342,7 +342,8 @@ VIR_ENUM_IMPL(virDomainFS, VIR_DOMAIN_FS_TYPE_LAST, "file", "template", "ram", - "bind") + "bind", + "mtp")
I don't think this is the right way to represent it. The 'type' attribute on <filesystem> represents where the backing store for the filesystem comes from. The distinction of 9p vs mtp reflects the type of guest device to expose it as. We shouldn't try to overload these two concepts in the same attribute. We should instead try to add a <device> or <model> child element as we have for some other device types. 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:
On Thu, Aug 07, 2014 at 04:10:31PM +0200, Giuseppe Scrivano wrote:
Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com> --- src/conf/domain_conf.c | 34 +++++++++++++++++++++++++--------- src/conf/domain_conf.h | 1 + 2 files changed, 26 insertions(+), 9 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index c25c74b..3bdf46a 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -342,7 +342,8 @@ VIR_ENUM_IMPL(virDomainFS, VIR_DOMAIN_FS_TYPE_LAST, "file", "template", "ram", - "bind") + "bind", + "mtp")
I don't think this is the right way to represent it.
The 'type' attribute on <filesystem> represents where the backing store for the filesystem comes from.
The distinction of 9p vs mtp reflects the type of guest device to expose it as.
We shouldn't try to overload these two concepts in the same attribute. We should instead try to add a <device> or <model> child element as we have for some other device types.
I see, thanks for the clarification. Would you agree with something like this? <filesystem type='mount'> <device name="mtp share">mtp</device> ... Regards, Giuseppe
On Mon, Aug 11, 2014 at 12:15:41PM +0200, Giuseppe Scrivano wrote:
"Daniel P. Berrange" <berrange@redhat.com> writes:
On Thu, Aug 07, 2014 at 04:10:31PM +0200, Giuseppe Scrivano wrote:
Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com> --- src/conf/domain_conf.c | 34 +++++++++++++++++++++++++--------- src/conf/domain_conf.h | 1 + 2 files changed, 26 insertions(+), 9 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index c25c74b..3bdf46a 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -342,7 +342,8 @@ VIR_ENUM_IMPL(virDomainFS, VIR_DOMAIN_FS_TYPE_LAST, "file", "template", "ram", - "bind") + "bind", + "mtp")
I don't think this is the right way to represent it.
The 'type' attribute on <filesystem> represents where the backing store for the filesystem comes from.
The distinction of 9p vs mtp reflects the type of guest device to expose it as.
We shouldn't try to overload these two concepts in the same attribute. We should instead try to add a <device> or <model> child element as we have for some other device types.
I see, thanks for the clarification.
Would you agree with something like this?
<filesystem type='mount'> <device name="mtp share">mtp</device> ...
What is the name="mtp share" bit trying to reflect ? It seems we're mostly biased towards <model> so I think we should aim for <model type='mtp|9p'/> 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:
Would you agree with something like this?
<filesystem type='mount'> <device name="mtp share">mtp</device> ...
What is the name="mtp share" bit trying to reflect ?
It seems we're mostly biased towards <model> so I think we should aim for <model type='mtp|9p'/>
it is the name of the MTP share within the guest. How will it look with <model>? Thanks, Giuseppe
On Mon, Aug 11, 2014 at 12:32:17PM +0200, Giuseppe Scrivano wrote:
"Daniel P. Berrange" <berrange@redhat.com> writes:
Would you agree with something like this?
<filesystem type='mount'> <device name="mtp share">mtp</device> ...
What is the name="mtp share" bit trying to reflect ?
It seems we're mostly biased towards <model> so I think we should aim for <model type='mtp|9p'/>
it is the name of the MTP share within the guest. How will it look with <model>?
For the name of the guest share we'll use the existing <target path='....'> element. Yes 'path' is a misleading name - it doesn't have to be a path it can be any string - this is what we do for 9p already. 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 :|
Since the target for MTP is a name and not a path, make the function more generic. Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com> --- src/conf/domain_conf.c | 4 ++-- src/conf/domain_conf.h | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 3bdf46a..e8abc1a 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -18782,12 +18782,12 @@ virDomainFSRemove(virDomainDefPtr def, size_t i) virDomainFSDefPtr virDomainGetFilesystemForTarget(virDomainDefPtr def, - const char *path) + const char *target) { size_t i; for (i = 0; i < def->nfss; i++) { - if (STREQ(def->fss[i]->dst, path)) + if (STREQ(def->fss[i]->dst, target)) return def->fss[i]; } diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index a4d8a76..48968f0 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -2485,7 +2485,7 @@ int virDiskNameToBusDeviceIndex(virDomainDiskDefPtr disk, int *devIdx); virDomainFSDefPtr virDomainGetFilesystemForTarget(virDomainDefPtr def, - const char *path); + const char *target); int virDomainFSInsert(virDomainDefPtr def, virDomainFSDefPtr fs); int virDomainFSIndexByName(virDomainDefPtr def, const char *name); virDomainFSDefPtr virDomainFSRemove(virDomainDefPtr def, size_t i); -- 1.9.3
Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com> --- src/qemu/qemu_command.c | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index a5ff10a..716be0a 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -3988,13 +3988,13 @@ char *qemuBuildFSStr(virDomainFSDefPtr fs, } if (fs->wrpolicy) { - if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_FSDEV_WRITEOUT)) { - virBufferAsprintf(&opt, ",writeout=%s", wrpolicy); - } else { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("filesystem writeout not supported")); - goto error; - } + if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_FSDEV_WRITEOUT)) { + virBufferAsprintf(&opt, ",writeout=%s", wrpolicy); + } else { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("filesystem writeout not supported")); + goto error; + } } virBufferAsprintf(&opt, ",id=%s%s", QEMU_FSDEV_HOST_PREFIX, fs->info.alias); -- 1.9.3
Generate the qemu command line option: -device 'usb-mtp,root=$SRC,desc=$TARGET' from the definition XML: <filesystem type='mtp'> <source dir='$SRC'/> <target name='$TARGET'/> </filesystem> Closes: https://bugzilla.redhat.com/show_bug.cgi?id=1121781 Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com> --- src/qemu/qemu_command.c | 53 ++++++++++++++++++++++++++++--------------------- 1 file changed, 30 insertions(+), 23 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 716be0a..ad5b318 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -3955,12 +3955,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")); @@ -4029,22 +4023,26 @@ 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); - 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; + if (qemuBuildDeviceAddressStr(&opt, def, &fs->info, qemuCaps) < 0) + goto error; - if (virBufferCheckError(&opt) < 0) + if (virBufferCheckError(&opt) < 0) + goto error; + } + else if (fs->type == VIR_DOMAIN_FS_TYPE_MTP) { + virBufferAddLit(&opt, "usb-mtp"); + virBufferAsprintf(&opt, ",root=%s,desc=%s", fs->src, fs->dst); + } else { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("unsupported filesystem type")); goto error; + } return virBufferContentAndReset(&opt); @@ -8310,11 +8308,20 @@ 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 && + fs->type != VIR_DOMAIN_FS_TYPE_MTP) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("only supports mount or mtp filesystem type")); goto error; - virCommandAddArg(cmd, optstr); - VIR_FREE(optstr); + } + + if (fs->type == VIR_DOMAIN_FS_TYPE_MOUNT) { + 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))) -- 1.9.3
Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com> --- docs/formatdomain.html.in | 9 +++++++++ docs/schemas/domaincommon.rng | 15 ++++++++++++--- 2 files changed, 21 insertions(+), 3 deletions(-) diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index e5b1adb..938409b 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -2309,6 +2309,10 @@ <target dir='/import/from/host'/> <readonly/> </filesystem> + <filesystem type='mtp'> + <source dir='/export/to/guest'/> + <target name='my-vm-template'/> + </filesystem> ... </devices> ...</pre> @@ -2367,6 +2371,11 @@ A directory inside the guest will be bound to another directory inside the guest. Only used by LXC driver <span class="since"> (since 0.9.13)</span></dd> + <dt><code>type='mtp'</code></dt> + <dd> + A host directory will be exposed to the guest as a MTP + device. Only used by QEMU/KVM driver + <span class="since"> (since 1.2.8)</span></dd> </dl> The filesystem block has an optional attribute <code>accessmode</code> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index 11f0fd0..dd870ef 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -1899,9 +1899,18 @@ </choice> <interleave> <element name="target"> - <attribute name="dir"> - <ref name="absDirPath"/> - </attribute> + <choice> + <group> + <attribute name="dir"> + <ref name="absDirPath"/> + </attribute> + </group> + <group> + <attribute name="name"> + <ref name="genericName"/> + </attribute> + </group> + </choice> <empty/> </element> <optional> -- 1.9.3
Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com> --- tests/domainconfdata/getfilesystem.xml | 4 ++++ tests/domainconftest.c | 2 ++ 2 files changed, 6 insertions(+) diff --git a/tests/domainconfdata/getfilesystem.xml b/tests/domainconfdata/getfilesystem.xml index 2ee78b4..efcd83b 100644 --- a/tests/domainconfdata/getfilesystem.xml +++ b/tests/domainconfdata/getfilesystem.xml @@ -21,6 +21,10 @@ <source dir='/'/> <target dir='/dev'/> </filesystem> + <filesystem type='mtp'> + <source dir='/'/> + <target name='mtp share'/> + </filesystem> <console type='pty'> <target type='lxc' port='0'/> </console> diff --git a/tests/domainconftest.c b/tests/domainconftest.c index 3d6ebe1..eee3193 100644 --- a/tests/domainconftest.c +++ b/tests/domainconftest.c @@ -111,6 +111,8 @@ 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); + DO_TEST_GET_FS("mtp not existing share", false); virObjectUnref(caps); virObjectUnref(xmlopt); -- 1.9.3
participants (2)
-
Daniel P. Berrange -
Giuseppe Scrivano