Re: [Qemu-devel] [QEMU PATCH] block: Remove blk_attach_dev_legacy() / legacy_dev code

[Thomas Huth <thuth@redhat.com>]

On 2018-12-18 19:33, Markus Armbruster wrote:
> Thomas Huth <thuth@redhat.com> writes:
> 
>> The last user of blk_attach_dev_legacy() is the code in xen_disk.c.
>> It passes a pointer to a XenBlkDev as second parameter. XenBlkDev
>> is derived from XenDevice which in turn is derived from DeviceState
>> since commit 3a6c9172ac5951e ("xen: create qdev for each backend device").
>> Thus the code can also simply use blk_attach_dev() with a pointer
>> to the DeviceState instead.
>> So we can finally remove all code related to the "legacy_dev" flag, too,
>> and turn the related "void *" in block-backend.c into "DeviceState *"
>> to fix some of the remaining TODOs there.
>>
>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>> ---
>>  Note: I haven't tested the Xen code since I don't have a working Xen
>>  installation at hand. I'd appreciate if someone could check it...
[...]
>> diff --git a/hw/block/xen_disk.c b/hw/block/xen_disk.c
>> index 36eff94..9605caf 100644
>> --- a/hw/block/xen_disk.c
>> +++ b/hw/block/xen_disk.c
>> @@ -801,7 +801,9 @@ static int blk_connect(struct XenDevice *xendev)
>>           * so we can blk_unref() unconditionally */
>>          blk_ref(blkdev->blk);
>>      }
>> -    blk_attach_dev_legacy(blkdev->blk, blkdev);
>> +    if (blk_attach_dev(blkdev->blk, DEVICE(blkdev)) < 0) {
>> +        return -1;
>> +    }
> 
> Other error returns in this function call xen_pv_printf() first.  Should
> this one, too?

Only some of them do a xen_pv_printf() first, there are also many that
don't. blk_attach_dev() currently only returns an error if a device has
already been attach - which should simply never happen here, so a printf
currently does not seem to be justified to me. Alternatively, we could
also abort() here instead, I think. I'll leave that decision up to the
Xen maintainers...

 Thomas