| Message ID | 1300de97ea0a05962e60de5aba0eb56b5b77c6ee.1426779661.git.berto@igalia.com |
|---|---|
| State | New |
| Headers | show |
On 2015-03-19 at 11:43, Alberto Garcia wrote: > 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 <berto@igalia.com> > --- > block.c | 5 +++++ > block/quorum.c | 5 +---- > include/block/block.h | 1 + > 3 files changed, 7 insertions(+), 4 deletions(-) Reviewed-by: Max Reitz <mreitz@redhat.com>
Alberto Garcia <berto@igalia.com> writes: > 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 <berto@igalia.com> > --- > block.c | 5 +++++ > block/quorum.c | 5 +---- > include/block/block.h | 1 + > 3 files changed, 7 insertions(+), 4 deletions(-) > > diff --git a/block.c b/block.c > index 0fe97de..af284e3 100644 > --- a/block.c > +++ b/block.c > @@ -3920,6 +3920,11 @@ const char *bdrv_get_device_name(const BlockDriverState *bs) > return bs->blk ? blk_name(bs->blk) : ""; > } > > +const char *bdrv_get_device_or_node_name(const BlockDriverState *bs) > +{ > + return bs->blk ? blk_name(bs->blk) : bs->node_name; > +} > + Does this have uses beyond identifying @bs to the user? > int bdrv_get_flags(BlockDriverState *bs) > { > return bs->open_flags; > diff --git a/block/quorum.c b/block/quorum.c > index 437b122..f91ef75 100644 > --- a/block/quorum.c > +++ b/block/quorum.c > @@ -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); > } Preexisting: what if reference is null? event.json suggests it can't be null: ## # @QUORUM_FAILURE # # Emitted by the Quorum block driver if it fails to establish a quorum # # @reference: device name if defined else node name # # @sector-num: number of the first sector of the failed read operation # # @sectors-count: failed read operation sector count # # Since: 2.0 ## { 'event': 'QUORUM_FAILURE', 'data': { 'reference': 'str', 'sector-num': 'int', 'sectors-count': 'int' } } > diff --git a/include/block/block.h b/include/block/block.h > index 4c57d63..b285e0d 100644 > --- a/include/block/block.h > +++ b/include/block/block.h > @@ -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);
On Fri, Mar 20, 2015 at 08:40:32AM +0100, Markus Armbruster wrote: > > +const char *bdrv_get_device_or_node_name(const BlockDriverState *bs) > > +{ > > + return bs->blk ? blk_name(bs->blk) : bs->node_name; > > +} > > + > > Does this have uses beyond identifying @bs to the user? None that I can think of, although apart from error messages it can also be used in data types, like the cases of BLOCK_IMAGE_CORRUPTED and BlockJobInfo that are being discussed: https://lists.gnu.org/archive/html/qemu-devel/2015-03/msg03651.html > > 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); > > } > > Preexisting: what if reference is null? It can't happen, both the device and the node name strings are guaranteed to be non-null. The latter is actually a static string so there's no chance bs->node_name returns a null pointer. Berto
Alberto Garcia <berto@igalia.com> writes: > On Fri, Mar 20, 2015 at 08:40:32AM +0100, Markus Armbruster wrote: > >> > +const char *bdrv_get_device_or_node_name(const BlockDriverState *bs) >> > +{ >> > + return bs->blk ? blk_name(bs->blk) : bs->node_name; >> > +} >> > + >> >> Does this have uses beyond identifying @bs to the user? > > None that I can think of, although apart from error messages it can > also be used in data types, like the cases of BLOCK_IMAGE_CORRUPTED > and BlockJobInfo that are being discussed: > > https://lists.gnu.org/archive/html/qemu-devel/2015-03/msg03651.html Workable because BB and BDS names share a common name space. But is it a good idea? No need to discuss this here if it's already discussed elsewhere. >> > 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); >> > } >> >> Preexisting: what if reference is null? > > It can't happen, both the device and the node name strings are > guaranteed to be non-null. The latter is actually a static string so > there's no chance bs->node_name returns a null pointer. You're right, I missed the fact that BDS member node_name is an array. Suggest to add a function comment to bdrv_get_device_or_node_name() explaining intended use, and that it never returns null.
diff --git a/block.c b/block.c index 0fe97de..af284e3 100644 --- a/block.c +++ b/block.c @@ -3920,6 +3920,11 @@ const char *bdrv_get_device_name(const BlockDriverState *bs) return bs->blk ? blk_name(bs->blk) : ""; } +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; diff --git a/block/quorum.c b/block/quorum.c index 437b122..f91ef75 100644 --- a/block/quorum.c +++ b/block/quorum.c @@ -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); } diff --git a/include/block/block.h b/include/block/block.h index 4c57d63..b285e0d 100644 --- a/include/block/block.h +++ b/include/block/block.h @@ -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);
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 <berto@igalia.com> --- block.c | 5 +++++ block/quorum.c | 5 +---- include/block/block.h | 1 + 3 files changed, 7 insertions(+), 4 deletions(-)