| Message ID | 09728d784888b38d7a8f09ee5e9e9c542c875e1e.1737973614.git.pkrempa@redhat.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | block-backend: Fix argument order when calling 'qapi_event_send_block_io_error()' | expand |
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
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.
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
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
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));
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(-)