From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:60874) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Z7ndp-0002fR-9k for qemu-devel@nongnu.org; Wed, 24 Jun 2015 12:35:16 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Z7ndk-0000FP-MI for qemu-devel@nongnu.org; Wed, 24 Jun 2015 12:35:13 -0400 Message-ID: <558ADC32.9060205@redhat.com> Date: Wed, 24 Jun 2015 18:34:58 +0200 From: Max Reitz MIME-Version: 1.0 References: <1433816027-32588-1-git-send-email-famz@redhat.com> <1433816027-32588-4-git-send-email-famz@redhat.com> In-Reply-To: <1433816027-32588-4-git-send-email-famz@redhat.com> Content-Type: text/plain; charset=iso-8859-15; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 3/6] block: Extract blockdev part of qmp_drive_mirror List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Fam Zheng , qemu-devel@nongnu.org Cc: Kevin Wolf , qemu-block@nongnu.org, Jeff Cody , Markus Armbruster , Stefan Hajnoczi , pbonzini@redhat.com On 09.06.2015 04:13, Fam Zheng wrote: > This is the part that will be reused by blockdev-mirror. > > Signed-off-by: Fam Zheng > --- > blockdev.c | 158 +++++++++++++++++++++++++++++++++++-------------------------- > 1 file changed, 92 insertions(+), 66 deletions(-) > > diff --git a/blockdev.c b/blockdev.c > index b573e56..c32a9a9 100644 > --- a/blockdev.c > +++ b/blockdev.c > @@ -2883,29 +2883,22 @@ out: > > #define DEFAULT_MIRROR_BUF_SIZE (10 << 20) > > -void qmp_drive_mirror(const char *device, const char *target, > - bool has_format, const char *format, > - bool has_node_name, const char *node_name, > - bool has_replaces, const char *replaces, > - enum MirrorSyncMode sync, > - bool has_mode, enum NewImageMode mode, > - bool has_speed, int64_t speed, > - bool has_granularity, uint32_t granularity, > - bool has_buf_size, int64_t buf_size, > - bool has_on_source_error, BlockdevOnError on_source_error, > - bool has_on_target_error, BlockdevOnError on_target_error, > - Error **errp) > +/* Parameter check and block job starting for mirroring. > + * Caller should hold @device and @target's aio context (They must to be on the > + * same aio context). */ > +static void blockdev_mirror_common(BlockDriverState *bs, > + BlockDriverState *target, > + bool has_replaces, const char *replaces, > + enum MirrorSyncMode sync, > + bool has_speed, int64_t speed, > + bool has_granularity, uint32_t granularity, > + bool has_buf_size, int64_t buf_size, > + bool has_on_source_error, > + BlockdevOnError on_source_error, > + bool has_on_target_error, > + BlockdevOnError on_target_error, > + Error **errp) > { > - BlockBackend *blk; > - BlockDriverState *bs; > - BlockDriverState *source, *target_bs; > - AioContext *aio_context; > - BlockDriver *drv = NULL; > - Error *local_err = NULL; > - QDict *options = NULL; > - int flags; > - int64_t size; > - int ret; > > if (!has_speed) { > speed = 0; > @@ -2916,9 +2909,6 @@ void qmp_drive_mirror(const char *device, const char *target, > if (!has_on_target_error) { > on_target_error = BLOCKDEV_ON_ERROR_REPORT; > } > - if (!has_mode) { > - mode = NEW_IMAGE_MODE_ABSOLUTE_PATHS; > - } > if (!has_granularity) { > granularity = 0; > } > @@ -2936,35 +2926,73 @@ void qmp_drive_mirror(const char *device, const char *target, > return; > } > > - blk = blk_by_name(device); > - if (!blk) { > - error_set(errp, QERR_DEVICE_NOT_FOUND, device); > - return; > - } > - > - aio_context = blk_get_aio_context(blk); > - aio_context_acquire(aio_context); > - > - if (!blk_is_available(blk)) { > - error_set(errp, QERR_DEVICE_HAS_NO_MEDIUM, device); > - goto out; > - } > - bs = blk_bs(blk); > - > - if (!has_format) { > - format = mode == NEW_IMAGE_MODE_EXISTING ? NULL : bs->drv->format_name; > - } > - if (format) { > - drv = bdrv_find_format(format); > - if (!drv) { > - error_set(errp, QERR_INVALID_BLOCK_FORMAT, format); > - goto out; > - } > - } > - > if (bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_MIRROR_SOURCE, errp)) { > + return; > + } > + > + if (!bs->backing_hd && sync == MIRROR_SYNC_MODE_TOP) { > + sync = MIRROR_SYNC_MODE_FULL; > + } > + > + /* pass the node name to replace to mirror start since it's loose coupling > + * and will allow to check whether the node still exist at mirror completion > + */ > + mirror_start(bs, target, > + has_replaces ? replaces : NULL, > + speed, granularity, buf_size, sync, > + on_source_error, on_target_error, > + block_job_cb, bs, errp); > +} > + > +void qmp_drive_mirror(const char *device, const char *target, > + bool has_format, const char *format, > + bool has_node_name, const char *node_name, > + bool has_replaces, const char *replaces, > + enum MirrorSyncMode sync, > + bool has_mode, enum NewImageMode mode, > + bool has_speed, int64_t speed, > + bool has_granularity, uint32_t granularity, > + bool has_buf_size, int64_t buf_size, > + bool has_on_source_error, BlockdevOnError on_source_error, > + bool has_on_target_error, BlockdevOnError on_target_error, > + Error **errp) > +{ > + BlockDriverState *bs; > + BlockBackend *blk; > + BlockDriverState *source, *target_bs; > + AioContext *aio_context; > + BlockDriver *drv = NULL; > + Error *local_err = NULL; > + QDict *options = NULL; > + int flags; > + int64_t size; > + int ret; > + > + blk = blk_by_name(device); > + if (!blk) { > + error_set(errp, QERR_DEVICE_NOT_FOUND, device); > + return; > + } > + > + aio_context = blk_get_aio_context(blk); > + aio_context_acquire(aio_context); > + > + if (!blk_is_available(blk)) { > + error_set(errp, QERR_DEVICE_HAS_NO_MEDIUM, device); > goto out; > } > + bs = blk_bs(blk); > + > + if (!has_format) { > + format = mode == NEW_IMAGE_MODE_EXISTING ? NULL : bs->drv->format_name; > + } > + if (format) { > + drv = bdrv_find_format(format); > + if (!drv) { > + error_set(errp, QERR_INVALID_BLOCK_FORMAT, format); > + goto out; > + } > + } > > flags = bs->open_flags | BDRV_O_RDWR; > source = bs->backing_hd; > @@ -2989,14 +3017,14 @@ void qmp_drive_mirror(const char *device, const char *target, > if (!has_node_name) { > error_setg(errp, "a node-name must be provided when replacing a" > " named node of the graph"); > - goto out; > + return; > } > > to_replace_bs = check_to_replace_node(replaces, &local_err); > > if (!to_replace_bs) { > error_propagate(errp, local_err); > - goto out; > + return; > } > > replace_aio_context = bdrv_get_aio_context(to_replace_bs); > @@ -3007,7 +3035,7 @@ void qmp_drive_mirror(const char *device, const char *target, > if (size != replace_size) { > error_setg(errp, "cannot replace image with a mirror image of " > "different size"); > - goto out; > + return; > } > } There are still a couple of "return;" statements left in the "if (has_replaces)" block. I think they should be replaced, too. Max