1. Patchew
  2. Libvirt
  3. View series

[PATCH] qemu: virtiofs: make fs->sock guard more consistent

Cole Robinson via Devel posted 1 patch 1 week, 5 days ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/libvirt tags/patchew/2269966ba32c15f57045851938d7471e47151f76.1775145518.git.crobinso@redhat.com
src/qemu/qemu_extdevice.c |  8 +++-----
src/qemu/qemu_hotplug.c   | 18 ++++++++----------
src/qemu/qemu_virtiofs.c  |  9 +++++++++
3 files changed, 20 insertions(+), 15 deletions(-)
[PATCH] qemu: virtiofs: make fs->sock guard more consistent
Posted by Cole Robinson via Devel 1 week, 5 days ago 
When fs->sock is set, virtiofs is externally managed, and most
of qemu_virtiofs.c should do nothing.

Some qemu_virtiofs.c functions handle this case, but some require the
caller to guard against it.

Standardize on making this the responsibility of qemu_virtiofs.c

Signed-off-by: Cole Robinson <crobinso@redhat.com>
---
 src/qemu/qemu_extdevice.c |  8 +++-----
 src/qemu/qemu_hotplug.c   | 18 ++++++++----------
 src/qemu/qemu_virtiofs.c  |  9 +++++++++
 3 files changed, 20 insertions(+), 15 deletions(-)

