| Message ID | 20230801160332.122564-2-sgarzare@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | block/blkio: fix fd leak and add more comments for the fd passing | expand |
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); - ...
On Wed, Aug 02, 2023 at 01:15:40PM +0200, Hanna Czenczek wrote: >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); >+} I like this, I'll do it in v2! Thanks, Stefano
diff --git a/block/blkio.c b/block/blkio.c index 8e7ce42c79..2d53a865e7 100644 --- a/block/blkio.c +++ b/block/blkio.c @@ -739,6 +739,7 @@ static int blkio_virtio_blk_connect(BlockDriverState *bs, QDict *options, * directly setting `path`. */ if (fd_supported && ret == -EINVAL) { + fd_supported = false; qemu_close(fd); /* @@ -763,6 +764,14 @@ static int blkio_virtio_blk_connect(BlockDriverState *bs, QDict *options, } if (ret < 0) { + if (fd_supported) { + /* + * libblkio drivers take ownership of `fd` only after a successful + * blkio_connect(), so if it fails, we are still the owners. + */ + qemu_close(fd); + } + error_setg_errno(errp, -ret, "blkio_connect failed: %s", blkio_get_error_msg()); return ret;
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(+)