From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:60405) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WLpXo-0000QD-NE for qemu-devel@nongnu.org; Fri, 07 Mar 2014 02:50:13 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1WLpXn-0005xn-LJ for qemu-devel@nongnu.org; Fri, 07 Mar 2014 02:50:12 -0500 Received: from mx1.redhat.com ([209.132.183.28]:52224) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WLpXn-0005w5-Be for qemu-devel@nongnu.org; Fri, 07 Mar 2014 02:50:11 -0500 Date: Fri, 7 Mar 2014 15:50:17 +0800 From: Fam Zheng Message-ID: <20140307075017.GD2514@T430.nay.redhat.com> References: <1393120495-13815-1-git-send-email-famz@redhat.com> <1393120495-13815-4-git-send-email-famz@redhat.com> <8761o0yctj.fsf@blackfin.pond.sub.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <8761o0yctj.fsf@blackfin.pond.sub.org> Subject: Re: [Qemu-devel] [PATCH v15 03/14] block: Replace in_use with operation blocker List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Markus Armbruster Cc: kwolf@redhat.com, benoit.canet@irqsave.net, qemu-devel@nongnu.org, rjones@redhat.com, imain@redhat.com, stefanha@redhat.com, pbonzini@redhat.com, ptoscano@redhat.com On Thu, 02/27 13:12, Markus Armbruster wrote: > Fam Zheng 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