[PATCH v2] block/file-posix: fix g_file_get_contents return path

[PATCH v2] block/file-posix: fix g_file_get_contents return path

[Sam Li]

The g_file_get_contents() function returns a g_boolean. If it fails, the
returned value will be 0 instead of -1. Solve the issue by skipping
assigning ret value.

This issue was found by Matthew Rosato using virtio-blk-{pci,ccw} backed
by an NVMe partition e.g. /dev/nvme0n1p1 on s390x.

Signed-off-by: Sam Li <faithilikerun@gmail.com>
Reviewed-by: Matthew Rosato <mjrosato@linux.ibm.com>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 block/file-posix.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/block/file-posix.c b/block/file-posix.c
index 9e8e3d8ca5..b16e9c21a1 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -1232,7 +1232,6 @@ static int hdev_get_max_hw_transfer(int fd, struct stat *st)
 static int get_sysfs_str_val(struct stat *st, const char *attribute,
                              char **val) {
     g_autofree char *sysfspath = NULL;
-    int ret;
     size_t len;
 
     if (!S_ISBLK(st->st_mode)) {
@@ -1242,8 +1241,7 @@ static int get_sysfs_str_val(struct stat *st, const char *attribute,
     sysfspath = g_strdup_printf("/sys/dev/block/%u:%u/queue/%s",
                                 major(st->st_rdev), minor(st->st_rdev),
                                 attribute);
-    ret = g_file_get_contents(sysfspath, val, &len, NULL);
-    if (ret == -1) {
+    if (!g_file_get_contents(sysfspath, val, &len, NULL)) {
         return -ENOENT;
     }
 
@@ -1253,7 +1251,7 @@ static int get_sysfs_str_val(struct stat *st, const char *attribute,
     if (*(p + len - 1) == '\n') {
         *(p + len - 1) = '\0';
     }
-    return ret;
+    return 0;
 }
 #endif
 
-- 
2.40.1

Re: [PATCH v2] block/file-posix: fix g_file_get_contents return path

[Daniel P. Berrangé]

On Thu, Jul 27, 2023 at 07:58:44PM +0800, Sam Li wrote:
> The g_file_get_contents() function returns a g_boolean. If it fails, the
> returned value will be 0 instead of -1. Solve the issue by skipping
> assigning ret value.
> 
> This issue was found by Matthew Rosato using virtio-blk-{pci,ccw} backed
> by an NVMe partition e.g. /dev/nvme0n1p1 on s390x.
> 
> Signed-off-by: Sam Li <faithilikerun@gmail.com>
> Reviewed-by: Matthew Rosato <mjrosato@linux.ibm.com>
> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  block/file-posix.c | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>

We should put this in 8.1 rc2 IMHO

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH v2] block/file-posix: fix g_file_get_contents return path

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 715 bytes --]

On Thu, Jul 27, 2023 at 07:58:44PM +0800, Sam Li wrote:
> The g_file_get_contents() function returns a g_boolean. If it fails, the
> returned value will be 0 instead of -1. Solve the issue by skipping
> assigning ret value.
> 
> This issue was found by Matthew Rosato using virtio-blk-{pci,ccw} backed
> by an NVMe partition e.g. /dev/nvme0n1p1 on s390x.
> 
> Signed-off-by: Sam Li <faithilikerun@gmail.com>
> Reviewed-by: Matthew Rosato <mjrosato@linux.ibm.com>
> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  block/file-posix.c | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)

Thanks, applied to my block tree:
https://gitlab.com/stefanha/qemu/commits/block

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v2] block/file-posix: fix g_file_get_contents return path

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 883 bytes --]

On Thu, Jul 27, 2023 at 01:15:32PM +0100, Daniel P. Berrangé wrote:
> On Thu, Jul 27, 2023 at 07:58:44PM +0800, Sam Li wrote:
> > The g_file_get_contents() function returns a g_boolean. If it fails, the
> > returned value will be 0 instead of -1. Solve the issue by skipping
> > assigning ret value.
> > 
> > This issue was found by Matthew Rosato using virtio-blk-{pci,ccw} backed
> > by an NVMe partition e.g. /dev/nvme0n1p1 on s390x.
> > 
> > Signed-off-by: Sam Li <faithilikerun@gmail.com>
> > Reviewed-by: Matthew Rosato <mjrosato@linux.ibm.com>
> > Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
> > ---
> >  block/file-posix.c | 6 ++----
> >  1 file changed, 2 insertions(+), 4 deletions(-)
> 
> Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
> 
> We should put this in 8.1 rc2 IMHO

Agreed, I will send a block pull request for 8.1.

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]