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

[Max Reitz]

On 22.09.2014 17:06, Markus Armbruster wrote:
> 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 */

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 <address@hidden>
> Thanks!