5:50 a.m.
*** 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
5:50 a.m.
New subject: [PATCH 1/2] qemu: Drop support for C implementation of virtiofsd
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(a)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
6:07 a.m.
New subject: [PATCH 1/2] qemu: Drop support for C implementation of virtiofsd
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.
2:34 a.m.
New subject: [PATCH 1/2] qemu: Drop support for C implementation of virtiofsd
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
5:35 a.m.
New subject: [PATCH 1/2] qemu: Drop support for C implementation of virtiofsd
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(a)lists.libvirt.org
To unsubscribe send an email to devel-leave(a)lists.libvirt.org
6:14 p.m.
New subject: [PATCH 1/2] qemu: Drop support for C implementation of virtiofsd
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
5:50 a.m.
New subject: [PATCH 2/2] qemu_virtiofs: Don't use deprecated cmd line
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(a)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
345
days inactive
348
days old
6 comments
5 participants
participants (5)
-
Jim Fehlig -
Ján Tomko -
Michal Privoznik -
Michal Prívozník
-
Peter Krempa