[PATCH] block-backend: Fix argument order when calling 'qapi_event_send_block_io_error()'

[PATCH] block-backend: Fix argument order when calling 'qapi_event_send_block_io_error()'

[Peter Krempa]

Commit 7452162adec25c10 introduced 'qom-path' argument to BLOCK_IO_ERROR
event but when the event is instantiated in 'send_qmp_error_event()' the
arguments for 'device' and 'qom_path' in
qapi_event_send_block_io_error() were reversed :

Generated code for sending event:

  void qapi_event_send_block_io_error(const char *qom_path,
                                      const char *device,
                                      const char *node_name,
                                      IoOperationType operation,
                                      [...]

Call inside send_qmp_error_event():

     qapi_event_send_block_io_error(blk_name(blk),
                                    blk_get_attached_dev_path(blk),
                                    bs ? bdrv_get_node_name(bs) : NULL, optype,
                                    [...]

This results into reporting the QOM path as the device alias and vice
versa which in turn breaks libvirt, which expects the device alias being
either a valid alias or empty (which would make libvirt do the lookup by
node-name instead).

Fixes: 7452162adec25c1003d5bf0079aca52913a80e0c
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
---
 block/block-backend.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/block/block-backend.c b/block/block-backend.c
index c93a7525ad..981be67e5d 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -2136,8 +2136,8 @@ static void send_qmp_error_event(BlockBackend *blk,
     BlockDriverState *bs = blk_bs(blk);

     optype = is_read ? IO_OPERATION_TYPE_READ : IO_OPERATION_TYPE_WRITE;
-    qapi_event_send_block_io_error(blk_name(blk),
-                                   blk_get_attached_dev_path(blk),
+    qapi_event_send_block_io_error(blk_get_attached_dev_path(blk),
+                                   blk_name(blk),
                                    bs ? bdrv_get_node_name(bs) : NULL, optype,
                                    action, blk_iostatus_is_enabled(blk),
                                    error == ENOSPC, strerror(error));
-- 
2.48.1

Re: [PATCH] block-backend: Fix argument order when calling 'qapi_event_send_block_io_error()'

[Daniel P. Berrangé]

On Mon, Jan 27, 2025 at 11:29:24AM +0100, Peter Krempa wrote:
> Commit 7452162adec25c10 introduced 'qom-path' argument to BLOCK_IO_ERROR
> event but when the event is instantiated in 'send_qmp_error_event()' the
> arguments for 'device' and 'qom_path' in
> qapi_event_send_block_io_error() were reversed :
> 
> Generated code for sending event:
> 
>   void qapi_event_send_block_io_error(const char *qom_path,
>                                       const char *device,
>                                       const char *node_name,
>                                       IoOperationType operation,
>                                       [...]
> 
> Call inside send_qmp_error_event():
> 
>      qapi_event_send_block_io_error(blk_name(blk),
>                                     blk_get_attached_dev_path(blk),
>                                     bs ? bdrv_get_node_name(bs) : NULL, optype,
>                                     [...]
> 
> This results into reporting the QOM path as the device alias and vice
> versa which in turn breaks libvirt, which expects the device alias being
> either a valid alias or empty (which would make libvirt do the lookup by
> node-name instead).
> 
> Fixes: 7452162adec25c1003d5bf0079aca52913a80e0c
> Signed-off-by: Peter Krempa <pkrempa@redhat.com>

Hmm, luckily that was only introduced in 9.2.0, so we can probably just
fix it in stable, and not worry about libvirt needs to add back compat
to workaround the initially broken 9.2.0 release events.

> ---
>  block/block-backend.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)

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

> 
> diff --git a/block/block-backend.c b/block/block-backend.c
> index c93a7525ad..981be67e5d 100644
> --- a/block/block-backend.c
> +++ b/block/block-backend.c
> @@ -2136,8 +2136,8 @@ static void send_qmp_error_event(BlockBackend *blk,
>      BlockDriverState *bs = blk_bs(blk);
> 
>      optype = is_read ? IO_OPERATION_TYPE_READ : IO_OPERATION_TYPE_WRITE;
> -    qapi_event_send_block_io_error(blk_name(blk),
> -                                   blk_get_attached_dev_path(blk),
> +    qapi_event_send_block_io_error(blk_get_attached_dev_path(blk),
> +                                   blk_name(blk),
>                                     bs ? bdrv_get_node_name(bs) : NULL, optype,
>                                     action, blk_iostatus_is_enabled(blk),
>                                     error == ENOSPC, strerror(error));
> -- 
> 2.48.1
> 
> 

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] block-backend: Fix argument order when calling 'qapi_event_send_block_io_error()'

[Peter Krempa]

On Mon, Jan 27, 2025 at 10:53:16 +0000, Daniel P. Berrangé wrote:
> On Mon, Jan 27, 2025 at 11:29:24AM +0100, Peter Krempa wrote:
> > Commit 7452162adec25c10 introduced 'qom-path' argument to BLOCK_IO_ERROR
> > event but when the event is instantiated in 'send_qmp_error_event()' the
> > arguments for 'device' and 'qom_path' in
> > qapi_event_send_block_io_error() were reversed :
> > 
> > Generated code for sending event:
> > 
> >   void qapi_event_send_block_io_error(const char *qom_path,
> >                                       const char *device,
> >                                       const char *node_name,
> >                                       IoOperationType operation,
> >                                       [...]
> > 
> > Call inside send_qmp_error_event():
> > 
> >      qapi_event_send_block_io_error(blk_name(blk),
> >                                     blk_get_attached_dev_path(blk),
> >                                     bs ? bdrv_get_node_name(bs) : NULL, optype,
> >                                     [...]
> > 
> > This results into reporting the QOM path as the device alias and vice
> > versa which in turn breaks libvirt, which expects the device alias being
> > either a valid alias or empty (which would make libvirt do the lookup by
> > node-name instead).
> > 
> > Fixes: 7452162adec25c1003d5bf0079aca52913a80e0c
> > Signed-off-by: Peter Krempa <pkrempa@redhat.com>
> 
> Hmm, luckily that was only introduced in 9.2.0, so we can probably just
> fix it in stable, and not worry about libvirt needs to add back compat
> to workaround the initially broken 9.2.0 release events.

Indeed, there's no need for a specific libvirt workaround. Although I
still plan to modify the libvirt code to prefer node-name if present due
to other reasons (to have the correct path of the image if the error
happened in a backing image), which will also workaround this issue in
the end.

Re: [PATCH] block-backend: Fix argument order when calling 'qapi_event_send_block_io_error()'

[Kevin Wolf]

Am 27.01.2025 um 12:07 hat Peter Krempa geschrieben:
> On Mon, Jan 27, 2025 at 10:53:16 +0000, Daniel P. Berrangé wrote:
> > On Mon, Jan 27, 2025 at 11:29:24AM +0100, Peter Krempa wrote:
> > > Commit 7452162adec25c10 introduced 'qom-path' argument to BLOCK_IO_ERROR
> > > event but when the event is instantiated in 'send_qmp_error_event()' the
> > > arguments for 'device' and 'qom_path' in
> > > qapi_event_send_block_io_error() were reversed :
> > > 
> > > Generated code for sending event:
> > > 
> > >   void qapi_event_send_block_io_error(const char *qom_path,
> > >                                       const char *device,
> > >                                       const char *node_name,
> > >                                       IoOperationType operation,
> > >                                       [...]
> > > 
> > > Call inside send_qmp_error_event():
> > > 
> > >      qapi_event_send_block_io_error(blk_name(blk),
> > >                                     blk_get_attached_dev_path(blk),
> > >                                     bs ? bdrv_get_node_name(bs) : NULL, optype,
> > >                                     [...]
> > > 
> > > This results into reporting the QOM path as the device alias and vice
> > > versa which in turn breaks libvirt, which expects the device alias being
> > > either a valid alias or empty (which would make libvirt do the lookup by
> > > node-name instead).
> > > 
> > > Fixes: 7452162adec25c1003d5bf0079aca52913a80e0c
> > > Signed-off-by: Peter Krempa <pkrempa@redhat.com>
> > 
> > Hmm, luckily that was only introduced in 9.2.0, so we can probably just
> > fix it in stable, and not worry about libvirt needs to add back compat
> > to workaround the initially broken 9.2.0 release events.
> 
> Indeed, there's no need for a specific libvirt workaround. Although I
> still plan to modify the libvirt code to prefer node-name if present due
> to other reasons (to have the correct path of the image if the error
> happened in a backing image), which will also workaround this issue in
> the end.

Currently, QEMU always puts the node name of the root node there, so for
now you won't see backing file nodes in these events. But it's probably
a good idea to prepare libvirt for it anyway, maybe we can find a way to
have better error events later. The challenge in doing that is that the
error events are sent by the devices, not by the block node that
actually first encountered the error.

Kevin

Re: [PATCH] block-backend: Fix argument order when calling 'qapi_event_send_block_io_error()'

[Kevin Wolf]

Am 27.01.2025 um 11:29 hat Peter Krempa geschrieben:
> Commit 7452162adec25c10 introduced 'qom-path' argument to BLOCK_IO_ERROR
> event but when the event is instantiated in 'send_qmp_error_event()' the
> arguments for 'device' and 'qom_path' in
> qapi_event_send_block_io_error() were reversed :
> 
> Generated code for sending event:
> 
>   void qapi_event_send_block_io_error(const char *qom_path,
>                                       const char *device,
>                                       const char *node_name,
>                                       IoOperationType operation,
>                                       [...]
> 
> Call inside send_qmp_error_event():
> 
>      qapi_event_send_block_io_error(blk_name(blk),
>                                     blk_get_attached_dev_path(blk),
>                                     bs ? bdrv_get_node_name(bs) : NULL, optype,
>                                     [...]
> 
> This results into reporting the QOM path as the device alias and vice
> versa which in turn breaks libvirt, which expects the device alias being
> either a valid alias or empty (which would make libvirt do the lookup by
> node-name instead).
> 
> Fixes: 7452162adec25c1003d5bf0079aca52913a80e0c
> Signed-off-by: Peter Krempa <pkrempa@redhat.com>

Thanks, applied to the block branch after resolving a merge conflict
with:

    block: Fix leak in send_qmp_error_event
    https://lists.gnu.org/archive/html/qemu-block/2024-11/msg00258.html

Both patches are trivial, so I'm reasonably confident I got this one
right. :-)

Kevin