From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:46840) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Wkuk3-0000pD-I8 for qemu-devel@nongnu.org; Thu, 15 May 2014 08:26:36 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Wkujy-0002WL-Dc for qemu-devel@nongnu.org; Thu, 15 May 2014 08:26:31 -0400 Received: from lputeaux-656-01-25-125.w80-12.abo.wanadoo.fr ([80.12.84.125]:37674 helo=paradis.irqsave.net) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Wkujy-0002WA-4W for qemu-devel@nongnu.org; Thu, 15 May 2014 08:26:26 -0400 Date: Thu, 15 May 2014 14:26:57 +0200 From: =?iso-8859-1?Q?Beno=EEt?= Canet Message-ID: <20140515122657.GH2812@irqsave.net> References: <50765faa0041d43d22dd14a0128d3b55850571ac.1400123059.git.jcody@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline In-Reply-To: <50765faa0041d43d22dd14a0128d3b55850571ac.1400123059.git.jcody@redhat.com> Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH 5/5] block: extend block-commit to accept a string for the backing file List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Jeff Cody Cc: kwolf@redhat.com, benoit.canet@irqsave.net, pkrempa@redhat.com, famz@redhat.com, qemu-devel@nongnu.org, stefanha@redhat.com The Wednesday 14 May 2014 =E0 23:20:19 (-0400), Jeff Cody wrote : > On some image chains, QEMU may not always be able to resolve the > filenames properly, when updating the backing file of an image > after a block commit. >=20 > For instance, certain relative pathnames may fail, or drives may > have been specified originally by file descriptor (e.g. /dev/fd/???), > or a relative protocol pathname may have been used. >=20 > In these instances, QEMU may lack the information to be able to make > the correct choice, but the user or management layer most likely does > have that knowledge. >=20 > With this extension to the block-commit api, the user is able to change > the backing file of the overlay image as part of the block-commit > operation. >=20 > This allows the change to be 'safe', in the sense that if the attempt > to write the overlay image metadata fails, then the block-commit > operation returns failure, without disrupting the guest. >=20 > If the commit top is the active layer, then specifying the backing > file string will be treated as an error (there is no overlay image > to modify in that case). >=20 > If a backing file string is not specified in the command, the backing > file string to use is determined in the same manner as it was > previously. >=20 > Signed-off-by: Jeff Cody > --- > block.c | 8 ++++++-- > block/commit.c | 9 ++++++--- > blockdev.c | 8 +++++++- > include/block/block.h | 3 ++- > include/block/block_int.h | 3 ++- > qapi-schema.json | 18 ++++++++++++++++-- > qmp-commands.hx | 14 +++++++++++++- > 7 files changed, 52 insertions(+), 11 deletions(-) >=20 > diff --git a/block.c b/block.c > index c4f77c2..7e63a1f 100644 > --- a/block.c > +++ b/block.c > @@ -2547,12 +2547,15 @@ typedef struct BlkIntermediateStates { > * > * base <- active > * > + * If backing_file_str is non-NULL, it will be used when modifying top= 's > + * overlay image metadata. > + * > * Error conditions: > * if active =3D=3D top, that is considered an error > * > */ > int bdrv_drop_intermediate(BlockDriverState *active, BlockDriverState = *top, > - BlockDriverState *base) > + BlockDriverState *base, const char *backing= _file_str) > { > BlockDriverState *intermediate; > BlockDriverState *base_bs =3D NULL; > @@ -2604,7 +2607,8 @@ int bdrv_drop_intermediate(BlockDriverState *acti= ve, BlockDriverState *top, > } > =20 > /* success - we can delete the intermediate states, and link top->= base */ > - ret =3D bdrv_change_backing_file(new_top_bs, base_bs->filename, > + backing_file_str =3D backing_file_str ? backing_file_str : base_bs= ->filename; > + ret =3D bdrv_change_backing_file(new_top_bs, backing_file_str, > base_bs->drv ? base_bs->drv->format= _name : ""); > if (ret) { > goto exit; > diff --git a/block/commit.c b/block/commit.c > index 5c09f44..91517d3 100644 > --- a/block/commit.c > +++ b/block/commit.c > @@ -37,6 +37,7 @@ typedef struct CommitBlockJob { > BlockdevOnError on_error; > int base_flags; > int orig_overlay_flags; > + char *backing_file_str; > } CommitBlockJob; > =20 > static int coroutine_fn commit_populate(BlockDriverState *bs, > @@ -141,7 +142,7 @@ wait: > =20 > if (!block_job_is_cancelled(&s->common) && sector_num =3D=3D end) = { > /* success */ > - ret =3D bdrv_drop_intermediate(active, top, base); > + ret =3D bdrv_drop_intermediate(active, top, base, s->backing_f= ile_str); > } > =20 > exit_free_buf: > @@ -158,7 +159,7 @@ exit_restore_reopen: > if (overlay_bs && s->orig_overlay_flags !=3D bdrv_get_flags(overla= y_bs)) { > bdrv_reopen(overlay_bs, s->orig_overlay_flags, NULL); > } > - > + g_free(s->backing_file_str); > block_job_completed(&s->common, ret); > } > =20 > @@ -182,7 +183,7 @@ static const BlockJobDriver commit_job_driver =3D { > void commit_start(BlockDriverState *bs, BlockDriverState *base, > BlockDriverState *top, int64_t speed, > BlockdevOnError on_error, BlockDriverCompletionFunc = *cb, > - void *opaque, Error **errp) > + void *opaque, const char *backing_file_str, Error **= errp) > { > CommitBlockJob *s; > BlockReopenQueue *reopen_queue =3D NULL; > @@ -244,6 +245,8 @@ void commit_start(BlockDriverState *bs, BlockDriver= State *base, > s->base_flags =3D orig_base_flags; > s->orig_overlay_flags =3D orig_overlay_flags; > =20 > + s->backing_file_str =3D g_strdup(backing_file_str); > + > s->on_error =3D on_error; > s->common.co =3D qemu_coroutine_create(commit_run); > =20 > diff --git a/blockdev.c b/blockdev.c > index d8cb396..c0c7867 100644 > --- a/blockdev.c > +++ b/blockdev.c > @@ -1872,6 +1872,7 @@ void qmp_block_commit(const char *device, > bool has_base_node_name, const char *base_node_n= ame, > bool has_top, const char *top, > bool has_top_node_name, const char *top_node_nam= e, > + bool has_backing_file, const char *backing_file, > bool has_speed, int64_t speed, > Error **errp) > { > @@ -1951,11 +1952,16 @@ void qmp_block_commit(const char *device, > } > =20 > if (top_bs =3D=3D bs) { > + if (has_backing_file) { > + error_setg(errp, "'backing-file' specified," > + " but 'top' is the active layer"); > + return; > + } > commit_active_start(bs, base_bs, speed, on_error, block_job_cb= , > bs, &local_err); > } else { > commit_start(bs, base_bs, top_bs, speed, on_error, block_job_c= b, bs, > - &local_err); > + backing_file, &local_err); I don't know QAPI well enough to be sure but are we certain that if=20 has_backing_file =3D=3D false then backing_file =3D=3D NULL and not some random pointer ? If am thinking to add has_backing_file ? backing_file : NULL here. > } > if (local_err !=3D NULL) { > error_propagate(errp, local_err); > diff --git a/include/block/block.h b/include/block/block.h > index 283a6f3..e7aeca6 100644 > --- a/include/block/block.h > +++ b/include/block/block.h > @@ -266,7 +266,8 @@ int bdrv_change_backing_file(BlockDriverState *bs, > const char *backing_file, const char *backing_fmt); > void bdrv_register(BlockDriver *bdrv); > int bdrv_drop_intermediate(BlockDriverState *active, BlockDriverState = *top, > - BlockDriverState *base); > + BlockDriverState *base, > + const char *backing_file_str); > BlockDriverState *bdrv_find_overlay(BlockDriverState *active, > BlockDriverState *bs); > BlockDriverState *bdrv_find_base(BlockDriverState *bs); > diff --git a/include/block/block_int.h b/include/block/block_int.h > index 9ffcb69..8e7dce7 100644 > --- a/include/block/block_int.h > +++ b/include/block/block_int.h > @@ -428,13 +428,14 @@ void stream_start(BlockDriverState *bs, BlockDriv= erState *base, > * @on_error: The action to take upon error. > * @cb: Completion function for the job. > * @opaque: Opaque pointer value passed to @cb. > + * @backing_file_str: String to use as the backing file in @top's over= lay Miss a final dot to be like the other lines. > * @errp: Error object. > * > */ > void commit_start(BlockDriverState *bs, BlockDriverState *base, > BlockDriverState *top, int64_t speed, > BlockdevOnError on_error, BlockDriverCompletionFunc *= cb, > - void *opaque, Error **errp); > + void *opaque, const char *backing_file_str, Error **e= rrp); > /** > * commit_active_start: > * @bs: Active block device to be committed. > diff --git a/qapi-schema.json b/qapi-schema.json > index eddb2b8..ef775fe 100644 > --- a/qapi-schema.json > +++ b/qapi-schema.json > @@ -2118,6 +2118,18 @@ > # topmost data to be committed down. > # (Since 2.1) > # > +# @backing-file: #optional The backing file string to write into the o= verlay > +# image of 'top'. If 'top' is the active lay= er, > +# specifying a backing file string is an erro= r. This > +# backing file string is only written into th= e the > +# image file metadata - internal structures i= nside > +# QEMU are not updated, and the string is no= t validated. > +# If not specified, QEMU will automatically d= etermine > +# the backing file string to use. Care shoul= d be taken > +# when specifying the string, to specify a va= lid filename > +# or protocol. > +# (Since 2.1) > +# > # If top =3D=3D base, that is an error. > # If top =3D=3D active, the job will not be complet= ed by itself, > # user needs to complete the job with the block-job= -complete > @@ -2130,7 +2142,6 @@ > # size of the smaller top, you can safely truncate = it > # yourself once the commit operation successfully c= ompletes. > # > -# Spurious line removal. > # @speed: #optional the maximum speed, in bytes per second > # > # Returns: Nothing on success > @@ -2141,13 +2152,16 @@ > # If @speed is invalid, InvalidParameter > # If both @base and @base-node-name are specified, GenericErr= or > # If both @top and @top-node-name are specified, GenericError > +# If @top or @top-node-name is the active layer, and @backing= -file is > +# specified, GenericError > # > # Since: 1.3 > # > ## > { 'command': 'block-commit', > 'data': { 'device': 'str', '*base': 'str', '*base-node-name': 'str', > - '*top': 'str', '*top-node-name': 'str', '*speed': 'int' } = } > + '*top': 'str', '*top-node-name': 'str', '*backing-file': '= str', > + '*speed': 'int' } } > =20 > ## > # @drive-backup > diff --git a/qmp-commands.hx b/qmp-commands.hx > index 2b9b1dc..e2c446f 100644 > --- a/qmp-commands.hx > +++ b/qmp-commands.hx > @@ -985,7 +985,7 @@ EQMP > =20 > { > .name =3D "block-commit", > - .args_type =3D "device:B,base:s?,base-node-name:s?,top:s?,top= -node-name:s?,speed:o?", > + .args_type =3D "device:B,base:s?,base-node-name:s?,top:s?,top= -node-name:s?,backing-file:s?,speed:o?", > .mhandler.cmd_new =3D qmp_marshal_input_block_commit, > }, > =20 > @@ -1021,6 +1021,18 @@ For 'top', either top or top-node-name must be s= et but not both. > topmost data to be committed down. > (json-string, optional) (Since 2.1) > =20 > +- "backing-file": The backing file string to write into the overlay > + image of 'top'. If 'top' is the active layer, > + specifying a backing file string is an error. This > + backing file string is only written into the the > + image file metadata - internal structures inside > + QEMU are not updated, and the string is not validat= ed. > + If not specified, QEMU will automatically determine > + the backing file string to use. Care should be take= n > + when specifying the string, to specify a valid filen= ame > + or protocol. > + (json-string, optional) (Since 2.1) > + > If top =3D=3D base, that is an error. > If top =3D=3D active, the job will not be completed by itsel= f, > user needs to complete the job with the block-job-complete > --=20 > 1.8.3.1 >=20 >=20