Benoît Canet <benoit.ca...@irqsave.net> writes: > The Thursday 11 Sep 2014 à 07:00:41 (-0600), Eric Blake wrote : >> 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, > > It think it can't: see patch 2 extract: > >> +BlockBackend *blk_new(const char *name, Error **errp) >> +{ >> + BlockBackend *blk = g_new0(BlockBackend, 1); >> + >> + assert(name && name[0]); > >> but this function is guaranteed to return non-NULL.
You're right, blk_new() always returns a non-null, non-empty string. The condition to check here is "bs is not owned by a BB". Benoît's bs->blk ? blk_name(bs->blk) : "" does that nicely. Of course, Eric is right in that returning NULL on "not owned by a BB" would be cleaner than returning "". However, doing that in the middle of a series with a four-digit diffstat doesn't strike me as a good idea. Better do it on top.