From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:36326) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Yif2m-0000AL-Ji for qemu-devel@nongnu.org; Thu, 16 Apr 2015 04:21:08 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Yif2h-0004SN-GQ for qemu-devel@nongnu.org; Thu, 16 Apr 2015 04:21:04 -0400 From: Alberto Garcia In-Reply-To: <552E8225.1070806@redhat.com> References: <09630807f1f0b683168820e53c678d5e12a08342.1428503789.git.berto@igalia.com> <552E8225.1070806@redhat.com> Date: Thu, 16 Apr 2015 10:20:18 +0200 Message-ID: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Subject: Re: [Qemu-devel] [Qemu-block] [PATCH 2/6] block: allow block jobs in any arbitrary node List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Max Reitz , qemu-devel@nongnu.org Cc: Stefan Hajnoczi , qemu-block@nongnu.org On Wed 15 Apr 2015 05:22:13 PM CEST, Max Reitz wrote: >> diff --git a/block/mirror.c b/block/mirror.c >> index 4056164..189e8f8 100644 >> --- a/block/mirror.c >> +++ b/block/mirror.c >> @@ -607,8 +607,9 @@ static void mirror_complete(BlockJob *job, Error **errp) >> return; >> } >> if (!s->synced) { >> - error_set(errp, QERR_BLOCK_JOB_NOT_READY, >> - bdrv_get_device_name(job->bs)); >> + error_setg(errp, >> + "The active block job for device '%s' cannot be completed", >> + bdrv_get_device_name(job->bs)); > > I know there is no way right now that bdrv_get_device_name() returns > an empty string, but somehow I'd feel more consistent for this to be > bdrv_get_device_or_node_name() and the string saying "node" instead of > "device". Your choice, though, since it's completely correct for now. Yeah, I decided to leave it like that while the mirror operation can only work on root nodes. In general in all these patches I'm only using bdrv_get_device_or_node_name() in the cases where it's actually possible that the device name is empty. >> +/* Get the block job for a given device or node name >> + * and acquire its AioContext */ >> +static BlockJob *find_block_job(const char *device_or_node, >> + AioContext **aio_context, >> Error **errp) >> { >> - BlockBackend *blk; >> BlockDriverState *bs; >> >> - blk = blk_by_name(device); >> - if (!blk) { >> + bs = bdrv_lookup_bs(device_or_node, device_or_node, NULL); >> + if (!bs) { >> goto notfound; > > It would be possible to pass errp to bdrv_lookup_bs() and move the > error_set() under notfound to the if (!bs->job) block (I'd find the > error message confusing if there just is no node with that name; but > on the other hand, it wouldn't be a regression). Sounds reasonable, I'll change that. >> diff --git a/blockjob.c b/blockjob.c >> index 562362b..b2b6cc9 100644 >> --- a/blockjob.c >> +++ b/blockjob.c >> @@ -52,7 +52,7 @@ void *block_job_create(const BlockJobDriver *driver, BlockDriverState *bs, >> BlockJob *job; >> >> if (bs->job) { >> - error_set(errp, QERR_DEVICE_IN_USE, bdrv_get_device_name(bs)); >> + error_set(errp, QERR_DEVICE_IN_USE, bdrv_get_device_or_node_name(bs)); > > Hm, the error message only mentions "device" now... Not too nice. Errr... I overlooked that, I'll fix it. >> @@ -1895,7 +1895,7 @@ >> # >> # @type: job type >> # >> -# @device: device name >> +# @device: device name, or node name if not present >> # >> # @len: maximum progress value >> # > > I think you need to apply the same treatment for the comment of > BlockJobInfo, too. You're right again :) Berto