Add bdrv_get_device_or_node_name()
PATCH v2 3/3 block: add 'node-name' field to BLOCK_IMAGE_CORRUPTED
In reply to Alberto GarciaSince this event can occur in nodes that cannot have a device name associated, include also a field with the node name.
--- block/qcow2.c | 8 ++++++-- docs/qmp/qmp-events.txt | 16 +++++++++------- qapi/block-core.json | 13 ++++++++----- 3 files changed, 23 insertions(+), 14 deletions(-)
block/qcow2.c +6 -2
@@ -2809,6 +2809,7 @@ void qcow2_signal_corruption(BlockDriverState *bs, bool fatal, int64_t offset, int64_t size, const char *message_format, ...) { BDRVQcowState *s = bs->opaque; +const char *node_name;char *message;va_list ap;
@@ -2832,8 +2833,11 @@ void qcow2_signal_corruption(BlockDriverState *bs, bool fatal, int64_t offset, "corruption events will be suppressed\n", message); } -qapi_event_send_block_image_corrupted (bdrv_get_device_name (bs), message, - offset >=0 , offset, size >=0 , size, + node_name =bdrv_get_node_name (bs); +qapi_event_send_block_image_corrupted (bdrv_get_device_name (bs), + *node_name !='\0' , node_name, + message, offset >=0 , offset, + size >=0 , size, fatal, &error_abort);g_free (message);
docs/qmp/qmp-events.txt +9 -7
@@ -35,17 +35,19 @@ Emitted when a disk image is being marked corrupt. Data: -- "device": Device name (json-string) -- "msg": Informative message (e.g., reason for the corruption) (json-string) -- "offset": If the corruption resulted from an image access, this is the access - offset into the image (json-int) -- "size": If the corruption resulted from an image access, this is the access - size (json-int) +- "device": Device name (json-string) +- "node-name": Node name, if it's present (json-string) +- "msg": Informative message (e.g., reason for the corruption) + (json-string) +- "offset": If the corruption resulted from an image access, this + is the access offset into the image (json-int) +- "size": If the corruption resulted from an image access, this + is the access size (json-int) Example: { "event": "BLOCK_IMAGE_CORRUPTED", - "data": { "device": "ide0-hd0", + "data": { "device": "ide0-hd0", "node-name": "node0", "msg": "Prevented active L1 table overwrite", "offset": 196608, "size": 65536 }, "timestamp": { "seconds": 1378126126, "microseconds": 966463 } }
qapi/block-core.json +8 -5
@@ -1753,6 +1753,8 @@ # # @device: device name # +# @node-name: #optional node name (Since: 2.3) +# # @msg: informative message for human consumption, such as the kind of # corruption being detected. It should not be parsed by machine as it is # not guaranteed to be stable
@@ -1770,11 +1772,12 @@ # Since: 1.7 ## { 'event': 'BLOCK_IMAGE_CORRUPTED', - 'data': { 'device' : 'str', - 'msg' : 'str', - '*offset': 'int', - '*size' : 'int', - 'fatal' : 'bool' } } + 'data': { 'device' : 'str', + '*node-name' : 'str', + 'msg' : 'str', + '*offset' : 'int', + '*size' : 'int', + 'fatal' : 'bool' } } ## # @BLOCK_IO_ERROR
Signature · 1 line
2.1.4
v2 cover Add bdrv_get_device_or_node_name()
Second version of the series.
v2:
- bdrv_get_device_or_node_name() includes a comment explaining its usage.
- The error messages have been updated to say 'node' instead of 'device' where appropriate.
- The BLOCK_IMAGE_CORRUPTED event has a new 'node-name' field.
Regards,
Berto
Alberto Garcia (3):
block: add bdrv_get_device_or_node_name()
block: use bdrv_get_device_or_node_name() in error messages
block: add 'node-name' field to BLOCK_IMAGE_CORRUPTED
block.c | 30 ++++++++++++++++++++---------- block/qcow.c | 4 ++-- block/qcow2.c | 10 +++++++--- block/qed.c | 2 +- block/quorum.c | 5 +---- block/vdi.c | 2 +- block/vhdx.c | 2 +- block/vmdk.c | 4 ++-- block/vpc.c | 2 +- block/vvfat.c | 3 ++- blockdev.c | 4 ++-- docs/qmp/qmp-events.txt | 16 +++++++++------- include/block/block.h | 1 + include/qapi/qmp/qerror.h | 8 ++++---- qapi/block-core.json | 13 ++++++++----- 15 files changed, 62 insertions(+), 44 deletions(-)
Signature · 1 line
2.1.4
PATCH v2 1/3 block: add bdrv_get_device_or_node_name()
This function gets the device name associated with a BlockDriverState, or its node name if the device name is empty.
--- block.c | 9 +++++++++ block/quorum.c | 5 +---- include/block/block.h | 1 + 3 files changed, 11 insertions(+), 4 deletions(-)
block.c +9 -0
@@ -3920,6 +3920,15 @@ const char *bdrv_get_device_name(const BlockDriverState *bs)return bs->blk ?blk_name (bs->blk) :"" ; } +/* This can be used to identify nodes that might not have a device +* name associated. Since node and device names live in the same +* namespace, the result is unambiguous. The exception is if both are +* absent, then this returns an empty (non-null) string. */ +const char *bdrv_get_device_or_node_name (const BlockDriverState *bs) +{ +return bs->blk ?blk_name (bs->blk) : bs->node_name; +} +int bdrv_get_flags (BlockDriverState *bs) {return bs->open_flags;
block/quorum.c +1 -4
@@ -226,10 +226,7 @@ static void quorum_report_bad(QuorumAIOCB *acb, char *node_name, int ret)static void quorum_report_failure (QuorumAIOCB *acb) { -const char *reference =bdrv_get_device_name (acb->common.bs)[0 ] ? -bdrv_get_device_name (acb->common.bs) : - acb->common.bs->node_name; - +const char *reference =bdrv_get_device_or_node_name (acb->common.bs);qapi_event_send_quorum_failure (reference, acb->sector_num, acb->nb_sectors, &error_abort); }
include/block/block.h +1 -0
@@ -398,6 +398,7 @@ void bdrv_iterate_format(void (*it)(void *opaque, const char *name), void *opaque);const char *bdrv_get_node_name (const BlockDriverState *bs);const char *bdrv_get_device_name (const BlockDriverState *bs); +const char *bdrv_get_device_or_node_name (const BlockDriverState *bs);int bdrv_get_flags (BlockDriverState *bs);int bdrv_write_compressed (BlockDriverState *bs,int64_t sector_num,const uint8_t *buf,int nb_sectors);
Signature · 1 line
2.1.4
PATCH v2 2/3 block: use bdrv_get_device_or_node_name() in error messages
In reply to Alberto GarciaThere are several error messages that identify a BlockDriverState by its device name. However those errors can be produced in nodes that don't have a device name associated.
In those cases we should use bdrv_get_device_or_node_name() to fall back to the node name and produce a more meaningful message. The messages are also updated to use the more generic term 'node' instead of 'device'.
--- block.c | 21 +++++++++++---------- block/qcow.c | 4 ++-- block/qcow2.c | 2 +- block/qed.c | 2 +- block/vdi.c | 2 +- block/vhdx.c | 2 +- block/vmdk.c | 4 ++-- block/vpc.c | 2 +- block/vvfat.c | 3 ++- blockdev.c | 4 ++-- include/qapi/qmp/qerror.h | 8 ++++---- 11 files changed, 28 insertions(+), 26 deletions(-)
block.c +11 -10
@@ -1224,8 +1224,8 @@ void bdrv_set_backing_hd(BlockDriverState *bs, BlockDriverState *backing_hd)bdrv_op_unblock_all (bs->backing_hd, bs->backing_blocker); }else if (backing_hd) {error_setg (&bs->backing_blocker, -"device is used as backing hd of '%s'" , -bdrv_get_device_name (bs)); +"node is used as backing hd of '%s'" , +bdrv_get_device_or_node_name (bs)); } bs->backing_hd = backing_hd;
@@ -1812,8 +1812,8 @@ int bdrv_reopen_prepare(BDRVReopenState *reopen_state, BlockReopenQueue *queue,* to r/w */ if (!(reopen_state->bs->open_flags &BDRV_O_ALLOW_RDWR ) && reopen_state->flags &BDRV_O_RDWR ) { -error_set (errp,QERR_DEVICE_IS_READ_ONLY , -bdrv_get_device_name (reopen_state->bs)); +error_set (errp,QERR_NODE_IS_READ_ONLY , +bdrv_get_device_or_node_name (reopen_state->bs)); goto error; }
@@ -1840,7 +1840,8 @@ int bdrv_reopen_prepare(BDRVReopenState *reopen_state, BlockReopenQueue *queue,/* It is currently mandatory to have a bdrv_reopen_prepare() * handler for each supported drv. */ error_set (errp,QERR_BLOCK_FORMAT_FEATURE_NOT_SUPPORTED , - drv->format_name,bdrv_get_device_name (reopen_state->bs), + drv->format_name, +bdrv_get_device_or_node_name (reopen_state->bs),"reopening of file" ); ret =-1 ; goto error;
@@ -3764,8 +3765,8 @@ void bdrv_add_key(BlockDriverState *bs, const char *key, Error **errp) {if (key) {if (!bdrv_is_encrypted (bs)) { -error_setg (errp,"Device '%s' is not encrypted" , -bdrv_get_device_name (bs)); +error_setg (errp,"Node '%s' is not encrypted" , +bdrv_get_device_or_node_name (bs)); }else if (bdrv_set_key (bs, key) <0 ) {error_set (errp,QERR_INVALID_PASSWORD ); }
@@ -3773,7 +3774,7 @@ void bdrv_add_key(BlockDriverState *bs, const char *key, Error **errp)if (bdrv_key_required (bs)) {error_set (errp,ERROR_CLASS_DEVICE_ENCRYPTED ,"'%s' (%s) is encrypted" , -bdrv_get_device_name (bs), +bdrv_get_device_or_node_name (bs),bdrv_get_encrypted_filename (bs)); } }
@@ -5548,8 +5549,8 @@ bool bdrv_op_is_blocked(BlockDriverState *bs, BlockOpType op, Error **errp)if (!QLIST_EMPTY (&bs->op_blockers[op])) { blocker =QLIST_FIRST (&bs->op_blockers[op]);if (errp) { -error_setg (errp,"Device '%s' is busy: %s" , -bdrv_get_device_name (bs), +error_setg (errp,"Node '%s' is busy: %s" , +bdrv_get_device_or_node_name (bs),error_get_pretty (blocker->reason)); }return true;
block/qcow.c +2 -2
@@ -124,7 +124,7 @@ static int qcow_open(BlockDriverState *bs, QDict *options, int flags,snprintf (version,sizeof (version),"QCOW version %" PRIu32, header.version);error_set (errp,QERR_UNKNOWN_BLOCK_FORMAT_FEATURE , -bdrv_get_device_name (bs),"qcow" , version); +bdrv_get_device_or_node_name (bs),"qcow" , version); ret = -ENOTSUP ; goto fail; }
@@ -231,7 +231,7 @@ static int qcow_open(BlockDriverState *bs, QDict *options, int flags,/* Disable migration when qcow images are used */ error_set (&s->migration_blocker,QERR_BLOCK_FORMAT_FEATURE_NOT_SUPPORTED , -"qcow" ,bdrv_get_device_name (bs),"live migration" ); +"qcow" ,bdrv_get_device_or_node_name (bs),"live migration" );migrate_add_blocker (s->migration_blocker);qemu_co_mutex_init (&s->lock);
block/qcow2.c +1 -1
@@ -207,7 +207,7 @@ static void GCC_FMT_ATTR(3, 4) report_unsupported(BlockDriverState *bs,va_end (ap);error_set (errp,QERR_UNKNOWN_BLOCK_FORMAT_FEATURE , -bdrv_get_device_name (bs),"qcow2" , msg); +bdrv_get_device_or_node_name (bs),"qcow2" , msg); } static void report_unsupported_feature(BlockDriverState *bs,
block/qed.c +1 -1
@@ -408,7 +408,7 @@ static int bdrv_qed_open(BlockDriverState *bs, QDict *options, int flags,snprintf (buf,sizeof (buf),"%" PRIx64, s->header.features & ~QED_FEATURE_MASK );error_set (errp,QERR_UNKNOWN_BLOCK_FORMAT_FEATURE , -bdrv_get_device_name (bs),"QED" , buf); +bdrv_get_device_or_node_name (bs),"QED" , buf);return -ENOTSUP ; }if (!qed_is_cluster_size_valid (s->header.cluster_size)) {
block/vdi.c +1 -1
@@ -504,7 +504,7 @@ static int vdi_open(BlockDriverState *bs, QDict *options, int flags,/* Disable migration when vdi images are used */ error_set (&s->migration_blocker,QERR_BLOCK_FORMAT_FEATURE_NOT_SUPPORTED , -"vdi" ,bdrv_get_device_name (bs),"live migration" ); +"vdi" ,bdrv_get_device_or_node_name (bs),"live migration" );migrate_add_blocker (s->migration_blocker);qemu_co_mutex_init (&s->write_lock);
block/vhdx.c +1 -1
@@ -1004,7 +1004,7 @@ static int vhdx_open(BlockDriverState *bs, QDict *options, int flags,/* Disable migration when VHDX images are used */ error_set (&s->migration_blocker,QERR_BLOCK_FORMAT_FEATURE_NOT_SUPPORTED , -"vhdx" ,bdrv_get_device_name (bs),"live migration" ); +"vhdx" ,bdrv_get_device_or_node_name (bs),"live migration" );migrate_add_blocker (s->migration_blocker);return 0 ;
block/vmdk.c +2 -2
@@ -669,7 +669,7 @@ static int vmdk_open_vmdk4(BlockDriverState *bs,snprintf (buf,sizeof (buf),"VMDK version %" PRId32,le32_to_cpu (header.version));error_set (errp,QERR_UNKNOWN_BLOCK_FORMAT_FEATURE , -bdrv_get_device_name (bs),"vmdk" , buf); +bdrv_get_device_or_node_name (bs),"vmdk" , buf);return -ENOTSUP ; }else if (le32_to_cpu (header.version) ==3 && (flags &BDRV_O_RDWR )) {/* VMware KB 2064959 explains that version 3 added support for
@@ -964,7 +964,7 @@ static int vmdk_open(BlockDriverState *bs, QDict *options, int flags,/* Disable migration when VMDK images are used */ error_set (&s->migration_blocker,QERR_BLOCK_FORMAT_FEATURE_NOT_SUPPORTED , -"vmdk" ,bdrv_get_device_name (bs),"live migration" ); +"vmdk" ,bdrv_get_device_or_node_name (bs),"live migration" );migrate_add_blocker (s->migration_blocker);g_free (buf);return 0 ;
block/vpc.c +1 -1
@@ -320,7 +320,7 @@ static int vpc_open(BlockDriverState *bs, QDict *options, int flags,/* Disable migration when VHD images are used */ error_set (&s->migration_blocker,QERR_BLOCK_FORMAT_FEATURE_NOT_SUPPORTED , -"vpc" ,bdrv_get_device_name (bs),"live migration" ); +"vpc" ,bdrv_get_device_or_node_name (bs),"live migration" );migrate_add_blocker (s->migration_blocker);return 0 ;
block/vvfat.c +2 -1
@@ -1182,7 +1182,8 @@ static int vvfat_open(BlockDriverState *bs, QDict *options, int flags,if (s->qcow) {error_set (&s->migration_blocker,QERR_BLOCK_FORMAT_FEATURE_NOT_SUPPORTED , -"vvfat (rw)" ,bdrv_get_device_name (bs),"live migration" ); +"vvfat (rw)" ,bdrv_get_device_or_node_name (bs), +"live migration" );migrate_add_blocker (s->migration_blocker); }
blockdev.c +2 -2
@@ -1248,7 +1248,7 @@ static void internal_snapshot_prepare(BlkTransactionState *common, }if (bdrv_is_read_only (bs)) { -error_set (errp,QERR_DEVICE_IS_READ_ONLY , device); +error_set (errp,QERR_NODE_IS_READ_ONLY , device);return ; }
@@ -2055,7 +2055,7 @@ void qmp_block_resize(bool has_device, const char *device,error_set (errp,QERR_UNSUPPORTED );break ;case -EACCES : -error_set (errp,QERR_DEVICE_IS_READ_ONLY , device); +error_set (errp,QERR_NODE_IS_READ_ONLY , device);break ;case -EBUSY :error_set (errp,QERR_DEVICE_IN_USE , device);
include/qapi/qmp/qerror.h +4 -4
@@ -38,7 +38,7 @@ void qerror_report_err(Error *err); ERROR_CLASS_GENERIC_ERROR, "Base '%s' not found"#define QERR_BLOCK_FORMAT_FEATURE_NOT_SUPPORTED \ - ERROR_CLASS_GENERIC_ERROR, "Block format '%s' used by device '%s' does not support feature '%s'" + ERROR_CLASS_GENERIC_ERROR, "Block format '%s' used by node '%s' does not support feature '%s'"#define QERR_BLOCK_JOB_NOT_READY \ ERROR_CLASS_GENERIC_ERROR, "The active block job for device '%s' cannot be completed"
@@ -58,9 +58,6 @@ void qerror_report_err(Error *err);#define QERR_DEVICE_IN_USE \ ERROR_CLASS_GENERIC_ERROR, "Device '%s' is in use" -#define QERR_DEVICE_IS_READ_ONLY \ - ERROR_CLASS_GENERIC_ERROR, "Device '%s' is read only" -#define QERR_DEVICE_NO_HOTPLUG \ ERROR_CLASS_GENERIC_ERROR, "Device '%s' does not support hotplugging"
@@ -103,6 +100,9 @@ void qerror_report_err(Error *err);#define QERR_MISSING_PARAMETER \ ERROR_CLASS_GENERIC_ERROR, "Parameter '%s' is missing" +#define QERR_NODE_IS_READ_ONLY \ + ERROR_CLASS_GENERIC_ERROR, "Node '%s' is read only" +#define QERR_PERMISSION_DENIED \ ERROR_CLASS_GENERIC_ERROR, "Insufficient permission to perform this operation"
Signature · 1 line
2.1.4
Re: [Qemu-devel] [PATCH 1/3] block: add bdrv_get_device_or_node_name()
In reply to Alberto GarciaQuoted 9 lines · Alberto Garcia
This function gets the device name associated with a BlockDriverState, or its node name if the device name is empty.
Signed-off-by Alberto Garcia <[email protected]>--- block.c | 9 +++++++++ block/quorum.c | 5 +---- include/block/block.h | 1 + 3 files changed, 11 insertions(+), 4 deletions(-)
Re: [Qemu-devel] [PATCH 2/3] block: use bdrv_get_device_or_node_name() in error messages
In reply to Alberto GarciaQuoted 29 lines · Alberto Garcia
There are several error messages that identify a BlockDriverState by its device name. However those errors can be produced in nodes that don't have a device name associated.
In those cases we should use
bdrv_get_device_or_node_name()to fall back to the node name and produce a more meaningful message. The messages are also updated to use the more generic term 'node' instead of 'device'.Signed-off-by Alberto Garcia <[email protected]>--- block.c | 21 +++++++++++---------- block/qcow.c | 4 ++-- block/qcow2.c | 2 +- block/qed.c | 2 +- block/vdi.c | 2 +- block/vhdx.c | 2 +- block/vmdk.c | 4 ++-- block/vpc.c | 2 +- block/vvfat.c | 3 ++- blockdev.c | 4 ++-- include/qapi/qmp/qerror.h | 8 ++++---- 11 files changed, 28 insertions(+), 26 deletions(-)block.c +0
-0
block.c void bdrv_set_backing_hd(BlockDriverState *bs, BlockDriverState *backing_hd) · line 1228
Quoted 7 lines
block.c +2
-2@@ -1224,8 +1224,8 @@ void bdrv_set_backing_hd(BlockDriverState *bs, BlockDriverState *backing_hd)bdrv_op_unblock_all (bs->backing_hd, bs->backing_blocker); }else if (backing_hd) {error_setg (&bs->backing_blocker, -"device is used as backing hd of '%s'" , -bdrv_get_device_name (bs)); +"node is used as backing hd of '%s'" , +bdrv_get_device_or_node_name (bs));
Actually, the change of "device" to "node" here is independent of the use of bdrv_get_device_or_node_name(). But it's correct anyway.
block.c int bdrv_reopen_prepare(BDRVReopenState *reopen_state, BlockReopenQueue *queue, · line 1816
Quoted 11 lines
block.c +2
-2} bs->backing_hd = backing_hd;@@ -1812,8 +1812,8 @@ int bdrv_reopen_prepare(BDRVReopenState *reopen_state, BlockReopenQueue *queue,* to r/w */ if (!(reopen_state->bs->open_flags &BDRV_O_ALLOW_RDWR ) && reopen_state->flags &BDRV_O_RDWR ) { -error_set (errp,QERR_DEVICE_IS_READ_ONLY , -bdrv_get_device_name (reopen_state->bs)); +error_set (errp,QERR_NODE_IS_READ_ONLY , +bdrv_get_device_or_node_name (reopen_state->bs));
I think Markus would be happier with just removing the macro.
(Make it error_setg(errp, "Node '%s' is read only", bdrv_get_device_or_node_name(reopen_state->bs)) and remove the QERR_DEVICE_IS_READ_ONLY macro from qerror.h)
[snip]
include/qapi/qmp/qerror.h void qerror_report_err(Error *err); · line 41
Quoted 10 lines
include/qapi/qmp/qerror.h +1
-1@@ -38,7 +38,7 @@ void qerror_report_err(Error *err);ERROR_CLASS_GENERIC_ERROR ,"Base '%s' not found" #define QERR_BLOCK_FORMAT_FEATURE_NOT_SUPPORTED \ -ERROR_CLASS_GENERIC_ERROR ,"Block format '%s' used by device '%s' does not support feature '%s'" +ERROR_CLASS_GENERIC_ERROR ,"Block format '%s' used by node '%s' does not support feature '%s'"
Preexisting, but first: This line has over 80 characters.
Second: This seems like a good opportunity to drop this macro and replace its occurrences by error_setg(errp, "Block format...") and the like.
Quoted 13 lines
include/qapi/qmp/qerror.h +0
-3#define QERR_BLOCK_JOB_NOT_READY \ ERROR_CLASS_GENERIC_ERROR, "The active block job for device '%s' cannot be completed"@@ -58,9 +58,6 @@ void qerror_report_err(Error *err);#define QERR_DEVICE_IN_USE \ ERROR_CLASS_GENERIC_ERROR, "Device '%s' is in use" -#define QERR_DEVICE_IS_READ_ONLY \ - ERROR_CLASS_GENERIC_ERROR, "Device '%s' is read only" -#define QERR_DEVICE_NO_HOTPLUG \ ERROR_CLASS_GENERIC_ERROR, "Device '%s' does not support hotplugging"
include/qapi/qmp/qerror.h void qerror_report_err(Error *err); · line 107
Quoted 8 lines
include/qapi/qmp/qerror.h +3
-0@@ -103,6 +100,9 @@ void qerror_report_err(Error *err);#define QERR_MISSING_PARAMETER \ ERROR_CLASS_GENERIC_ERROR, "Parameter '%s' is missing" +#define QERR_NODE_IS_READ_ONLY \ + ERROR_CLASS_GENERIC_ERROR, "Node '%s' is read only" +#define QERR_PERMISSION_DENIED \ ERROR_CLASS_GENERIC_ERROR, "Insufficient permission to perform this operation"
As said above, the same goes for this macro.
I'm not so ardent about this, so if you want to keep the macros, it's fine with me; but the overly long line need to be fixed (only the one you're touching, not necessarily the rest, because I guess they'll be dropped in the future anyway).
Markus?
Max
Re: [Qemu-devel] [PATCH 3/3] block: add 'node-name' field to BLOCK_IMAGE_CORRUPTED
In reply to Alberto GarciaQuoted 72 lines · Alberto Garcia
Since this event can occur in nodes that cannot have a device name associated, include also a field with the node name.
Signed-off-by Alberto Garcia <[email protected]>--- block/qcow2.c | 8 ++++++-- docs/qmp/qmp-events.txt | 16 +++++++++------- qapi/block-core.json | 13 ++++++++----- 3 files changed, 23 insertions(+), 14 deletions(-)block/qcow2.c +6
-2@@ -2809,6 +2809,7 @@ void qcow2_signal_corruption(BlockDriverState *bs, bool fatal, int64_t offset, int64_t size, const char *message_format, ...) { BDRVQcowState *s = bs->opaque; +const char *node_name;char *message;va_list ap;@@ -2832,8 +2833,11 @@ void qcow2_signal_corruption(BlockDriverState *bs, bool fatal, int64_t offset, "corruption events will be suppressed\n", message); } -qapi_event_send_block_image_corrupted (bdrv_get_device_name (bs), message, - offset >=0 , offset, size >=0 , size, + node_name =bdrv_get_node_name (bs); +qapi_event_send_block_image_corrupted (bdrv_get_device_name (bs), + *node_name !='\0' , node_name, + message, offset >=0 , offset, + size >=0 , size, fatal, &error_abort);g_free (message);docs/qmp/qmp-events.txt +9
-7@@ -35,17 +35,19 @@ Emitted when a disk image is being marked corrupt. Data: -- "device": Device name (json-string) -- "msg": Informative message (e.g., reason for the corruption) (json-string) -- "offset": If the corruption resulted from an image access, this is the access - offset into the image (json-int) -- "size": If the corruption resulted from an image access, this is the access - size (json-int) +- "device": Device name (json-string) +- "node-name": Node name, if it's present (json-string) +- "msg": Informative message (e.g., reason for the corruption) + (json-string) +- "offset": If the corruption resulted from an image access, this + is the access offset into the image (json-int) +- "size": If the corruption resulted from an image access, this + is the access size (json-int) Example: { "event": "BLOCK_IMAGE_CORRUPTED", - "data": { "device": "ide0-hd0", + "data": { "device": "ide0-hd0", "node-name": "node0", "msg": "Prevented active L1 table overwrite", "offset": 196608, "size": 65536 }, "timestamp": { "seconds": 1378126126, "microseconds": 966463 } }qapi/block-core.json +0
-0
qapi/block-core.json line 1756
Quoted 4 lines
qapi/block-core.json +1
-0@@ -1753,6 +1753,8 @@ # # @device: device name # +# @node-name: #optional node name (Since: 2.3)
*2.4
With that fixed:
Quoted 22 lines
qapi/block-core.json +7
-5+# # @msg: informative message for human consumption, such as the kind of # corruption being detected. It should not be parsed by machine as it is # not guaranteed to be stable@@ -1770,11 +1772,12 @@ # Since: 1.7 ## { 'event': 'BLOCK_IMAGE_CORRUPTED', - 'data': { 'device' : 'str', - 'msg' : 'str', - '*offset': 'int', - '*size' : 'int', - 'fatal' : 'bool' } } + 'data': { 'device' : 'str', + '*node-name' : 'str', + 'msg' : 'str', + '*offset' : 'int', + '*size' : 'int', + 'fatal' : 'bool' } } ## # @BLOCK_IO_ERROR
Re: [Qemu-devel] [PATCH 2/3] block: use bdrv_get_device_or_node_name() in error messages
In reply to Max ReitzQuoted 97 lines · Max Reitz
Quoted 29 lines · Alberto Garcia
There are several error messages that identify a BlockDriverState by its device name. However those errors can be produced in nodes that don't have a device name associated.
In those cases we should use
bdrv_get_device_or_node_name()to fall back to the node name and produce a more meaningful message. The messages are also updated to use the more generic term 'node' instead of 'device'.Signed-off-by Alberto Garcia <[email protected]>--- block.c | 21 +++++++++++---------- block/qcow.c | 4 ++-- block/qcow2.c | 2 +- block/qed.c | 2 +- block/vdi.c | 2 +- block/vhdx.c | 2 +- block/vmdk.c | 4 ++-- block/vpc.c | 2 +- block/vvfat.c | 3 ++- blockdev.c | 4 ++-- include/qapi/qmp/qerror.h | 8 ++++---- 11 files changed, 28 insertions(+), 26 deletions(-)block.c +0
-0block.c void bdrv_set_backing_hd(BlockDriverState *bs, BlockDriverState *backing_hd) · line 1228
Quoted 7 lines
block.c +2
-2@@ -1224,8 +1224,8 @@ void bdrv_set_backing_hd(BlockDriverState *bs, BlockDriverState *backing_hd)bdrv_op_unblock_all (bs->backing_hd, bs->backing_blocker); }else if (backing_hd) {error_setg (&bs->backing_blocker, -"device is used as backing hd of '%s'" , -bdrv_get_device_name (bs)); +"node is used as backing hd of '%s'" , +bdrv_get_device_or_node_name (bs));Actually, the change of "device" to "node" here is independent of the use of
bdrv_get_device_or_node_name(). But it's correct anyway.block.c int bdrv_reopen_prepare(BDRVReopenState *reopen_state, BlockReopenQueue *queue, · line 1816
Quoted 10 lines
block.c +2
-2} bs->backing_hd = backing_hd;@@ -1812,8 +1812,8 @@ int bdrv_reopen_prepare(BDRVReopenState *reopen_state, BlockReopenQueue *queue,* to r/w */ if (!(reopen_state->bs->open_flags &BDRV_O_ALLOW_RDWR ) && reopen_state->flags &BDRV_O_RDWR ) { -error_set (errp,QERR_DEVICE_IS_READ_ONLY , -bdrv_get_device_name (reopen_state->bs)); +error_set (errp,QERR_NODE_IS_READ_ONLY , +bdrv_get_device_or_node_name (reopen_state->bs));I think Markus would be happier with just removing the macro.
(Make it
error_setg(errp, "Node '%s' is read only", bdrv_get_device_or_node_name(reopen_state->bs))and remove theQERR_DEVICE_IS_READ_ONLYmacro fromqerror.h)[snip]
include/qapi/qmp/qerror.h void qerror_report_err(Error *err); · line 40
Quoted 9 lines
include/qapi/qmp/qerror.h +1
-1@@ -38,7 +38,7 @@ void qerror_report_err(Error *err);ERROR_CLASS_GENERIC_ERROR ,"Base '%s' not found" #define QERR_BLOCK_FORMAT_FEATURE_NOT_SUPPORTED \ -ERROR_CLASS_GENERIC_ERROR ,"Block format '%s' used by device '%s' does not support feature '%s'" +ERROR_CLASS_GENERIC_ERROR ,"Block format '%s' used by node '%s' does not support feature '%s'" Preexisting, but first: This line has over 80 characters.
Second: This seems like a good opportunity to drop this macro and replace its occurrences by
error_setg(errp, "Block format...")and the like.Quoted 8 lines
include/qapi/qmp/qerror.h +0
-2#define QERR_BLOCK_JOB_NOT_READY \ ERROR_CLASS_GENERIC_ERROR, "The active block job for device '%s' cannot be completed"@@ -58,9 +58,6 @@ void qerror_report_err(Error *err);#define QERR_DEVICE_IN_USE \ ERROR_CLASS_GENERIC_ERROR, "Device '%s' is in use" -#define QERR_DEVICE_IS_READ_ONLY \ - ERROR_CLASS_GENERIC_ERROR, "Device '%s' is read only" -include/qapi/qmp/qerror.h void qerror_report_err(Error *err);
Quoted 10 lines
include/qapi/qmp/qerror.h +2
-0#define QERR_DEVICE_NO_HOTPLUG \ ERROR_CLASS_GENERIC_ERROR, "Device '%s' does not support hotplugging" @@ -103,6 +100,9 @@ void qerror_report_err(Error *err);#define QERR_MISSING_PARAMETER \ ERROR_CLASS_GENERIC_ERROR, "Parameter '%s' is missing" +#define QERR_NODE_IS_READ_ONLY \ + ERROR_CLASS_GENERIC_ERROR, "Node '%s' is read only" +#define QERR_PERMISSION_DENIED \ ERROR_CLASS_GENERIC_ERROR, "Insufficient permission to perform this operation"As said above, the same goes for this macro.
Quoted 6 lines
I'm not so ardent about this, so if you want to keep the macros, it's fine with me; but the overly long line need to be fixed (only the one you're touching, not necessarily the rest, because I guess they'll be dropped in the future anyway).
Markus?
I've been chipping away at qerror.h for a while. I think I can empty it out in the next development cycle. So every new QERR_ macro you add I'll have to replace again. Using literal strings instead will save me the trouble.
If you find yourself using the same error string in many places, chances are that some of these places do the same thing. Consider factoring out.
Other than that, I wouldn't worry about duplicating error strings. Drop in the bucket.