* [PATCH 0/1] add support for QEMU 9pfs 'multidevs' option @ 2020-03-17 16:38 Christian Schoenebeck 2020-03-17 13:59 ` [PATCH 1/1] conf: qemu 9pfs: add " Christian Schoenebeck 0 siblings, 1 reply; 6+ messages in thread From: Christian Schoenebeck @ 2020-03-17 16:38 UTC (permalink / raw) To: libvir-list; +Cc: qemu-devel, Greg Kurz QEMU 4.2 added a new option 'multidevs' for 9pfs. The following patch adds support for this new option to libvirt. In short, what is this about: to distinguish files uniquely from each other in general, numeric file IDs are typically used for comparison, which in practice is the combination of a file's device ID and the file's inode number. Unfortunately 9p protocol's QID field used for this purpose, currently is too small to fit both the device ID and inode number in, which hence is a problem if one 9pfs export contains multiple devices and may thus lead to misbheaviours on guest (e.g. with SAMBA file servers) in that case due to potential file ID collisions. To mitigate this problem with 9pfs a 'multidevs' option was introduced in QEMU 4.2 for defining how to deal with this, e.g. multidevs=remap will cause QEMU's 9pfs implementation to remap all inodes from host side to different inode numbers on guest side in a way that prevents file ID collisions. NOTE: In the libvirt docs changes of this libvirt patch I simply assumed "since 6.2.0". So the final libvirt version number would need to be adjusted in that text if necessary. See QEMU discussion with following Message-ID for details: 8a2ffe17fda3a86b9a5a437e1245276881f1e235.1567680121.git.qemu_oss@crudebyte.com Christian Schoenebeck (1): conf: qemu 9pfs: add 'multidevs' option docs/formatdomain.html.in | 47 ++++++++++++++++++++++++++++++++++- docs/schemas/domaincommon.rng | 10 ++++++++ src/conf/domain_conf.c | 30 ++++++++++++++++++++++ src/conf/domain_conf.h | 13 ++++++++++ src/qemu/qemu_command.c | 7 ++++++ 5 files changed, 106 insertions(+), 1 deletion(-) -- 2.20.1 ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 1/1] conf: qemu 9pfs: add 'multidevs' option @ 2020-03-17 13:59 ` Christian Schoenebeck 2020-03-19 13:10 ` Ján Tomko 0 siblings, 1 reply; 6+ messages in thread From: Christian Schoenebeck @ 2020-03-17 13:59 UTC (permalink / raw) To: libvir-list; +Cc: qemu-devel, Greg Kurz Introduce new 'multidevs' option for filesystem. <filesystem type='mount' accessmode='mapped' multidevs='remap'> <source dir='/path'/> <target dir='mount_tag'> </filesystem> This option prevents misbheaviours on guest if a 9pfs export contains multiple devices, due to the potential file ID collisions this otherwise may cause. Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com> --- docs/formatdomain.html.in | 47 ++++++++++++++++++++++++++++++++++- docs/schemas/domaincommon.rng | 10 ++++++++ src/conf/domain_conf.c | 30 ++++++++++++++++++++++ src/conf/domain_conf.h | 13 ++++++++++ src/qemu/qemu_command.c | 7 ++++++ 5 files changed, 106 insertions(+), 1 deletion(-) diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index 594146009d..13c506988b 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -3967,7 +3967,7 @@ <source name='my-vm-template'/> <target dir='/'/> </filesystem> - <filesystem type='mount' accessmode='passthrough'> + <filesystem type='mount' accessmode='passthrough' multidevs='remap'> <driver type='path' wrpolicy='immediate'/> <source dir='/export/to/guest'/> <target dir='/import/from/host'/> @@ -4084,13 +4084,58 @@ </dd> </dl> + <p> <span class="since">Since 5.2.0</span>, the filesystem element has an optional attribute <code>model</code> with supported values "virtio-transitional", "virtio-non-transitional", or "virtio". See <a href="#elementsVirtioTransitional">Virtio transitional devices</a> for more details. + </p> + + <p> + The filesystem element has an optional attribute <code>multidevs</code> + which specifies how to deal with a filesystem export containing more than + one device, in order to avoid file ID collisions on guest when using 9pfs + (<span class="since">since 6.2.0, requires QEMU 4.2</span>). + This attribute is not available for virtiofs. The possible values are: + </p> + + <dl> + <dt><code>default</code></dt> + <dd> + Use QEMU's default setting (which currently is <code>warn</code>). + </dd> + <dt><code>remap</code></dt> + <dd> + This setting allows guest to access multiple devices per export without + encountering misbehaviours. Inode numbers from host are automatically + remapped on guest to actively prevent file ID collisions if guest + accesses one export containing multiple devices. + </dd> + <dt><code>forbid</code></dt> + <dd> + Only allow to access one device per export by guest. Attempts to access + additional devices on the same export will cause the individual + filesystem access by guest to fail with an error and being logged (once) + as error on host side. + </dd> + <dt><code>warn</code></dt> + <dd> + This setting resembles the behaviour of 9pfs prior to QEMU 4.2, that is + no action is performed to prevent any potential file ID collisions if an + export contains multiple devices, with the only exception: a warning is + logged (once) on host side now. This setting may lead to misbehaviours + on guest side if more than one device is exported per export, due to the + potential file ID collisions this may cause on guest side in that case. + </dd> + </dl> + </dd> + <p> + The <code>filesystem</code> element may contain the following subelements: + </p> + <dt><code>driver</code></dt> <dd> The optional driver element allows specifying further details diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index 6805420451..9b37740e30 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -2676,6 +2676,16 @@ </choice> </attribute> </optional> + <optional> + <attribute name="multidevs"> + <choice> + <value>default</value> + <value>remap</value> + <value>forbid</value> + <value>warn</value> + </choice> + </attribute> + </optional> <optional> <element name='readonly'> <empty/> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 71535f53f5..b96f87063a 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -501,6 +501,14 @@ VIR_ENUM_IMPL(virDomainFSModel, "virtio-non-transitional", ); +VIR_ENUM_IMPL(virDomainFSMultidevs, + VIR_DOMAIN_FS_MULTIDEVS_LAST, + "default", + "remap", + "forbid", + "warn", +); + VIR_ENUM_IMPL(virDomainFSCacheMode, VIR_DOMAIN_FS_CACHE_MODE_LAST, "default", @@ -11376,6 +11384,7 @@ virDomainFSDefParseXML(virDomainXMLOptionPtr xmlopt, g_autofree char *usage = NULL; g_autofree char *units = NULL; g_autofree char *model = NULL; + g_autofree char *multidevs = NULL; ctxt->node = node; @@ -11414,6 +11423,17 @@ virDomainFSDefParseXML(virDomainXMLOptionPtr xmlopt, } } + multidevs = virXMLPropString(node, "multidevs"); + if (multidevs) { + if ((def->multidevs = virDomainFSMultidevsTypeFromString(multidevs)) < 0) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("unknown multidevs '%s'"), multidevs); + goto error; + } + } else { + def->multidevs = VIR_DOMAIN_FS_MULTIDEVS_DEFAULT; + } + if (virDomainParseScaledValue("./space_hard_limit[1]", NULL, ctxt, &def->space_hard_limit, 1, ULLONG_MAX, false) < 0) @@ -25397,6 +25417,7 @@ virDomainFSDefFormat(virBufferPtr buf, const char *accessmode = virDomainFSAccessModeTypeToString(def->accessmode); const char *fsdriver = virDomainFSDriverTypeToString(def->fsdriver); const char *wrpolicy = virDomainFSWrpolicyTypeToString(def->wrpolicy); + const char *multidevs = virDomainFSMultidevsTypeToString(def->multidevs); const char *src = def->src->path; g_auto(virBuffer) driverAttrBuf = VIR_BUFFER_INITIALIZER; g_auto(virBuffer) driverBuf = VIR_BUFFER_INIT_CHILD(buf); @@ -25415,6 +25436,12 @@ virDomainFSDefFormat(virBufferPtr buf, return -1; } + if (!multidevs) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("unexpected multidevs %d"), def->multidevs); + return -1; + } + virBufferAsprintf(buf, "<filesystem type='%s' accessmode='%s'", type, accessmode); @@ -25422,6 +25449,9 @@ virDomainFSDefFormat(virBufferPtr buf, virBufferAsprintf(buf, " model='%s'", virDomainFSModelTypeToString(def->model)); } + if (def->multidevs) { + virBufferAsprintf(buf, " multidevs='%s'", multidevs); + } virBufferAddLit(buf, ">\n"); virBufferAdjustIndent(buf, 2); diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 91b776c28a..23b7924781 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -796,6 +796,18 @@ typedef enum { VIR_DOMAIN_FS_WRPOLICY_LAST } virDomainFSWrpolicy; +/* How to handle exports containing multiple devices. */ +typedef enum { + VIR_DOMAIN_FS_MULTIDEVS_DEFAULT = 0, /* Use QEMU's default setting */ + VIR_DOMAIN_FS_MULTIDEVS_REMAP, /* Remap inodes from host to guest */ + VIR_DOMAIN_FS_MULTIDEVS_FORBID, /* Prohibit more than one device */ + VIR_DOMAIN_FS_MULTIDEVS_WARN, /* Just log a warning if multiple devices */ + + VIR_DOMAIN_FS_MULTIDEVS_LAST +} virDomainFSMultidevs; + +VIR_ENUM_DECL(virDomainFSMultidevs); + typedef enum { VIR_DOMAIN_FS_MODEL_DEFAULT = 0, VIR_DOMAIN_FS_MODEL_VIRTIO, @@ -820,6 +832,7 @@ struct _virDomainFSDef { int wrpolicy; /* enum virDomainFSWrpolicy */ int format; /* virStorageFileFormat */ int model; /* virDomainFSModel */ + int multidevs; /* virDomainFSMultidevs */ unsigned long long usage; /* in bytes */ virStorageSourcePtr src; char *dst; diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 9790c92cf8..7020e5448c 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -2632,6 +2632,13 @@ qemuBuildFSStr(virDomainFSDefPtr fs) } else if (fs->accessmode == VIR_DOMAIN_FS_ACCESSMODE_SQUASH) { virBufferAddLit(&opt, ",security_model=none"); } + if (fs->multidevs == VIR_DOMAIN_FS_MULTIDEVS_REMAP) { + virBufferAddLit(&opt, ",multidevs=remap"); + } else if (fs->multidevs == VIR_DOMAIN_FS_MULTIDEVS_FORBID) { + virBufferAddLit(&opt, ",multidevs=forbid"); + } else if (fs->multidevs == VIR_DOMAIN_FS_MULTIDEVS_WARN) { + virBufferAddLit(&opt, ",multidevs=warn"); + } } else if (fs->fsdriver == VIR_DOMAIN_FS_DRIVER_TYPE_HANDLE) { /* removed since qemu 4.0.0 see v3.1.0-29-g93aee84f57 */ virBufferAddLit(&opt, "handle"); -- 2.20.1 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 1/1] conf: qemu 9pfs: add 'multidevs' option 2020-03-17 13:59 ` [PATCH 1/1] conf: qemu 9pfs: add " Christian Schoenebeck @ 2020-03-19 13:10 ` Ján Tomko 2020-03-19 15:57 ` Christian Schoenebeck 0 siblings, 1 reply; 6+ messages in thread From: Ján Tomko @ 2020-03-19 13:10 UTC (permalink / raw) To: Christian Schoenebeck; +Cc: libvir-list, qemu-devel, Greg Kurz [-- Attachment #1: Type: text/plain, Size: 4982 bytes --] On a Tuesday in 2020, Christian Schoenebeck wrote: >Introduce new 'multidevs' option for filesystem. > > <filesystem type='mount' accessmode='mapped' multidevs='remap'> I don't like the 'multidevs' name, but cannot think of anything beter. 'collisions' maybe? > <source dir='/path'/> > <target dir='mount_tag'> > </filesystem> > >This option prevents misbheaviours on guest if a 9pfs export >contains multiple devices, due to the potential file ID collisions >this otherwise may cause. > >Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com> >--- > docs/formatdomain.html.in | 47 ++++++++++++++++++++++++++++++++++- > docs/schemas/domaincommon.rng | 10 ++++++++ > src/conf/domain_conf.c | 30 ++++++++++++++++++++++ > src/conf/domain_conf.h | 13 ++++++++++ > src/qemu/qemu_command.c | 7 ++++++ > 5 files changed, 106 insertions(+), 1 deletion(-) Please split the XML changes from the qemu driver changes. Also missing: * qemu_capabilities addition * qemuDomainDeviceDefValidateFS in qemu_domain.c - check for the capability, reject this setting for virtiofs * qemuxml2xmltest addition * qemuxml2argvtest addition (no changes required for virschematest - it checks all the XML files in the directories used by the above tests against the schema) > >diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in >index 594146009d..13c506988b 100644 >--- a/docs/formatdomain.html.in >+++ b/docs/formatdomain.html.in >@@ -3967,7 +3967,7 @@ > <source name='my-vm-template'/> > <target dir='/'/> > </filesystem> >- <filesystem type='mount' accessmode='passthrough'> >+ <filesystem type='mount' accessmode='passthrough' multidevs='remap'> > <driver type='path' wrpolicy='immediate'/> > <source dir='/export/to/guest'/> > <target dir='/import/from/host'/> >@@ -4084,13 +4084,58 @@ > </dd> > </dl> > >+ <p> > <span class="since">Since 5.2.0</span>, the filesystem element > has an optional attribute <code>model</code> with supported values > "virtio-transitional", "virtio-non-transitional", or "virtio". > See <a href="#elementsVirtioTransitional">Virtio transitional devices</a> > for more details. >+ </p> >+ Unrelated change that can be split out. >+ <p> >+ The filesystem element has an optional attribute <code>multidevs</code> >+ which specifies how to deal with a filesystem export containing more than >+ one device, in order to avoid file ID collisions on guest when using 9pfs >+ (<span class="since">since 6.2.0, requires QEMU 4.2</span>). >+ This attribute is not available for virtiofs. The possible values are: >+ </p> >+ >+ <dl> >+ <dt><code>default</code></dt> >+ <dd> >+ Use QEMU's default setting (which currently is <code>warn</code>). >+ </dd> >+ <dt><code>remap</code></dt> >+ <dd> >+ This setting allows guest to access multiple devices per export without >+ encountering misbehaviours. Inode numbers from host are automatically >+ remapped on guest to actively prevent file ID collisions if guest >+ accesses one export containing multiple devices. >+ </dd> >+ <dt><code>forbid</code></dt> >+ <dd> >+ Only allow to access one device per export by guest. Attempts to access >+ additional devices on the same export will cause the individual >+ filesystem access by guest to fail with an error and being logged (once) >+ as error on host side. >+ </dd> >+ <dt><code>warn</code></dt> >+ <dd> >+ This setting resembles the behaviour of 9pfs prior to QEMU 4.2, that is >+ no action is performed to prevent any potential file ID collisions if an >+ export contains multiple devices, with the only exception: a warning is >+ logged (once) on host side now. This setting may lead to misbehaviours >+ on guest side if more than one device is exported per export, due to the >+ potential file ID collisions this may cause on guest side in that case. >+ </dd> >+ </dl> >+ > </dd> > >+ <p> >+ The <code>filesystem</code> element may contain the following subelements: >+ </p> >+ And so can this one. > <dt><code>driver</code></dt> > <dd> > The optional driver element allows specifying further details >@@ -25422,6 +25449,9 @@ virDomainFSDefFormat(virBufferPtr buf, > virBufferAsprintf(buf, " model='%s'", > virDomainFSModelTypeToString(def->model)); > } >+ if (def->multidevs) { >+ virBufferAsprintf(buf, " multidevs='%s'", multidevs); >+ } make syntax-check complains here: Curly brackets around single-line body: ../src/conf/domain_conf.c:25452-25454: if (def->multidevs) { virBufferAsprintf(buf, " multidevs='%s'", multidevs); } Jano [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --] ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/1] conf: qemu 9pfs: add 'multidevs' option 2020-03-19 13:10 ` Ján Tomko @ 2020-03-19 15:57 ` Christian Schoenebeck 2020-03-19 16:02 ` Daniel P. Berrangé 2020-03-19 16:19 ` Ján Tomko 0 siblings, 2 replies; 6+ messages in thread From: Christian Schoenebeck @ 2020-03-19 15:57 UTC (permalink / raw) To: qemu-devel; +Cc: Ján Tomko, libvir-list, Greg Kurz On Donnerstag, 19. März 2020 14:10:26 CET Ján Tomko wrote: > On a Tuesday in 2020, Christian Schoenebeck wrote: > >Introduce new 'multidevs' option for filesystem. > > > > <filesystem type='mount' accessmode='mapped' multidevs='remap'> > > I don't like the 'multidevs' name, but cannot think of anything > beter. > > 'collisions' maybe? Not sure if 'collisions' is better, e.g. collisions='remap' sounds scary. :) And which collision would that be? At least IMO 'multidevs' is less ambigious. I have no problem though to change it to whatever name you might come up with. Just keep the resulting key-value pair set in mind: multidevs='default' multidevs='remap' multidevs='forbid' multidevs='warn' vs. collisions='default' collisions='remap' <- probably misleading what 'remap' means in this case collisions='forbid' collisions='warn' <- wrong, it warns about multiple devices, not about file ID collisions. So different key-name might also require different value-names. Another option would be the long form 'multi-devices=...' > > <source dir='/path'/> > > <target dir='mount_tag'> > > > > </filesystem> > > > >This option prevents misbheaviours on guest if a 9pfs export > >contains multiple devices, due to the potential file ID collisions > >this otherwise may cause. > > > >Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com> > >--- > > > > docs/formatdomain.html.in | 47 ++++++++++++++++++++++++++++++++++- > > docs/schemas/domaincommon.rng | 10 ++++++++ > > src/conf/domain_conf.c | 30 ++++++++++++++++++++++ > > src/conf/domain_conf.h | 13 ++++++++++ > > src/qemu/qemu_command.c | 7 ++++++ > > 5 files changed, 106 insertions(+), 1 deletion(-) > > Please split the XML changes from the qemu driver changes. Ok > Also missing: > * qemu_capabilities addition AFAICS the common procedure is to add new capabilities always to the end of the enum list. So I guess I will do that as well. > * qemuDomainDeviceDefValidateFS in qemu_domain.c - check for the capability, > reject this setting for virtiofs Good to know where that check is supposed to go to, thanks! > * qemuxml2xmltest addition > * qemuxml2argvtest addition Ok, I have to read up how those tests work. AFAICS I need to add xml files to their data subdirs. Separate patches required for those 2 tests? > (no changes required for virschematest - it checks all the XML files in > the directories used by the above tests against the schema) > > >diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in > >index 594146009d..13c506988b 100644 > >--- a/docs/formatdomain.html.in > >+++ b/docs/formatdomain.html.in > >@@ -3967,7 +3967,7 @@ > > > > <source name='my-vm-template'/> > > <target dir='/'/> > > > > </filesystem> > > > >- <filesystem type='mount' accessmode='passthrough'> > >+ <filesystem type='mount' accessmode='passthrough' > >multidevs='remap'>> > > <driver type='path' wrpolicy='immediate'/> > > <source dir='/export/to/guest'/> > > <target dir='/import/from/host'/> > > > >@@ -4084,13 +4084,58 @@ > > > > </dd> > > </dl> > > > >+ <p> > > > > <span class="since">Since 5.2.0</span>, the filesystem element > > has an optional attribute <code>model</code> with supported values > > "virtio-transitional", "virtio-non-transitional", or "virtio". > > See <a href="#elementsVirtioTransitional">Virtio transitional > > devices</a> > > for more details. > > > >+ </p> > >+ > > Unrelated change that can be split out. Ok, I'll make that the 1st preparatory patch then including ... > >+ <p> > >+ The filesystem element has an optional attribute > ><code>multidevs</code> + which specifies how to deal with a > >filesystem export containing more than + one device, in order to > >avoid file ID collisions on guest when using 9pfs + (<span > >class="since">since 6.2.0, requires QEMU 4.2</span>). + This > >attribute is not available for virtiofs. The possible values are: + > ></p> > >+ > >+ <dl> > >+ <dt><code>default</code></dt> > >+ <dd> > >+ Use QEMU's default setting (which currently is <code>warn</code>). > >+ </dd> > >+ <dt><code>remap</code></dt> > >+ <dd> > >+ This setting allows guest to access multiple devices per export > >without + encountering misbehaviours. Inode numbers from host are > >automatically + remapped on guest to actively prevent file ID > >collisions if guest + accesses one export containing multiple > >devices. > >+ </dd> > >+ <dt><code>forbid</code></dt> > >+ <dd> > >+ Only allow to access one device per export by guest. Attempts to > >access + additional devices on the same export will cause the > >individual + filesystem access by guest to fail with an error and > >being logged (once) + as error on host side. > >+ </dd> > >+ <dt><code>warn</code></dt> > >+ <dd> > >+ This setting resembles the behaviour of 9pfs prior to QEMU 4.2, > >that is + no action is performed to prevent any potential file ID > >collisions if an + export contains multiple devices, with the only > >exception: a warning is + logged (once) on host side now. This > >setting may lead to misbehaviours + on guest side if more than one > >device is exported per export, due to the + potential file ID > >collisions this may cause on guest side in that case. + </dd> > >+ </dl> > >+ > > > > </dd> > > > >+ <p> > >+ The <code>filesystem</code> element may contain the following > >subelements: + </p> > >+ > > And so can this one. ... this one. > > <dt><code>driver</code></dt> > > <dd> > > > > The optional driver element allows specifying further details > > > >@@ -25422,6 +25449,9 @@ virDomainFSDefFormat(virBufferPtr buf, > > > > virBufferAsprintf(buf, " model='%s'", > > > > virDomainFSModelTypeToString(def->model)); > > > > } > > > >+ if (def->multidevs) { > >+ virBufferAsprintf(buf, " multidevs='%s'", multidevs); > >+ } > > make syntax-check complains here: > Curly brackets around single-line body: > ../src/conf/domain_conf.c:25452-25454: > if (def->multidevs) { > virBufferAsprintf(buf, " multidevs='%s'", multidevs); > } > > Jano Sorry for that, I assumed same code style as qemu. I'll do the automated syntax checks from now on. Best regards, Christian Schoenebeck ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/1] conf: qemu 9pfs: add 'multidevs' option 2020-03-19 15:57 ` Christian Schoenebeck @ 2020-03-19 16:02 ` Daniel P. Berrangé 2020-03-19 16:19 ` Ján Tomko 1 sibling, 0 replies; 6+ messages in thread From: Daniel P. Berrangé @ 2020-03-19 16:02 UTC (permalink / raw) To: Christian Schoenebeck; +Cc: libvir-list, Ján Tomko, qemu-devel, Greg Kurz On Thu, Mar 19, 2020 at 04:57:41PM +0100, Christian Schoenebeck wrote: > On Donnerstag, 19. März 2020 14:10:26 CET Ján Tomko wrote: > > On a Tuesday in 2020, Christian Schoenebeck wrote: > > >Introduce new 'multidevs' option for filesystem. > > > > > > <filesystem type='mount' accessmode='mapped' multidevs='remap'> > > > > I don't like the 'multidevs' name, but cannot think of anything > > beter. > > > > 'collisions' maybe? > > Not sure if 'collisions' is better, e.g. collisions='remap' sounds scary. :) > And which collision would that be? At least IMO 'multidevs' is less ambigious. > I have no problem though to change it to whatever name you might come up with. > Just keep the resulting key-value pair set in mind: > > multidevs='default' > multidevs='remap' > multidevs='forbid' > multidevs='warn' > > vs. > > collisions='default' > collisions='remap' <- probably misleading what 'remap' means in this case > collisions='forbid' > collisions='warn' <- wrong, it warns about multiple devices, not about file ID > collisions. > > So different key-name might also require different value-names. > > Another option would be the long form 'multi-devices=...' I tried to come up with names when this was posted to QEMU, but didn't think of much better than multidevs, so I think that's acceptable for libvirt usage. "collisions" isn't better enough to justify different naming from QEMU Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :| ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/1] conf: qemu 9pfs: add 'multidevs' option 2020-03-19 15:57 ` Christian Schoenebeck 2020-03-19 16:02 ` Daniel P. Berrangé @ 2020-03-19 16:19 ` Ján Tomko 1 sibling, 0 replies; 6+ messages in thread From: Ján Tomko @ 2020-03-19 16:19 UTC (permalink / raw) To: Christian Schoenebeck; +Cc: libvir-list, qemu-devel, Greg Kurz [-- Attachment #1: Type: text/plain, Size: 2703 bytes --] On a Thursday in 2020, Christian Schoenebeck wrote: >On Donnerstag, 19. März 2020 14:10:26 CET Ján Tomko wrote: >> On a Tuesday in 2020, Christian Schoenebeck wrote: >> >Introduce new 'multidevs' option for filesystem. >> > >> > <filesystem type='mount' accessmode='mapped' multidevs='remap'> >> >> I don't like the 'multidevs' name, but cannot think of anything >> beter. >> >> 'collisions' maybe? > >Not sure if 'collisions' is better, e.g. collisions='remap' sounds scary. :) >And which collision would that be? At least IMO 'multidevs' is less ambigious. >I have no problem though to change it to whatever name you might come up with. >Just keep the resulting key-value pair set in mind: > >multidevs='default' >multidevs='remap' >multidevs='forbid' >multidevs='warn' > >vs. > >collisions='default' >collisions='remap' <- probably misleading what 'remap' means in this case >collisions='forbid' >collisions='warn' <- wrong, it warns about multiple devices, not about file ID >collisions. > >So different key-name might also require different value-names. > >Another option would be the long form 'multi-devices=...' Okay, let's leave it at 'multidevs'. > >> > <source dir='/path'/> >> > <target dir='mount_tag'> >> > >> > </filesystem> >> > >> >This option prevents misbheaviours on guest if a 9pfs export >> >contains multiple devices, due to the potential file ID collisions >> >this otherwise may cause. >> > >> >Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com> >> >--- >> > >> > docs/formatdomain.html.in | 47 ++++++++++++++++++++++++++++++++++- >> > docs/schemas/domaincommon.rng | 10 ++++++++ >> > src/conf/domain_conf.c | 30 ++++++++++++++++++++++ >> > src/conf/domain_conf.h | 13 ++++++++++ >> > src/qemu/qemu_command.c | 7 ++++++ >> > 5 files changed, 106 insertions(+), 1 deletion(-) >> >> Please split the XML changes from the qemu driver changes. > >Ok > >> Also missing: >> * qemu_capabilities addition > >AFAICS the common procedure is to add new capabilities always to the end of >the enum list. So I guess I will do that as well. > >> * qemuDomainDeviceDefValidateFS in qemu_domain.c - check for the capability, >> reject this setting for virtiofs > >Good to know where that check is supposed to go to, thanks! > >> * qemuxml2xmltest addition >> * qemuxml2argvtest addition > >Ok, I have to read up how those tests work. AFAICS I need to add xml files to >their data subdirs. > >Separate patches required for those 2 tests? Usually xml2xmltest is in the same test as the XML parser/formatter and xml2argvtest is a part of the qemu driver patch. Jano [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --] ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2020-03-19 16:21 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2020-03-17 16:38 [PATCH 0/1] add support for QEMU 9pfs 'multidevs' option Christian Schoenebeck 2020-03-17 13:59 ` [PATCH 1/1] conf: qemu 9pfs: add " Christian Schoenebeck 2020-03-19 13:10 ` Ján Tomko 2020-03-19 15:57 ` Christian Schoenebeck 2020-03-19 16:02 ` Daniel P. Berrangé 2020-03-19 16:19 ` Ján Tomko
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).