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

[Markus Armbruster]

Max Reitz <address@hidden> 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 <address@hidden>
>> ---
>>   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 */


>> 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.

> However, for this patch:
>
> Reviewed-by: Max Reitz <address@hidden>

Thanks!