Series comparison

-[PULL 0/2] Block patches
+[PULL for-8.1 0/2] Block patches
-The following changes since commit ca61fa4b803e5d0abaf6f1ceb690f23bb78a4def:
+The following changes since commit 9ba37026fcf6b7f3f096c0cca3e1e7307802486b:
-  Merge remote-tracking branch 'remotes/quic/tags/pull-hex-20211006' into staging (2021-10-06 12:11:14 -0700)
+  Update version for v8.1.0-rc2 release (2023-08-02 08:22:45 -0700)
 are available in the Git repository at:
   https://gitlab.com/stefanha/qemu.git tags/block-pull-request
-for you to fetch changes up to 1cc7eada97914f090125e588497986f6f7900514:
+for you to fetch changes up to 9b06d0d076271d76e5384d767ef94a676f0a9efd:
-  iothread: use IOThreadParamInfo in iothread_[set|get]_param() (2021-10-07 15:29:50 +0100)
+  block/blkio: add more comments on the fd passing handling (2023-08-03 11:28:43 -0400)
 ----------------------------------------------------------------
 Pull request
+Fix for an fd leak in the blkio block driver.
 ----------------------------------------------------------------
 Stefano Garzarella (2):
-  iothread: rename PollParamInfo to IOThreadParamInfo
+  block/blkio: close the fd when blkio_connect() fails
-  iothread: use IOThreadParamInfo in iothread_[set|get]_param()
+  block/blkio: add more comments on the fd passing handling
- iothread.c | 28 +++++++++++++++-------------
+ block/blkio.c | 28 +++++++++++++++++++++-------
-file changed, 15 insertions(+), 13 deletions(-)
+file changed, 21 insertions(+), 7 deletions(-)
 --
-.31.1
+.41.0

-[PULL 2/2] iothread: use IOThreadParamInfo in iothread_[set|get]_param()
+[PULL for-8.1 1/2] block/blkio: close the fd when blkio_connect() fails
 From: Stefano Garzarella <sgarzare@redhat.com>
