[PATCH] qemu: domfsinfo should print a target name on s390x, not a complete device path

Thomas Huth posted 1 patch 3 years, 4 months ago
Test syntax-check failed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/libvirt tags/patchew/20201112224802.784132-1-thuth@redhat.com
src/qemu/qemu_driver.c | 26 +++++++++++++++++++++++++-
1 file changed, 25 insertions(+), 1 deletion(-)
[PATCH] qemu: domfsinfo should print a target name on s390x, not a complete device path
Posted by Thomas Huth 3 years, 4 months ago
While fixing domfsinfo for non-PCI (i.e. CCW) devices on s390x,
I accidentally used the whole device path for the devAlias field.
However, it should only contain the base target name.

Currently we have the wrong output:

 $ virsh domfsinfo guestname
  Mountpoint   Name   Type   Target
 ---------------------------------------
  /            sda3   xfs    /dev/sda3
  /boot        sda1   xfs    /dev/sda1

It should look like this instead:

 $ virsh domfsinfo guestname
  Mountpoint   Name   Type   Target
 ------------------------------------
  /            sda3   xfs    sda
  /boot        sda1   xfs    sda

Thus we have to strip the "/dev/" prefix and the partition number
from the string.

Fixes: f8333b3b0a ("qemu: Fix domfsinfo for non-PCI device information ...")
Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1858771
Reported-by: Sebastian Mitterle <smitterl@redhat.com>
---
 src/qemu/qemu_driver.c | 26 +++++++++++++++++++++++++-
 1 file changed, 25 insertions(+), 1 deletion(-)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 05f8eb2cb7..d92bee1d35 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -18874,6 +18874,30 @@ qemuDomainGetFSInfoAgent(virQEMUDriverPtr driver,
     return ret;
 }
 
+/* Turn device node string like "/dev/vda1" into a target name like "vda" */
+static char *
+qemuAgentDevNodeToTarget(const char *devnode)
+{
+    char *str = g_strdup(devnode);
+    size_t len = strlen(str);
+
+    /* Remove the "/dev/" prefix from the string */
+    if (g_str_has_prefix(str, "/dev/")) {
+        len -= 5;
+        memmove(str, str + 5, len + 1);
+    }
+
+    /* Remove the partition number from the end of the string */
+    while (len > 0) {
+        len--;
+        if (!g_ascii_isdigit(str[len]))
+            break;
+        str[len] = 0;
+    }
+
+    return str;
+}
+
 static virDomainFSInfoPtr
 qemuAgentFSInfoToPublic(qemuAgentFSInfoPtr agent,
                         virDomainDefPtr vmdef)
@@ -18903,7 +18927,7 @@ qemuAgentFSInfoToPublic(qemuAgentFSInfoPtr agent,
         if (diskDef != NULL)
             ret->devAlias[i] = g_strdup(diskDef->dst);
         else if (agentdisk->devnode != NULL)
-            ret->devAlias[i] = g_strdup(agentdisk->devnode);
+            ret->devAlias[i] = qemuAgentDevNodeToTarget(agentdisk->devnode);
         else
             VIR_DEBUG("Missing devnode name for '%s'.", ret->mountpoint);
     }
-- 
2.18.4

Re: [PATCH] qemu: domfsinfo should print a target name on s390x, not a complete device path
Posted by Peter Krempa 3 years, 4 months ago
On Thu, Nov 12, 2020 at 23:48:02 +0100, Thomas Huth wrote:
> While fixing domfsinfo for non-PCI (i.e. CCW) devices on s390x,
> I accidentally used the whole device path for the devAlias field.
> However, it should only contain the base target name.
> 
> Currently we have the wrong output:
> 
>  $ virsh domfsinfo guestname
>   Mountpoint   Name   Type   Target
>  ---------------------------------------
>   /            sda3   xfs    /dev/sda3
>   /boot        sda1   xfs    /dev/sda1
> 
> It should look like this instead:
> 
>  $ virsh domfsinfo guestname
>   Mountpoint   Name   Type   Target
>  ------------------------------------
>   /            sda3   xfs    sda
>   /boot        sda1   xfs    sda
> 
> Thus we have to strip the "/dev/" prefix and the partition number
> from the string.

This is not the correct approach. While our "target" looks suspiciously
close to the guest /dev/name it can't be guaranteed to be the same.

Stripping prefix and suffix may work in many cases, but if you want to
uphold what's written in the API docs this won't be always correct.