diff --git a/src/qemu/qemu_extdevice.c b/src/qemu/qemu_extdevice.c
index 28cea52980..6e16c6be29 100644
--- a/src/qemu/qemu_extdevice.c
+++ b/src/qemu/qemu_extdevice.c
@@ -225,7 +225,7 @@ qemuExtDevicesStart(virQEMUDriver *driver,
     for (i = 0; i < def->nfss; i++) {
         virDomainFSDef *fs = def->fss[i];
 
-        if (fs->fsdriver == VIR_DOMAIN_FS_DRIVER_TYPE_VIRTIOFS && !fs->sock) {
+        if (fs->fsdriver == VIR_DOMAIN_FS_DRIVER_TYPE_VIRTIOFS) {
             if (qemuVirtioFSStart(driver, vm, fs) < 0)
                 return -1;
         }
@@ -315,8 +315,7 @@ qemuExtDevicesStop(virQEMUDriver *driver,
     for (i = 0; i < def->nfss; i++) {
         virDomainFSDef *fs = def->fss[i];
 
-        if (!fs->sock &&
-            fs->fsdriver == VIR_DOMAIN_FS_DRIVER_TYPE_VIRTIOFS)
+        if (fs->fsdriver == VIR_DOMAIN_FS_DRIVER_TYPE_VIRTIOFS)
             qemuVirtioFSStop(driver, vm, fs);
     }
 
@@ -473,8 +472,7 @@ qemuExtDevicesSetupCgroup(virQEMUDriver *driver,
     for (i = 0; i < def->nfss; i++) {
         virDomainFSDef *fs = def->fss[i];
 
-        if (!fs->sock &&
-            fs->fsdriver == VIR_DOMAIN_FS_DRIVER_TYPE_VIRTIOFS &&
+        if (fs->fsdriver == VIR_DOMAIN_FS_DRIVER_TYPE_VIRTIOFS &&
             qemuVirtioFSSetupCgroup(vm, fs, cgroup) < 0)
             return -1;
     }
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index 9439948089..deb02c20be 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -3361,17 +3361,15 @@ qemuDomainAttachFSDevice(virQEMUDriver *driver,
     if (!(devprops = qemuBuildVHostUserFsDevProps(fs, vm->def, charAlias, priv)))
         goto cleanup;
 
-    if (!fs->sock) {
-        if (qemuVirtioFSPrepareDomain(driver, fs) < 0)
-            goto cleanup;
+    if (qemuVirtioFSPrepareDomain(driver, fs) < 0)
+        goto cleanup;
 
-        if (qemuVirtioFSStart(driver, vm, fs) < 0)
-            goto cleanup;
-        started = true;
+    if (qemuVirtioFSStart(driver, vm, fs) < 0)
+        goto cleanup;
+    started = true;
 
-        if (qemuVirtioFSSetupCgroup(vm, fs, priv->cgroup) < 0)
-            goto cleanup;
-    }
+    if (qemuVirtioFSSetupCgroup(vm, fs, priv->cgroup) < 0)
+        goto cleanup;
 
     qemuDomainObjEnterMonitor(vm);
 
@@ -5506,7 +5504,7 @@ qemuDomainRemoveFSDevice(virQEMUDriver *driver,
     if (rc < 0)
         return -1;
 
-    if (!fs->sock && fs->fsdriver == VIR_DOMAIN_FS_DRIVER_TYPE_VIRTIOFS)
+    if (fs->fsdriver == VIR_DOMAIN_FS_DRIVER_TYPE_VIRTIOFS)
         qemuVirtioFSStop(driver, vm, fs);
 
     if ((idx = virDomainFSDefFind(vm->def, fs)) >= 0)
diff --git a/src/qemu/qemu_virtiofs.c b/src/qemu/qemu_virtiofs.c
index 91d58703f1..cbc3bc0bc2 100644
--- a/src/qemu/qemu_virtiofs.c
+++ b/src/qemu/qemu_virtiofs.c
@@ -228,6 +228,9 @@ qemuVirtioFSStart(virQEMUDriver *driver,
     g_autoptr(domainLogContext) logContext = NULL;
     int rc;
 
+    if (fs->sock)
+        return 0;
+
     if (!virFileIsExecutable(fs->binary)) {
         virReportError(VIR_ERR_INTERNAL_ERROR,
                        _("virtiofsd binary '%1$s' is not executable"),
@@ -327,6 +330,9 @@ qemuVirtioFSStop(virQEMUDriver *driver G_GNUC_UNUSED,
     g_autofree char *pidfile = NULL;
     virErrorPtr orig_err;
 
+    if (fs->sock)
+        return;
+
     virErrorPreserveLast(&orig_err);
 
     if (!(pidfile = qemuVirtioFSCreatePidFilename(vm, fs->info.alias)))
@@ -361,6 +367,9 @@ qemuVirtioFSSetupCgroup(virDomainObj *vm,
     pid_t pid = -1;
     int rc;
 
+    if (fs->sock)
+        return 0;
+
     if (!cgroup)
         return 0;
 
-- 
2.53.0
Re: [PATCH] qemu: virtiofs: make fs->sock guard more consistent
Posted by Peter Krempa via Devel 1 week ago 
On Thu, Apr 02, 2026 at 11:58:38 -0400, Cole Robinson via Devel wrote:
> When fs->sock is set, virtiofs is externally managed, and most
> of qemu_virtiofs.c should do nothing.
> 
> Some qemu_virtiofs.c functions handle this case, but some require the
> caller to guard against it.
> 
> Standardize on making this the responsibility of qemu_virtiofs.c
> 
> Signed-off-by: Cole Robinson <crobinso@redhat.com>
> ---
>  src/qemu/qemu_extdevice.c |  8 +++-----
>  src/qemu/qemu_hotplug.c   | 18 ++++++++----------
>  src/qemu/qemu_virtiofs.c  |  9 +++++++++
>  3 files changed, 20 insertions(+), 15 deletions(-)

[...]

> diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
> index 9439948089..deb02c20be 100644
> --- a/src/qemu/qemu_hotplug.c
> +++ b/src/qemu/qemu_hotplug.c
> @@ -3361,17 +3361,15 @@ qemuDomainAttachFSDevice(virQEMUDriver *driver,
>      if (!(devprops = qemuBuildVHostUserFsDevProps(fs, vm->def, charAlias, priv)))
>          goto cleanup;
>  
> -    if (!fs->sock) {
> -        if (qemuVirtioFSPrepareDomain(driver, fs) < 0)
> -            goto cleanup;
> +    if (qemuVirtioFSPrepareDomain(driver, fs) < 0)
> +        goto cleanup;
>  
> -        if (qemuVirtioFSStart(driver, vm, fs) < 0)
> -            goto cleanup;
> -        started = true;
> +    if (qemuVirtioFSStart(driver, vm, fs) < 0)
> +        goto cleanup;
> +    started = true;

While this doesn't change the actual result, the 'started' flag which is
present here without any additional documentation will be misleading.

Either add a comment stating that it may not be actually started or
rename the variable to something more self-explanatory.

>  
> -        if (qemuVirtioFSSetupCgroup(vm, fs, priv->cgroup) < 0)
> -            goto cleanup;
> -    }
> +    if (qemuVirtioFSSetupCgroup(vm, fs, priv->cgroup) < 0)
> +        goto cleanup;
>  
>      qemuDomainObjEnterMonitor(vm);

Reviewed-by: Peter Krempa <pkrempa@redhat.com>
Re: [PATCH] qemu: virtiofs: make fs->sock guard more consistent
Posted by Cole Robinson via Devel 1 week ago 
On 4/7/26 11:06 AM, Peter Krempa wrote:
> On Thu, Apr 02, 2026 at 11:58:38 -0400, Cole Robinson via Devel wrote:
>> When fs->sock is set, virtiofs is externally managed, and most
>> of qemu_virtiofs.c should do nothing.
>>
>> Some qemu_virtiofs.c functions handle this case, but some require the
>> caller to guard against it.
>>
>> Standardize on making this the responsibility of qemu_virtiofs.c
>>
>> Signed-off-by: Cole Robinson <crobinso@redhat.com>
>> ---
>>  src/qemu/qemu_extdevice.c |  8 +++-----
>>  src/qemu/qemu_hotplug.c   | 18 ++++++++----------
>>  src/qemu/qemu_virtiofs.c  |  9 +++++++++
>>  3 files changed, 20 insertions(+), 15 deletions(-)
> 
> [...]
> 
>> diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
>> index 9439948089..deb02c20be 100644
>> --- a/src/qemu/qemu_hotplug.c
>> +++ b/src/qemu/qemu_hotplug.c
>> @@ -3361,17 +3361,15 @@ qemuDomainAttachFSDevice(virQEMUDriver *driver,
>>      if (!(devprops = qemuBuildVHostUserFsDevProps(fs, vm->def, charAlias, priv)))
>>          goto cleanup;
>>  
>> -    if (!fs->sock) {
>> -        if (qemuVirtioFSPrepareDomain(driver, fs) < 0)
>> -            goto cleanup;
>> +    if (qemuVirtioFSPrepareDomain(driver, fs) < 0)
>> +        goto cleanup;
>>  
>> -        if (qemuVirtioFSStart(driver, vm, fs) < 0)
>> -            goto cleanup;
>> -        started = true;
>> +    if (qemuVirtioFSStart(driver, vm, fs) < 0)
>> +        goto cleanup;
>> +    started = true;
> 
> While this doesn't change the actual result, the 'started' flag which is
> present here without any additional documentation will be misleading.
> 
> Either add a comment stating that it may not be actually started or
> rename the variable to something more self-explanatory.
> 
>>  
>> -        if (qemuVirtioFSSetupCgroup(vm, fs, priv->cgroup) < 0)
>> -            goto cleanup;
>> -    }
>> +    if (qemuVirtioFSSetupCgroup(vm, fs, priv->cgroup) < 0)
>> +        goto cleanup;
>>  
>>      qemuDomainObjEnterMonitor(vm);
> 
> Reviewed-by: Peter Krempa <pkrempa@redhat.com>
> 

I renamed `started` to `startCalled`, and pushed

Thanks,
Cole

Patchew 2.1-devel-ea86937

© 2016 - 2026 Red Hat, Inc.