Re: [Qemu-devel] [PATCH v3 19/23] blockdev: Drop DriveInfo member enable_auto_del

[Max Reitz <mreitz@redhat.com>]

On 22.09.2014 17:06, Markus Armbruster wrote:
> Max Reitz <mreitz@redhat.com> writes:
>
>> On 16.09.2014 20:12, Markus Armbruster wrote:
>>> Commit 2d246f0 introduced DriveInfo member enable_auto_del to
>>> distinguish DriveInfo created via drive_new() from DriveInfo created
>>> via qmp_blockdev_add().  The latter no longer exist.  Drop
>>> enable_auto_del.
>>>
>>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>>> ---
>>>    blockdev.c                | 11 +++--------
>>>    include/sysemu/blockdev.h |  1 -
>>>    2 files changed, 3 insertions(+), 9 deletions(-)
>> I would've liked some comment somewhere about DriveInfo's presence
>> corresponding with the drive having been created through drive_new(),
>> but I can live without, too.
> Fair request.  I could squash the following into PATCH 18:
>
> diff --git a/block/block-backend.c b/block/block-backend.c
> index 5646628..5e4ecd7 100644
> --- a/block/block-backend.c
> +++ b/block/block-backend.c
> @@ -22,7 +22,7 @@ struct BlockBackend {
>       char *name;
>       int refcnt;
>       BlockDriverState *bs;
> -    DriveInfo *legacy_dinfo;
> +    DriveInfo *legacy_dinfo;    /* null unless created by drive_new() */
>       QTAILQ_ENTRY(BlockBackend) link; /* for blk_backends */
>   
>       void *dev;                  /* attached device model, if any */

Yes, that would be nice, thank you!

>>> diff --git a/blockdev.c b/blockdev.c
>>> index 0d99be0..e218c59 100644
>>> --- a/blockdev.c
>>> +++ b/blockdev.c
>> [snip]
>>
>>> @@ -1727,8 +1723,7 @@ int do_drive_del(Monitor *mon, const QDict *qdict, QObject **ret_data)
>>>        }
>>>        bs = blk_bs(blk);
>>>    -    dinfo = blk_legacy_dinfo(blk);
>>> -    if (dinfo && !dinfo->enable_auto_del) {
>>> +    if (!blk_legacy_dinfo(blk)) {
>>>            error_report("Deleting device added with blockdev-add"
>>>                         " is not supported");
>>>            return -1;
>> This doesn't look like a 1-to-1 correspondence. Before this patch, if
>> DriveInfo was not present, the condition was false (actually, it was
>> always false, which is the reason for this patch). Now it's true. It
>> seems like the behavior is now correct but wasn't before... I guess
>> this means patch 18 should be fixed?
> PATCH 18 is broken.  See also my reply to Fam on v2 of this patch.

Hm, okay. Fine with me. I don't object to squashing 19 into 18, either.

Max

>> However, for this patch:
>>
>> Reviewed-by: Max Reitz <mreitz@redhat.com>
> Thanks!