-Commit 0445409d74 ("iothread: generalize
+libblkio drivers take ownership of `fd` only after a successful
-iothread_set_param/iothread_get_param") moved common code to set and
+blkio_connect(), so if it fails, we are still the owners.
 get IOThread parameters in two new functions.
-These functions are called inside callbacks, so we don't need to use an
+Fixes: cad2ccc395 ("block/blkio: use qemu_open() to support fd passing for virtio-blk")
-opaque pointer. Let's replace `void *opaque` parameter with
+Suggested-by: Hanna Czenczek <hreitz@redhat.com>
 `IOThreadParamInfo *info`.
 Suggested-by: Kevin Wolf <kwolf@redhat.com>
 Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
-Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
+Reviewed-by: Hanna Czenczek <hreitz@redhat.com>
-Message-id: 20210727145936.147032-3-sgarzare@redhat.com
+Message-id: 20230803082825.25293-2-sgarzare@redhat.com
 Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
 ---
- iothread.c | 18 ++++++++++--------
+ block/blkio.c | 11 ++++++++---
-file changed, 10 insertions(+), 8 deletions(-)
+file changed, 8 insertions(+), 3 deletions(-)
-diff --git a/iothread.c b/iothread.c
+diff --git a/block/blkio.c b/block/blkio.c
 index XXXXXXX..XXXXXXX 100644
---- a/iothread.c
+--- a/block/blkio.c
-+++ b/iothread.c
++++ b/block/blkio.c
-@@ -XXX,XX +XXX,XX @@ static IOThreadParamInfo aio_max_batch_info = {
+@@ -XXX,XX +XXX,XX @@ static int blkio_virtio_blk_connect(BlockDriverState *bs, QDict *options,
- };
+     const char *path = qdict_get_try_str(options, "path");
+     BDRVBlkioState *s = bs->opaque;
- static void iothread_get_param(Object *obj, Visitor *v,
+     bool fd_supported = false;
--        const char *name, void *opaque, Error **errp)
+-    int fd, ret;
-+        const char *name, IOThreadParamInfo *info, Error **errp)
++    int fd = -1, ret;
- {
-     IOThread *iothread = IOTHREAD(obj);
+     if (!path) {
--    IOThreadParamInfo *info = opaque;
+         error_setg(errp, "missing 'path' option");
-     int64_t *field = (void *)iothread + info->offset;
+@@ -XXX,XX +XXX,XX @@ static int blkio_virtio_blk_connect(BlockDriverState *bs, QDict *options,
+             if (ret < 0) {
-     visit_type_int64(v, name, field, errp);
+                 fd_supported = false;
- }
+                 qemu_close(fd);
++                fd = -1;
- static bool iothread_set_param(Object *obj, Visitor *v,
+             }
--        const char *name, void *opaque, Error **errp)
+         }
 +        const char *name, IOThreadParamInfo *info, Error **errp)
  {
      IOThread *iothread = IOTHREAD(obj);
 -    IOThreadParamInfo *info = opaque;
      int64_t *field = (void *)iothread + info->offset;
      int64_t value;
@@ -XXX,XX +XXX,XX @@ static bool iothread_set_param(Object *obj, Visitor *v,
  static void iothread_get_poll_param(Object *obj, Visitor *v,
          const char *name, void *opaque, Error **errp)
  {
 +    IOThreadParamInfo *info = opaque;
 -    iothread_get_param(obj, v, name, opaque, errp);
 +    iothread_get_param(obj, v, name, info, errp);
  }
  static void iothread_set_poll_param(Object *obj, Visitor *v,
          const char *name, void *opaque, Error **errp)
  {
      IOThread *iothread = IOTHREAD(obj);
 +    IOThreadParamInfo *info = opaque;
 -    if (!iothread_set_param(obj, v, name, opaque, errp)) {
 +    if (!iothread_set_param(obj, v, name, info, errp)) {
          return;
      }
+@@ -XXX,XX +XXX,XX @@ static int blkio_virtio_blk_connect(BlockDriverState *bs, QDict *options,
@@ -XXX,XX +XXX,XX @@ static void iothread_set_poll_param(Object *obj, Visitor *v,
  static void iothread_get_aio_param(Object *obj, Visitor *v,
          const char *name, void *opaque, Error **errp)
  {
 +    IOThreadParamInfo *info = opaque;
 -    iothread_get_param(obj, v, name, opaque, errp);
 +    iothread_get_param(obj, v, name, info, errp);
  }
  static void iothread_set_aio_param(Object *obj, Visitor *v,
          const char *name, void *opaque, Error **errp)
  {
      IOThread *iothread = IOTHREAD(obj);
 +    IOThreadParamInfo *info = opaque;
 -    if (!iothread_set_param(obj, v, name, opaque, errp)) {
 +    if (!iothread_set_param(obj, v, name, info, errp)) {
          return;
      }
+     ret = blkio_connect(s->blkio);
++    if (ret < 0 && fd >= 0) {
++        /* Failed to give the FD to libblkio, close it */
++        qemu_close(fd);
++        fd = -1;
++    }
++
+     /*
+      * If the libblkio driver doesn't support the `fd` property, blkio_connect()
+      * will fail with -EINVAL. So let's try calling blkio_connect() again by
+      * directly setting `path`.
+      */
+     if (fd_supported && ret == -EINVAL) {
+-        qemu_close(fd);
+-
+         /*
+          * We need to clear the `fd` property we set previously by setting
+          * it to -1.
 --
-.31.1
+.41.0

-[PULL 1/2] iothread: rename PollParamInfo to IOThreadParamInfo
+[PULL for-8.1 2/2] block/blkio: add more comments on the fd passing handling
 From: Stefano Garzarella <sgarzare@redhat.com>
-Commit 1793ad0247 ("iothread: add aio-max-batch parameter") added
+As Hanna pointed out, it is not clear in the code why qemu_open()
-a new parameter (aio-max-batch) to IOThread and used PollParamInfo
+can fail, and why blkio_set_int("fd") is not enough to discover
-structure to handle it.
+the `fd` property support.
-Since it is not a parameter of the polling mechanism, we rename the
+Let's fix them by adding more details in the code comments.
 structure to a more generic IOThreadParamInfo.
-Suggested-by: Kevin Wolf <kwolf@redhat.com>
+Suggested-by: Hanna Czenczek <hreitz@redhat.com>
 Reviewed-by: Hanna Czenczek <hreitz@redhat.com>
 Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
-Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
+Message-id: 20230803082825.25293-3-sgarzare@redhat.com
 Message-id: 20210727145936.147032-2-sgarzare@redhat.com
 Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
 ---
- iothread.c | 14 +++++++-------
+ block/blkio.c | 15 ++++++++++++---
-file changed, 7 insertions(+), 7 deletions(-)
+file changed, 12 insertions(+), 3 deletions(-)
-diff --git a/iothread.c b/iothread.c
+diff --git a/block/blkio.c b/block/blkio.c
 index XXXXXXX..XXXXXXX 100644
---- a/iothread.c
+--- a/block/blkio.c
-+++ b/iothread.c
++++ b/block/blkio.c
-@@ -XXX,XX +XXX,XX @@ static void iothread_complete(UserCreatable *obj, Error **errp)
+@@ -XXX,XX +XXX,XX @@ static int blkio_virtio_blk_connect(BlockDriverState *bs, QDict *options,
- typedef struct {
+          */
-     const char *name;
+         fd = qemu_open(path, O_RDWR, NULL);
-     ptrdiff_t offset; /* field's byte offset in IOThread struct */
+         if (fd < 0) {
--} PollParamInfo;
++            /*
-+} IOThreadParamInfo;
++             * qemu_open() can fail if the user specifies a path that is not
++             * a file or device, for example in the case of Unix Domain Socket
--static PollParamInfo poll_max_ns_info = {
++             * for the virtio-blk-vhost-user driver. In such cases let's have
-+static IOThreadParamInfo poll_max_ns_info = {
++             * libblkio open the path directly.
-     "poll-max-ns", offsetof(IOThread, poll_max_ns),
++             */
- };
+             fd_supported = false;
--static PollParamInfo poll_grow_info = {
+         } else {
-+static IOThreadParamInfo poll_grow_info = {
+             ret = blkio_set_int(s->blkio, "fd", fd);
-     "poll-grow", offsetof(IOThread, poll_grow),
+@@ -XXX,XX +XXX,XX @@ static int blkio_virtio_blk_connect(BlockDriverState *bs, QDict *options,
- };
+     }
--static PollParamInfo poll_shrink_info = {
-+static IOThreadParamInfo poll_shrink_info = {
+     /*
-     "poll-shrink", offsetof(IOThread, poll_shrink),
+-     * If the libblkio driver doesn't support the `fd` property, blkio_connect()
- };
+-     * will fail with -EINVAL. So let's try calling blkio_connect() again by
--static PollParamInfo aio_max_batch_info = {
+-     * directly setting `path`.
-+static IOThreadParamInfo aio_max_batch_info = {
++     * Before https://gitlab.com/libblkio/libblkio/-/merge_requests/208
-     "aio-max-batch", offsetof(IOThread, aio_max_batch),
++     * (libblkio <= v1.3.0), setting the `fd` property is not enough to check
- };
++     * whether the driver supports the `fd` property or not. In that case,
++     * blkio_connect() will fail with -EINVAL.
-@@ -XXX,XX +XXX,XX @@ static void iothread_get_param(Object *obj, Visitor *v,
++     * So let's try calling blkio_connect() again by directly setting `path`
-         const char *name, void *opaque, Error **errp)
++     * to cover this scenario.
- {
+      */
-     IOThread *iothread = IOTHREAD(obj);
+     if (fd_supported && ret == -EINVAL) {
--    PollParamInfo *info = opaque;
+         /*
 +    IOThreadParamInfo *info = opaque;
      int64_t *field = (void *)iothread + info->offset;
      visit_type_int64(v, name, field, errp);
@@ -XXX,XX +XXX,XX @@ static bool iothread_set_param(Object *obj, Visitor *v,
          const char *name, void *opaque, Error **errp)
  {
      IOThread *iothread = IOTHREAD(obj);
 -    PollParamInfo *info = opaque;
 +    IOThreadParamInfo *info = opaque;
      int64_t *field = (void *)iothread + info->offset;
      int64_t value;
 --
-.31.1
+.41.0

From: Stefano Garzarella <sgarzare@redhat.com>

Commit 1793ad0247 ("iothread: add aio-max-batch parameter") added
a new parameter (aio-max-batch) to IOThread and used PollParamInfo
structure to handle it.

Since it is not a parameter of the polling mechanism, we rename the
structure to a more generic IOThreadParamInfo.

Suggested-by: Kevin Wolf <kwolf@redhat.com>
Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Message-id: 20210727145936.147032-2-sgarzare@redhat.com
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 iothread.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/iothread.c b/iothread.c
index XXXXXXX..XXXXXXX 100644
--- a/iothread.c
+++ b/iothread.c
@@ -XXX,XX +XXX,XX @@ static void iothread_complete(UserCreatable *obj, Error **errp)
 typedef struct {
     const char *name;
     ptrdiff_t offset; /* field's byte offset in IOThread struct */
-} PollParamInfo;
+} IOThreadParamInfo;
 
-static PollParamInfo poll_max_ns_info = {
+static IOThreadParamInfo poll_max_ns_info = {
     "poll-max-ns", offsetof(IOThread, poll_max_ns),
 };
-static PollParamInfo poll_grow_info = {
+static IOThreadParamInfo poll_grow_info = {
     "poll-grow", offsetof(IOThread, poll_grow),
 };
-static PollParamInfo poll_shrink_info = {
+static IOThreadParamInfo poll_shrink_info = {
     "poll-shrink", offsetof(IOThread, poll_shrink),
 };
-static PollParamInfo aio_max_batch_info = {
+static IOThreadParamInfo aio_max_batch_info = {
     "aio-max-batch", offsetof(IOThread, aio_max_batch),
 };
 
@@ -XXX,XX +XXX,XX @@ static void iothread_get_param(Object *obj, Visitor *v,
         const char *name, void *opaque, Error **errp)
 {
     IOThread *iothread = IOTHREAD(obj);
-    PollParamInfo *info = opaque;
+    IOThreadParamInfo *info = opaque;
     int64_t *field = (void *)iothread + info->offset;
 
     visit_type_int64(v, name, field, errp);
@@ -XXX,XX +XXX,XX @@ static bool iothread_set_param(Object *obj, Visitor *v,
         const char *name, void *opaque, Error **errp)
 {
     IOThread *iothread = IOTHREAD(obj);
-    PollParamInfo *info = opaque;
+    IOThreadParamInfo *info = opaque;
     int64_t *field = (void *)iothread + info->offset;
     int64_t value;
 
-- 
2.31.1

From: Stefano Garzarella <sgarzare@redhat.com>

Commit 0445409d74 ("iothread: generalize
iothread_set_param/iothread_get_param") moved common code to set and
get IOThread parameters in two new functions.

These functions are called inside callbacks, so we don't need to use an
opaque pointer. Let's replace `void *opaque` parameter with
`IOThreadParamInfo *info`.

Suggested-by: Kevin Wolf <kwolf@redhat.com>
Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Message-id: 20210727145936.147032-3-sgarzare@redhat.com
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 iothread.c | 18 ++++++++++--------
 1 file changed, 10 insertions(+), 8 deletions(-)

diff --git a/iothread.c b/iothread.c
index XXXXXXX..XXXXXXX 100644
--- a/iothread.c
+++ b/iothread.c
@@ -XXX,XX +XXX,XX @@ static IOThreadParamInfo aio_max_batch_info = {
 };
 
 static void iothread_get_param(Object *obj, Visitor *v,
-        const char *name, void *opaque, Error **errp)
+        const char *name, IOThreadParamInfo *info, Error **errp)
 {
     IOThread *iothread = IOTHREAD(obj);
-    IOThreadParamInfo *info = opaque;
     int64_t *field = (void *)iothread + info->offset;
 
     visit_type_int64(v, name, field, errp);
 }
 
 static bool iothread_set_param(Object *obj, Visitor *v,
-        const char *name, void *opaque, Error **errp)
+        const char *name, IOThreadParamInfo *info, Error **errp)
 {
     IOThread *iothread = IOTHREAD(obj);
-    IOThreadParamInfo *info = opaque;
     int64_t *field = (void *)iothread + info->offset;
     int64_t value;
 
@@ -XXX,XX +XXX,XX @@ static bool iothread_set_param(Object *obj, Visitor *v,
 static void iothread_get_poll_param(Object *obj, Visitor *v,
         const char *name, void *opaque, Error **errp)
 {
+    IOThreadParamInfo *info = opaque;
 
-    iothread_get_param(obj, v, name, opaque, errp);
+    iothread_get_param(obj, v, name, info, errp);
 }
 
 static void iothread_set_poll_param(Object *obj, Visitor *v,
         const char *name, void *opaque, Error **errp)
 {
     IOThread *iothread = IOTHREAD(obj);
+    IOThreadParamInfo *info = opaque;
 
-    if (!iothread_set_param(obj, v, name, opaque, errp)) {
+    if (!iothread_set_param(obj, v, name, info, errp)) {
         return;
     }
 
@@ -XXX,XX +XXX,XX @@ static void iothread_set_poll_param(Object *obj, Visitor *v,
 static void iothread_get_aio_param(Object *obj, Visitor *v,
         const char *name, void *opaque, Error **errp)
 {
+    IOThreadParamInfo *info = opaque;
 
-    iothread_get_param(obj, v, name, opaque, errp);
+    iothread_get_param(obj, v, name, info, errp);
 }
 
 static void iothread_set_aio_param(Object *obj, Visitor *v,
         const char *name, void *opaque, Error **errp)
 {
     IOThread *iothread = IOTHREAD(obj);
+    IOThreadParamInfo *info = opaque;
 
-    if (!iothread_set_param(obj, v, name, opaque, errp)) {
+    if (!iothread_set_param(obj, v, name, info, errp)) {
         return;
     }
 
-- 
2.31.1

From: Stefano Garzarella <sgarzare@redhat.com>

libblkio drivers take ownership of `fd` only after a successful
blkio_connect(), so if it fails, we are still the owners.

Fixes: cad2ccc395 ("block/blkio: use qemu_open() to support fd passing for virtio-blk")
Suggested-by: Hanna Czenczek <hreitz@redhat.com>
Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
Reviewed-by: Hanna Czenczek <hreitz@redhat.com>
Message-id: 20230803082825.25293-2-sgarzare@redhat.com
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 block/blkio.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/block/blkio.c b/block/blkio.c
index XXXXXXX..XXXXXXX 100644
--- a/block/blkio.c
+++ b/block/blkio.c
@@ -XXX,XX +XXX,XX @@ static int blkio_virtio_blk_connect(BlockDriverState *bs, QDict *options,
     const char *path = qdict_get_try_str(options, "path");
     BDRVBlkioState *s = bs->opaque;
     bool fd_supported = false;
-    int fd, ret;
+    int fd = -1, ret;
 
     if (!path) {
         error_setg(errp, "missing 'path' option");
@@ -XXX,XX +XXX,XX @@ static int blkio_virtio_blk_connect(BlockDriverState *bs, QDict *options,
             if (ret < 0) {
                 fd_supported = false;
                 qemu_close(fd);
+                fd = -1;
             }
         }
     }
@@ -XXX,XX +XXX,XX @@ static int blkio_virtio_blk_connect(BlockDriverState *bs, QDict *options,
     }
 
     ret = blkio_connect(s->blkio);
+    if (ret < 0 && fd >= 0) {
+        /* Failed to give the FD to libblkio, close it */
+        qemu_close(fd);
+        fd = -1;
+    }
+
     /*
      * If the libblkio driver doesn't support the `fd` property, blkio_connect()
      * will fail with -EINVAL. So let's try calling blkio_connect() again by
      * directly setting `path`.
      */
     if (fd_supported && ret == -EINVAL) {
-        qemu_close(fd);
-
         /*
          * We need to clear the `fd` property we set previously by setting
          * it to -1.
-- 
2.41.0

From: Stefano Garzarella <sgarzare@redhat.com>

As Hanna pointed out, it is not clear in the code why qemu_open()
can fail, and why blkio_set_int("fd") is not enough to discover
the `fd` property support.

Let's fix them by adding more details in the code comments.

Suggested-by: Hanna Czenczek <hreitz@redhat.com>
Reviewed-by: Hanna Czenczek <hreitz@redhat.com>
Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
Message-id: 20230803082825.25293-3-sgarzare@redhat.com
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 block/blkio.c | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

diff --git a/block/blkio.c b/block/blkio.c
index XXXXXXX..XXXXXXX 100644
--- a/block/blkio.c
+++ b/block/blkio.c
@@ -XXX,XX +XXX,XX @@ static int blkio_virtio_blk_connect(BlockDriverState *bs, QDict *options,
          */
         fd = qemu_open(path, O_RDWR, NULL);
         if (fd < 0) {
+            /*
+             * qemu_open() can fail if the user specifies a path that is not
+             * a file or device, for example in the case of Unix Domain Socket
+             * for the virtio-blk-vhost-user driver. In such cases let's have
+             * libblkio open the path directly.
+             */
             fd_supported = false;
         } else {
             ret = blkio_set_int(s->blkio, "fd", fd);
@@ -XXX,XX +XXX,XX @@ static int blkio_virtio_blk_connect(BlockDriverState *bs, QDict *options,
     }
 
     /*
-     * If the libblkio driver doesn't support the `fd` property, blkio_connect()
-     * will fail with -EINVAL. So let's try calling blkio_connect() again by
-     * directly setting `path`.
+     * Before https://gitlab.com/libblkio/libblkio/-/merge_requests/208
+     * (libblkio <= v1.3.0), setting the `fd` property is not enough to check
+     * whether the driver supports the `fd` property or not. In that case,
+     * blkio_connect() will fail with -EINVAL.
+     * So let's try calling blkio_connect() again by directly setting `path`
+     * to cover this scenario.
      */
     if (fd_supported && ret == -EINVAL) {
         /*
-- 
2.41.0