[PATCH 0/2] qemu: Drop support for virtiofsd written in C and modernize cmd line
*** BLURB HERE *** Michal Prívozník (2): qemu: Drop support for C implementation of virtiofsd qemu_virtiofs: Don't use deprecated cmd line src/qemu/qemu_validate.c | 6 ++++ src/qemu/qemu_virtiofs.c | 33 +++++++------------ .../vhost-user-fs-fd-memory.xml | 1 - 3 files changed, 17 insertions(+), 23 deletions(-) -- 2.41.0
Virtiofsd has two implementations: C and Rust. The former is now deprecated (QEMU commit v7.0.0-rc0~52^2~1) and in fact removed from QEMU (QEMU commit v8.0.0-rc0~55). While Rust version was originally a drop in replacement it is not the case anymore. Some arguments are silently ignored (like file locking) and there's no way to make them work for both implementations. Remove support for the C implementation. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_validate.c | 6 ++++++ src/qemu/qemu_virtiofs.c | 14 ++++---------- tests/qemuxml2argvdata/vhost-user-fs-fd-memory.xml | 1 - 3 files changed, 10 insertions(+), 11 deletions(-) diff --git a/src/qemu/qemu_validate.c b/src/qemu/qemu_validate.c index e475ad035e..2c70409756 100644 --- a/src/qemu/qemu_validate.c +++ b/src/qemu/qemu_validate.c @@ -4368,6 +4368,12 @@ qemuValidateDomainDeviceDefFS(virDomainFSDef *fs, _("virtiofs does not support fmode and dmode")); return -1; } + if (fs->flock != VIR_TRISTATE_SWITCH_ABSENT || + fs->posix_lock != VIR_TRISTATE_SWITCH_ABSENT) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("file locking is not supported")); + return -1; + } if (qemuValidateDomainDefVhostUserRequireSharedMemory(def, "virtiofs") < 0) { return -1; } diff --git a/src/qemu/qemu_virtiofs.c b/src/qemu/qemu_virtiofs.c index 230f85c291..7ff6066f07 100644 --- a/src/qemu/qemu_virtiofs.c +++ b/src/qemu/qemu_virtiofs.c @@ -132,6 +132,10 @@ qemuVirtioFSBuildCommandLine(virQEMUDriverConfig *cfg, g_autoptr(virCommand) cmd = NULL; g_auto(virBuffer) opts = VIR_BUFFER_INITIALIZER; + /* Some @fs attributes (lock and posix_lock) are not handled here nor + * anywhwere else. The reason is they exist because (now deprecated) C + * implementation of virtiofsd supported them, but RUST implementation does + * not. And we only support the latter. */ cmd = virCommandNew(fs->binary); virCommandAddArgFormat(cmd, "--fd=%d", *fd); @@ -151,16 +155,6 @@ qemuVirtioFSBuildCommandLine(virQEMUDriverConfig *cfg, else if (fs->xattr == VIR_TRISTATE_SWITCH_OFF) virBufferAddLit(&opts, ",no_xattr"); - if (fs->flock == VIR_TRISTATE_SWITCH_ON) - virBufferAddLit(&opts, ",flock"); - else if (fs->flock == VIR_TRISTATE_SWITCH_OFF) - virBufferAddLit(&opts, ",no_flock"); - - if (fs->posix_lock == VIR_TRISTATE_SWITCH_ON) - virBufferAddLit(&opts, ",posix_lock"); - else if (fs->posix_lock == VIR_TRISTATE_SWITCH_OFF) - virBufferAddLit(&opts, ",no_posix_lock"); - virCommandAddArgBuffer(cmd, &opts); if (fs->thread_pool_size >= 0) diff --git a/tests/qemuxml2argvdata/vhost-user-fs-fd-memory.xml b/tests/qemuxml2argvdata/vhost-user-fs-fd-memory.xml index 81de8c0dd7..5bc758b8d6 100644 --- a/tests/qemuxml2argvdata/vhost-user-fs-fd-memory.xml +++ b/tests/qemuxml2argvdata/vhost-user-fs-fd-memory.xml @@ -31,7 +31,6 @@ <binary path='/usr/libexec/virtiofsd' xattr='on'> <cache mode='always'/> <sandbox mode='chroot'/> - <lock posix='off' flock='off'/> <thread_pool size='16'/> </binary> <source dir='/path'/> -- 2.41.0
On Tue, Dec 12, 2023 at 12:50:52 +0100, Michal Privoznik wrote:
Virtiofsd has two implementations: C and Rust. The former is now deprecated (QEMU commit v7.0.0-rc0~52^2~1) and in fact removed from QEMU (QEMU commit v8.0.0-rc0~55). While Rust version was originally a drop in replacement it is not the case anymore. Some arguments are silently ignored (like file locking) and there's no way to make them work for both implementations.
Remove support for the C implementation.
Note that the virtiofs support in qemu seems to start dating from qemu-5.0 thus was already availabile at least in RHEL/CentOS 8 Now the rust version: https://repology.org/project/virtiofsd/versions is not available there. Since according to our platform support policy we're still about to support rhel-8 for another ~7 months I'm not sure we should be doing this. At least not with any form of proper justification, that this commit message is not providing.
On 12/12/23 13:07, Peter Krempa wrote:
On Tue, Dec 12, 2023 at 12:50:52 +0100, Michal Privoznik wrote:
Virtiofsd has two implementations: C and Rust. The former is now deprecated (QEMU commit v7.0.0-rc0~52^2~1) and in fact removed from QEMU (QEMU commit v8.0.0-rc0~55). While Rust version was originally a drop in replacement it is not the case anymore. Some arguments are silently ignored (like file locking) and there's no way to make them work for both implementations.
Remove support for the C implementation.
Note that the virtiofs support in qemu seems to start dating from qemu-5.0 thus was already availabile at least in RHEL/CentOS 8
Now the rust version:
https://repology.org/project/virtiofsd/versions
is not available there.
Since according to our platform support policy we're still about to support rhel-8 for another ~7 months I'm not sure we should be doing this. At least not with any form of proper justification, that this commit message is not providing.
Fair enough. I've noticed some warnings emitted by virtiofsd when I was debugging something else and I thought: let's fix them. And while Rust version was supposed to be a drop in replacement it's not the case anymore (though - I wonder if that ever was the case) as it has moved on. Now, what we usually do in this case is: capabilities. We could run the binary and try to guess if it is C or Rust version. But writing all of that code just so that it can be deleted in ~7 months, just no. So let's keep the code as is and these can be applied after RHEL-8 is dropped. Though, one could argue that the set of users that runs libvirt from git but qemu from repo on RHEL/CentOS 8 is relatively small, the rule we have is a bit different. Michal
On a Thursday in 2023, Michal Prívozník wrote:
On 12/12/23 13:07, Peter Krempa wrote:
On Tue, Dec 12, 2023 at 12:50:52 +0100, Michal Privoznik wrote:
Virtiofsd has two implementations: C and Rust. The former is now deprecated (QEMU commit v7.0.0-rc0~52^2~1) and in fact removed from QEMU (QEMU commit v8.0.0-rc0~55). While Rust version was originally a drop in replacement it is not the case anymore. Some arguments are silently ignored (like file locking) and there's no way to make them work for both implementations.
Remove support for the C implementation.
Note that the virtiofs support in qemu seems to start dating from qemu-5.0 thus was already availabile at least in RHEL/CentOS 8
Now the rust version:
https://repology.org/project/virtiofsd/versions
is not available there.
Since according to our platform support policy we're still about to support rhel-8 for another ~7 months I'm not sure we should be doing this. At least not with any form of proper justification, that this commit message is not providing.
Fair enough. I've noticed some warnings emitted by virtiofsd when I was debugging something else and I thought: let's fix them. And while Rust version was supposed to be a drop in replacement it's not the case anymore (though - I wonder if that ever was the case) as it has moved on.
Now, what we usually do in this case is: capabilities. We could run the binary and try to guess if it is C or Rust version. But writing all of that code just so that it can be deleted in ~7 months, just no. So let's keep the code as is and these can be applied after RHEL-8 is dropped.
Running the binary and guessing stuff is the approach we took when we used to parse QEMU's --help output. I'm glad that code is gone and I hope we don't have to do any guessing in the future. For virtiofsd users, most will have the vhost-user json file: $ cat /usr/share/qemu/vhost-user/50-qemu-virtiofsd.json { "description": "QEMU virtiofsd vhost-user-fs", "type": "fs", "binary": "/usr/libexec/virtiofsd" } which can in theory contain some hints/capabilities that would let us tell them apart. I reported the bug against virtiofsd: https://issues.redhat.com/browse/RHEL-7415 However the discussion died out and I did not volunteer any patches, since I don't like writing code that should be deleted soon. The approach however won't work for some old RHEL/CentOS releases, where the .json file was installed in some weird location and in cases where the user supplies their own path to the virtiofsd binary (e.g. to some build from git) - the former will likely use the old options, while the git build will probably have the new ones. (Also note that the ~7 months deadline is for RHEL/CentOS, Peter did not check whether other distros still ship the old virtiofsd.) Jano
Though, one could argue that the set of users that runs libvirt from git but qemu from repo on RHEL/CentOS 8 is relatively small, the rule we have is a bit different.
Michal _______________________________________________ Devel mailing list -- devel@lists.libvirt.org To unsubscribe send an email to devel-leave@lists.libvirt.org
On 12/12/23 05:07, Peter Krempa wrote:
On Tue, Dec 12, 2023 at 12:50:52 +0100, Michal Privoznik wrote:
Virtiofsd has two implementations: C and Rust. The former is now deprecated (QEMU commit v7.0.0-rc0~52^2~1) and in fact removed from QEMU (QEMU commit v8.0.0-rc0~55). While Rust version was originally a drop in replacement it is not the case anymore. Some arguments are silently ignored (like file locking) and there's no way to make them work for both implementations.
Remove support for the C implementation.
Note that the virtiofs support in qemu seems to start dating from qemu-5.0 thus was already availabile at least in RHEL/CentOS 8
Now the rust version:
https://repology.org/project/virtiofsd/versions
is not available there.
Since according to our platform support policy we're still about to support rhel-8 for another ~7 months I'm not sure we should be doing this. At least not with any form of proper justification, that this commit message is not providing.
The same can be said for SLE15 SP5. It has the C implementation and is under general support until Dec 31, 2024. SLE15 SP6 will have the Rust implementation. Regards, Jim
Rust implementation has deprecated use if "-o option1,option2" in favor of "--option1" "--option2". Actually, they did so quite while ago and continued using the old way only for backwards compatibility. Use "modern" way. Resolves: https://issues.redhat.com/browse/RHEL-7108 Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_virtiofs.c | 19 +++++++------------ 1 file changed, 7 insertions(+), 12 deletions(-) diff --git a/src/qemu/qemu_virtiofs.c b/src/qemu/qemu_virtiofs.c index 7ff6066f07..2d8eaa4bc5 100644 --- a/src/qemu/qemu_virtiofs.c +++ b/src/qemu/qemu_virtiofs.c @@ -130,7 +130,6 @@ qemuVirtioFSBuildCommandLine(virQEMUDriverConfig *cfg, int *fd) { g_autoptr(virCommand) cmd = NULL; - g_auto(virBuffer) opts = VIR_BUFFER_INITIALIZER; /* Some @fs attributes (lock and posix_lock) are not handled here nor * anywhwere else. The reason is they exist because (now deprecated) C @@ -142,26 +141,22 @@ qemuVirtioFSBuildCommandLine(virQEMUDriverConfig *cfg, virCommandPassFD(cmd, *fd, VIR_COMMAND_PASS_FD_CLOSE_PARENT); *fd = -1; - virCommandAddArg(cmd, "-o"); - virBufferAddLit(&opts, "source="); - virQEMUBuildBufferEscapeComma(&opts, fs->src->path); + virCommandAddArgPair(cmd, "--shared-dir", fs->src->path); + if (fs->cache) - virBufferAsprintf(&opts, ",cache=%s", virDomainFSCacheModeTypeToString(fs->cache)); + virCommandAddArgPair(cmd, "--cache", virDomainFSCacheModeTypeToString(fs->cache)); + if (fs->sandbox) - virBufferAsprintf(&opts, ",sandbox=%s", virDomainFSSandboxModeTypeToString(fs->sandbox)); + virCommandAddArgPair(cmd, "--sandbox", virDomainFSSandboxModeTypeToString(fs->sandbox)); if (fs->xattr == VIR_TRISTATE_SWITCH_ON) - virBufferAddLit(&opts, ",xattr"); - else if (fs->xattr == VIR_TRISTATE_SWITCH_OFF) - virBufferAddLit(&opts, ",no_xattr"); - - virCommandAddArgBuffer(cmd, &opts); + virCommandAddArg(cmd, "--xattr"); if (fs->thread_pool_size >= 0) virCommandAddArgFormat(cmd, "--thread-pool-size=%i", fs->thread_pool_size); if (cfg->virtiofsdDebug) - virCommandAddArg(cmd, "-d"); + virCommandAddArg(cmd, "--log-level=debug"); return g_steal_pointer(&cmd); } -- 2.41.0
participants (5)
-
Jim Fehlig -
Ján Tomko -
Michal Privoznik -
Michal Prívozník
-
Peter Krempa