[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 08/23] block: Eliminate BlockDriverState member
From: |
Markus Armbruster |
Subject: |
Re: [Qemu-devel] [PATCH 08/23] block: Eliminate BlockDriverState member device_name[] |
Date: |
Thu, 11 Sep 2014 21:11:51 +0200 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/24.3 (gnu/linux) |
Benoît Canet <address@hidden> 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.
- Re: [Qemu-devel] [PATCH 10/23] block: Eliminate DriveInfo member bdrv, use blk_by_legacy_dinfo(), (continued)
[Qemu-devel] [PATCH 15/23] ide: Complete conversion from BlockDriverState to BlockBackend, Markus Armbruster, 2014/09/10
[Qemu-devel] [PATCH 19/23] blockdev: Drop DriveInfo member enable_auto_del, Markus Armbruster, 2014/09/10
[Qemu-devel] [PATCH 13/23] virtio-blk: Rename VirtIOBlkConf variables to conf, Markus Armbruster, 2014/09/10
[Qemu-devel] [PATCH 20/23] block/qapi: Convert qmp_query_block() to BlockBackend, Markus Armbruster, 2014/09/10
[Qemu-devel] [PATCH 17/23] blockdev: Drop superfluous DriveInfo member id, Markus Armbruster, 2014/09/10
[Qemu-devel] [PATCH 18/23] blockdev: Fix blockdev-add not to create IDE drive (0, 0), Markus Armbruster, 2014/09/10
[Qemu-devel] [PATCH 23/23] block: Make device model's references to BlockBackend strong, Markus Armbruster, 2014/09/10
[Qemu-devel] [PATCH 21/23] blockdev: Convert qmp_eject(), qmp_change_blockdev() to BlockBackend, Markus Armbruster, 2014/09/10
[Qemu-devel] [PATCH 16/23] pc87312: Drop unused members of PC87312State, Markus Armbruster, 2014/09/10