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
_______________________________________________
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-leave@lists.libvirt.org

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.
_______________________________________________
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-leave@lists.libvirt.org

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
_______________________________________________
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-leave@lists.libvirt.org

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
_______________________________________________
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
_______________________________________________
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-leave@lists.libvirt.org