Re: [Qemu-devel] [PATCH 08/23] block: Eliminate BlockDriverState member device_name[]

[Eric Blake]

On 09/11/2014 05:34 AM, Benoît Canet wrote:
> The Wednesday 10 Sep 2014 à 10:13:37 (+0200), Markus Armbruster wrote :
>> device_name[] is can become non-empty only in bdrv_new_named() and
>> bdrv_move_feature_fields().  The latter is used only to undo damage
>> done by bdrv_swap().  The former is called only by blk_new_with_bs().
>> Therefore, when a BlockDriverState's device_name[] is non-empty, then
>> it's owned by a BlockBackend.

[lots of lines trimmed - it's not only okay, but desirable to trim out
portions of a patch that you are okay with, in order to call attention
to the problem spots that you are commenting on without making the
reader have to scroll through pages of quoted context]

>>  
>> -const char *bdrv_get_device_name(BlockDriverState *bs)
>> +const char *bdrv_get_device_name(const BlockDriverState *bs)
>>  {
>> -    return bs->device_name;
>> +    const char *name = bs->blk ? blk_name(bs->blk) : NULL;
>> +    return name ?: "";
>>  }
> 
> Why not ?
> 
>     return bs->blk ? blk_name(bs->blk) : "";

If I understand right, it was because blk_name(bs->blk) may return NULL,
but this function is guaranteed to return non-NULL.

> 
> or 
> 
>     if (bs->blk) {
>         return blk_name(bs->blk);
>     }
> 
>     return "";
> 
> Would it fail to do the job ?
> 
> Also we could have made sure that bdrv_get_device_name return either
> a non empty string or NULL.
> 
> (We know blk_name will return a non empty string it's asserted)
> 
> This would need to change a few test all around the code but the final
> code logic would be less convoluted:
> -convert NULL to "" then test for not ""
> would turn into
> -return NULL test for not NULL

Indeed, the logic of NULL vs. string is slightly easier than the logic
of "" vs non-empty string; if we can guarantee that a non-NULL string
will be non-empty.  I'm not sure how much churn it would cause, though.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

signature.asc
Description: OpenPGP digital signature