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

[Markus Armbruster]

Fam Zheng <address@hidden> 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.

[...]