qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH v2 1/6] block/qdev: Allow node name for drive pr


From: Max Reitz
Subject: Re: [Qemu-devel] [PATCH v2 1/6] block/qdev: Allow node name for drive properties
Date: Tue, 5 Jul 2016 16:41:51 +0200
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.1.1

On 04.07.2016 12:43, Kevin Wolf wrote:
> Am 02.07.2016 um 17:33 hat Max Reitz geschrieben:
>> On 30.06.2016 16:13, Kevin Wolf wrote:
>>> If a node name instead of a BlockBackend name is specified as the driver
>>> for a guest device, an anonymous BlockBackend is created now.
>>>
>>> Signed-off-by: Kevin Wolf <address@hidden>
>>> ---
>>>  hw/core/qdev-properties-system.c | 22 +++++++++++++++++++---
>>>  1 file changed, 19 insertions(+), 3 deletions(-)
>>
>> I just noticed this won't work with (at least) USB mass storage devices.
>> scsi_bus_legacy_add_drive() creates a new device and uses
>> qdev_prop_set_drive() to set its drive, and that function uses
>> blk_name() to get the required value for .drive.
>>
>> That can of course be fixed by using bdrv_get_node_name(blk_bs(value))
>> there if blk_name(value) is empty. But in addition we need to make sure
>> that the BB is not deleted in usb_msd_realize_storage() (because that
>> function has to detach the device from the BB before creating the SCSI
>> disk), so we need to wrap the explicitly hacky block there in
>> blk_ref(blk) and blk_unref(blk).
> 
> I'm not sure if I even want to fix this here... Maybe it's better to
> leave node names broken for usb-storage, as a motivation for someone to
> try and clean up this mess for good. I mean, it's not the first time
> that this hack breaks.

Well, you'll at least have to add the blk_ref()+blk_unref() wrapping, or
we'll get accesses to freed memory.

Realistically speaking, I doubt deliberately making USB mass storage
devices not support the new drive syntax would actually motivate someone
to clean the whole thing up.

In my opinion, just making qdev_prop_set_drive() support anonymous BBs
is simple enough, so it'd be worth it. I don't think we gain anything by
not doing it.

Max

Attachment: signature.asc
Description: OpenPGP digital signature


reply via email to

[Prev in Thread] Current Thread [Next in Thread]