Re: [PATCH 1/2] block/blkio: close the fd when blkio_connect() fails

[Hanna Czenczek]

On 01.08.23 18:03, Stefano Garzarella wrote:
> 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>
> ---
>   block/blkio.c | 9 +++++++++
>   1 file changed, 9 insertions(+)

Works, so:

Reviewed-by: Hanna Czenczek <hreitz@redhat.com>


But personally, instead of having `fd_supported` track whether we have a 
valid FD or not, I’d find it more intuitive to track ownership through 
the `fd` variable itself, i.e. initialize it to -1, and set it -1 when 
ownership is transferred, and then close it once we don’t need it 
anymore, but failed to transfer ownership to blkio.  The elaborate way 
would be something like

...
-int fd, ret;
+int fd = -1;
+int ret;
...
 ret = blkio_connect(s->blkio);
+if (!ret) {
+    /* If we had an FD, libblkio now has ownership of it */
+    fd = -1;
+}
+if (fd >= 0) {
+    /* We still have FD ownership, but no longer need it, so close it */
+    qemu_close(fd);
+    fd = -1;
+}
 /*
  * [...]
  */
 if (fd_supported && ret == -EINVAL) {
-    qemu_close(fd);
-
...


Or the shorter less-verbose version would be:

...
-int fd, ret;
+int fd = -1;
+int ret;
...
 ret = blkio_connect(s->blkio);
+if (fd >= 0 && ret < 0) {
+    /* Failed to give the FD to libblkio, close it */
+    qemu_close(fd);
+}
 /*
  * [...]
  */
 if (fd_supported && ret == -EINVAL) {
-    qemu_close(fd);
-
...