Re: [Qemu-devel] [PATCH 1/3] block: add bdrv_get_device_or_node_name()

[Markus Armbruster]

Alberto Garcia <address@hidden> 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.