[v2,19/23] blockdev: Drop DriveInfo member enable_auto_del

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(-)

On Sat, 09/13 17:00, 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(-)
> 
> diff --git a/blockdev.c b/blockdev.c
> index 0d99be0..e218c59 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -90,16 +90,14 @@ void blockdev_mark_auto_del(BlockBackend *blk)
>      DriveInfo *dinfo = blk_legacy_dinfo(blk);
>      BlockDriverState *bs = blk_bs(blk);
>  
> -    if (dinfo && !dinfo->enable_auto_del) {
> +    if (!dinfo) {
>          return;
>      }
>  
>      if (bs->job) {
>          block_job_cancel(bs->job);
>      }
> -    if (dinfo) {
> -        dinfo->auto_del = 1;
> -    }
> +    dinfo->auto_del = 1;
>  }

Looks good.

Based on the explanation in the commit message of patch 18, previouly, dinfo is
always non-NULL, so enable_auto_del distinguishes whether the device is created
by drive_new() or qmp_blockdev_add().

Now dinfo is NULL iff created by qmp_blockdev_add(), so enable_auto_del is not
necessary any more.

If I'm reading correctly, the dropped two "if (dinfo.."'s are also redundant.

Reviewed-by: Fam Zheng <famz@redhat.com>

>  
>  void blockdev_auto_del(BlockBackend *blk)
> @@ -900,7 +898,6 @@ DriveInfo *drive_new(QemuOpts *all_opts, BlockInterfaceType block_default_type)
>  
>      /* Set legacy DriveInfo fields */
>      dinfo = g_malloc0(sizeof(*dinfo));
> -    dinfo->enable_auto_del = true;
>      dinfo->opts = all_opts;
>      dinfo->cyls = cyls;
>      dinfo->heads = heads;
> @@ -1716,7 +1713,6 @@ int do_drive_del(Monitor *mon, const QDict *qdict, QObject **ret_data)
>      const char *id = qdict_get_str(qdict, "id");
>      BlockBackend *blk;
>      BlockDriverState *bs;
> -    DriveInfo *dinfo;
>      AioContext *aio_context;
>      Error *local_err = NULL;
>  
> @@ -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;
> diff --git a/include/sysemu/blockdev.h b/include/sysemu/blockdev.h
> index 27a40d5..2129d81 100644
> --- a/include/sysemu/blockdev.h
> +++ b/include/sysemu/blockdev.h
> @@ -35,7 +35,6 @@ struct DriveInfo {
>      int bus;
>      int unit;
>      int auto_del;               /* see blockdev_mark_auto_del() */
> -    bool enable_auto_del;       /* Only for legacy drive_new() */
>      int media_cd;
>      int cyls, heads, secs, trans;
>      QemuOpts *opts;
> -- 
> 1.9.3
> 
>

Fam Zheng <famz@redhat.com> writes:

> On Sat, 09/13 17:00, 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(-)
>> 
>> diff --git a/blockdev.c b/blockdev.c
>> index 0d99be0..e218c59 100644
>> --- a/blockdev.c
>> +++ b/blockdev.c
>> @@ -90,16 +90,14 @@ void blockdev_mark_auto_del(BlockBackend *blk)
>>      DriveInfo *dinfo = blk_legacy_dinfo(blk);
>>      BlockDriverState *bs = blk_bs(blk);
>>  
>> -    if (dinfo && !dinfo->enable_auto_del) {
>> +    if (!dinfo) {
>>          return;
>>      }
>>  
>>      if (bs->job) {
>>          block_job_cancel(bs->job);
>>      }
>> -    if (dinfo) {
>> -        dinfo->auto_del = 1;
>> -    }
>> +    dinfo->auto_del = 1;
>>  }
>
> Looks good.
>
> Based on the explanation in the commit message of patch 18, previouly,
> dinfo is always non-NULL, so enable_auto_del distinguishes whether the
> device is created by drive_new() or qmp_blockdev_add().

Correct.

> Now dinfo is NULL iff created by qmp_blockdev_add(), so
> enable_auto_del is not necessary any more.

Correct again.

> If I'm reading correctly, the dropped two "if (dinfo.."'s are also redundant.

Yes.  Let me explain why.

The two cases to distinguish are "created by drive_new()" and "created
by qmp_blockdev_add()".

Commit 2d246f0 introduced DriveInfo member enable_auto_del to exactly
that.

The code tests for "created by qmp_blockdev_add()" like this:

    dinfo && !dinfo->enable_auto_del

Before PATCH 18, this was actually unnecessarily careful, because dinfo
could never be null, but that's okay.

PATCH 18 changes the "created by qmp_blockdev_add() case: now dinfo is
null there.  The test remains correct, only now its second rather than
first part is unnecessary: dinfo now implies dinfo->enable_auto_del.

Before PATCH 18         dinfo != NULL	!dinfo->enable_auto_del   &&
by drive_new()          true            false                     false
by qmp_blockdev_add()   true            true                      true

After PATCH 18          dinfo != NULL	!dinfo->enable_auto_del   &&
by drive_new()          true            false                     false
by qmp_blockdev_add()   false           N/A                       false

Oops.

After PATCH 19          dinfo                                     !dinfo
by drive_new()          true                                      false
by qmp_blockdev_add()   false                                     true

Looks like I need to squash PATCH 19 into PATCH 18...  Thanks for making
me think!

[...]