I think we need to just relax the documentation of 'virsh domfsinfo' and
report the path as we do now if the s390x platform is unable to report
back any information which would allow us to match the device otherwise.

> 
> Fixes: f8333b3b0a ("qemu: Fix domfsinfo for non-PCI device information ...")
> Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1858771
> Reported-by: Sebastian Mitterle <smitterl@redhat.com>
> ---
>  src/qemu/qemu_driver.c | 26 +++++++++++++++++++++++++-
>  1 file changed, 25 insertions(+), 1 deletion(-)
> 
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index 05f8eb2cb7..d92bee1d35 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -18874,6 +18874,30 @@ qemuDomainGetFSInfoAgent(virQEMUDriverPtr driver,
>      return ret;
>  }
>  
> +/* Turn device node string like "/dev/vda1" into a target name like "vda" */
> +static char *
> +qemuAgentDevNodeToTarget(const char *devnode)
> +{
> +    char *str = g_strdup(devnode);
> +    size_t len = strlen(str);
> +
> +    /* Remove the "/dev/" prefix from the string */
> +    if (g_str_has_prefix(str, "/dev/")) {
> +        len -= 5;
> +        memmove(str, str + 5, len + 1);
> +    }
> +
> +    /* Remove the partition number from the end of the string */
> +    while (len > 0) {
> +        len--;
> +        if (!g_ascii_isdigit(str[len]))
> +            break;
> +        str[len] = 0;

Note that the guest-reported partition name does not necessarily match
what's configured in the domain XML, so this algorithm is not correct.


> +    }
> +
> +    return str;
> +}
> +
>  static virDomainFSInfoPtr
>  qemuAgentFSInfoToPublic(qemuAgentFSInfoPtr agent,
>                          virDomainDefPtr vmdef)
> @@ -18903,7 +18927,7 @@ qemuAgentFSInfoToPublic(qemuAgentFSInfoPtr agent,
>          if (diskDef != NULL)
>              ret->devAlias[i] = g_strdup(diskDef->dst);
>          else if (agentdisk->devnode != NULL)
> -            ret->devAlias[i] = g_strdup(agentdisk->devnode);
> +            ret->devAlias[i] = qemuAgentDevNodeToTarget(agentdisk->devnode);
>          else
>              VIR_DEBUG("Missing devnode name for '%s'.", ret->mountpoint);
>      }
> -- 
> 2.18.4
> 

Re: [PATCH] qemu: domfsinfo should print a target name on s390x, not a complete device path
Posted by Thomas Huth 3 years, 4 months ago
On 13/11/2020 08.21, Peter Krempa wrote:
> On Thu, Nov 12, 2020 at 23:48:02 +0100, Thomas Huth wrote:
>> While fixing domfsinfo for non-PCI (i.e. CCW) devices on s390x,
>> I accidentally used the whole device path for the devAlias field.
>> However, it should only contain the base target name.
>>
>> Currently we have the wrong output:
>>
>>  $ virsh domfsinfo guestname
>>   Mountpoint   Name   Type   Target
>>  ---------------------------------------
>>   /            sda3   xfs    /dev/sda3
>>   /boot        sda1   xfs    /dev/sda1
>>
>> It should look like this instead:
>>
>>  $ virsh domfsinfo guestname
>>   Mountpoint   Name   Type   Target
>>  ------------------------------------
>>   /            sda3   xfs    sda
>>   /boot        sda1   xfs    sda
>>
>> Thus we have to strip the "/dev/" prefix and the partition number
>> from the string.
> 
> This is not the correct approach. While our "target" looks suspiciously
> close to the guest /dev/name it can't be guaranteed to be the same.

Ouch, ok, got it now. On x86, I can apparently say:

 <target dev='vdc' bus='virtio'/>

for my first disk of my guest, and domfsinfo then shows up like this:

Mountpoint                   Name     Type     Target
-----------------------------------------------------------
/                            dm-0     xfs      vdc
/boot                        vda1     xfs      vdc

... very confusing if you're not used to it ;-)

> Stripping prefix and suffix may work in many cases, but if you want to
> uphold what's written in the API docs this won't be always correct.
> 
> I think we need to just relax the documentation of 'virsh domfsinfo' and
> report the path as we do now if the s390x platform is unable to report
> back any information which would allow us to match the device otherwise.

Ok, I'll ponder about this for a while - maybe there's a way to fix this on
s390x ... otherwise I'll consider sending a patch to update the docs.

 Thanks,
  Thomas