block: Fix NULL dereference on empty drive error
PATCH 1/1 block: Fix NULL dereference on empty drive error
blk_error_action() sends a BLOCK_IO_ERROR QMP event which includes the node name of its root node. If the BlockBackend represents an empty drive, there is no root node, so we should not try to access its node name. Make the field optional in the event and include it only when the BlockBackend isn't empty.
---
Stefan, this is needed for your patch that reverts the workaround in the IDE flush code. Without it, make check seems to succeed, but if you look closer, qemu actually segfaults.
qapi/block-core.json | 6 ++++--
block/block-backend.c | 5 +++-- 2 files changed, 7 insertions(+), 4 deletions(-)
qapi/block-core.json +4 -2
@@ -3676,7 +3676,8 @@ # # @node-name: node name. Note that errors may be reported for the root node # that is directly attached to a guest device rather than for the -# node where the error occurred. (Since: 2.8) +# node where the error occurred. The node name is not present if +# the drive is empty. (Since: 2.8) # # @operation: I/O operation #
@@ -3707,7 +3708,8 @@ # ## { 'event': 'BLOCK_IO_ERROR', - 'data': { 'device': 'str', 'node-name': 'str', 'operation': 'IoOperationType', + 'data': { 'device': 'str', '*node-name': 'str', + 'operation': 'IoOperationType', 'action': 'BlockErrorAction', '*nospace': 'bool', 'reason': 'str' } }
block/block-backend.c +3 -2
@@ -1615,10 +1615,11 @@ static void send_qmp_error_event(BlockBackend *blk, bool is_read, int error) {IoOperationType optype; +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), -bdrv_get_node_name (blk_bs (blk)), optype, +qapi_event_send_block_io_error (blk_name (blk), !!bs, + bs ?bdrv_get_node_name (bs) :NULL , optype, action,blk_iostatus_is_enabled (blk), error ==ENOSPC ,strerror (error), &error_abort);
Signature · 1 line
2.13.6
Quoted 22 lines · Kevin Wolf
blk_error_action()sends aBLOCK_IO_ERRORQMP event which includes the node name of its root node. If the BlockBackend represents an empty drive, there is no root node, so we should not try to access its node name. Make the field optional in the event and include it only when the BlockBackend isn't empty.Signed-off-by Kevin Wolf <[email protected]>---
Stefan, this is needed for your patch that reverts the workaround in the IDE flush code. Without it, make check seems to succeed, but if you look closer, qemu actually segfaults.
qapi/block-core.json| 6 ++++--block/block-backend.c | 5 +++-- 2 files changed, 7 insertions(+), 4 deletions(-)qapi/block-core.json +0
-0
qapi/block-core.json line 3680
Quoted 6 lines
qapi/block-core.json +2
-1@@ -3676,7 +3676,8 @@ # # @node-name: node name. Note that errors may be reported for the root node # that is directly attached to a guest device rather than for the -# node where the error occurred. (Since: 2.8) +# node where the error occurred. The node name is not present if +# the drive is empty. (Since: 2.8)
Making an output field change from always present to sometimes absent might break older clients that expected to be able to parse the field unconditionally. Would it be better to keep the 'node-name' field mandatory in the output but make it an empty string?
Then again, since the field was not present prior to 2.8, but the event itself is older, we can argue that clients of older qemu have to be prepared for the field to not be present. So I think I can live with this change as-is.
qapi/block-core.json line 3715
Quoted 13 lines
qapi/block-core.json +2
-1# # @operation: I/O operation #@@ -3707,7 +3708,8 @@ # ## { 'event': 'BLOCK_IO_ERROR', - 'data': { 'device': 'str', 'node-name': 'str', 'operation': 'IoOperationType', + 'data': { 'device': 'str', '*node-name': 'str', + 'operation': 'IoOperationType', 'action': 'BlockErrorAction', '*nospace': 'bool', 'reason': 'str' } }
Signature · 3 lines
Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
Am 05.03.2018 um 17:10 hat Eric Blake geschrieben:
Quoted 18 lines · Kevin Wolf
Quoted 17 lines
blk_error_action()sends aBLOCK_IO_ERRORQMP event which includes the node name of its root node. If the BlockBackend represents an empty drive, there is no root node, so we should not try to access its node name. Make the field optional in the event and include it only when the BlockBackend isn't empty.Signed-off-by Kevin Wolf <[email protected]>---
Stefan, this is needed for your patch that reverts the workaround in the IDE flush code. Without it, make check seems to succeed, but if you look closer, qemu actually segfaults.
qapi/block-core.json| 6 ++++--block/block-backend.c | 5 +++-- 2 files changed, 7 insertions(+), 4 deletions(-)
Quoted 16 lines
qapi/block-core.json line 3680
Quoted 11 lines
qapi/block-core.json +2
-1@@ -3676,7 +3676,8 @@ # # @node-name: node name. Note that errors may be reported for the root node # that is directly attached to a guest device rather than for the -# node where the error occurred. (Since: 2.8) +# node where the error occurred. The node name is not present if +# the drive is empty. (Since: 2.8)Making an output field change from always present to sometimes absent might break older clients that expected to be able to parse the field unconditionally. Would it be better to keep the 'node-name' field mandatory in the output but make it an empty string?
I considered that, but how likely is it that a client can handle an empty string instead of a valid node name, but can't handle an absent field? I assume that such clients would probably break either way. And in that case I preferred to use the cleaner design.
Quoted 4 lines
Then again, since the field was not present prior to 2.8, but the event itself is older, we can argue that clients of older qemu have to be prepared for the field to not be present. So I think I can live with this change as-is.
Right, that too. If libvirt can deal with it (and I suppose it can because it doesn't really use node names yet), we should be okay.
Quoted 15 lines
qapi/block-core.json line 3714
Quoted 13 lines
qapi/block-core.json +2
-1# # @operation: I/O operation #@@ -3707,7 +3708,8 @@ # ## { 'event': 'BLOCK_IO_ERROR', - 'data': { 'device': 'str', 'node-name': 'str', 'operation': 'IoOperationType', + 'data': { 'device': 'str', '*node-name': 'str', + 'operation': 'IoOperationType', 'action': 'BlockErrorAction', '*nospace': 'bool', 'reason': 'str' } }Reviewed-by Eric Blake <[email protected]>
Thanks.
Kevin