On Thu, 02/27 13:12, Markus Armbruster wrote:
> Fam Zheng <f...@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
