Re: [Qemu-devel] [PATCH v15 03/14] block: Replace in_use with operation blocker

[Fam Zheng <famz@redhat.com>]

On Thu, 02/27 13:12, Markus Armbruster wrote:
> Fam Zheng <famz@redhat.com> writes:
> 
> > This drops BlockDriverState.in_use with op_blockers:
> >
> >   - Call bdrv_op_block_all in place of bdrv_set_in_use(bs, 1).
> >   - Call bdrv_op_unblock_all in place of bdrv_set_in_use(bs, 0).
> >   - Check bdrv_op_is_blocked() in place of bdrv_in_use(bs).
> >     The specific types are used, e.g. in place of starting block backup,
> >     bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_BACKUP, ...).
> >   - Check bdrv_op_blocker_is_empty() in place of assert(!bs->in_use).
> >
> > Note: there is only bdrv_op_block_all and bdrv_op_unblock_all callers at
> > this moment. So although the checks are specific to op types, this
> > changes can still be seen as identical logic with previously with
> > in_use. The difference is error message are improved because of blocker
> > error info.
> [...]
> > diff --git a/blockdev.c b/blockdev.c
> > index 357f760..5c5a9c4 100644
> > --- a/blockdev.c
> > +++ b/blockdev.c
> [...]
> > @@ -1723,7 +1722,7 @@ int do_drive_del(Monitor *mon, const QDict *qdict, QObject **ret_data)
> >          qerror_report(QERR_DEVICE_NOT_FOUND, id);
> >          return -1;
> >      }
> > -    if (bdrv_in_use(bs)) {
> > +    if (bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_DRIVE_DEL, NULL)) {
> >          qerror_report(QERR_DEVICE_IN_USE, id);
> >          return -1;
> >      }
> 
> Loses the nice message you stored in bs->blockers[].  You could put it
> to use like this:
> 
>     diff --git a/blockdev.c b/blockdev.c
>     index 0843ca7..4ab9832 100644
>     --- a/blockdev.c
>     +++ b/blockdev.c
>     @@ -1763,6 +1763,7 @@ void qmp_block_set_io_throttle(const char *device, int64_t bps, int64_t bps_rd,
>      int do_drive_del(Monitor *mon, const QDict *qdict, QObject **ret_data)
>      {
>          const char *id = qdict_get_str(qdict, "id");
>     +    Error *local_err = NULL;
>          BlockDriverState *bs;
> 
>          bs = bdrv_find(id);
>     @@ -1770,8 +1771,9 @@ int do_drive_del(Monitor *mon, const QDict *qdict, QObject **ret_data)
>              qerror_report(QERR_DEVICE_NOT_FOUND, id);
>              return -1;
>          }
>     -    if (bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_DRIVE_DEL, NULL)) {
>     -        qerror_report(QERR_DEVICE_IN_USE, id);
>     +    if (bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_DRIVE_DEL, &local_err)) {
>     +        error_report("%s", error_get_pretty(local_err));
>     +        error_free(local_err);
>              return -1;
>          }
> 
> I can do it on top, if you prefer.
> 

Nice. Thanks, will update this patch when respin.

Fam