From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:48525) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bwU3T-0004yM-VG for qemu-devel@nongnu.org; Tue, 18 Oct 2016 09:03:49 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bwU3S-0000hb-5X for qemu-devel@nongnu.org; Tue, 18 Oct 2016 09:03:44 -0400 Date: Tue, 18 Oct 2016 15:03:28 +0200 From: Kevin Wolf Message-ID: <20161018130328.GF4706@noname.str.redhat.com> References: <528780a9a2fd3f7cd759231bba30d3a48901f8be.1476741489.git.tgolembi@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <528780a9a2fd3f7cd759231bba30d3a48901f8be.1476741489.git.tgolembi@redhat.com> Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH v2] raw_bsd: add offset and size options List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: =?utf-8?B?VG9tw6HFoSBHb2xlbWJpb3Zza8O9?= Cc: qemu-devel@nongnu.org, Max Reitz , Markus Armbruster , Eric Blake , qemu-block@nongnu.org Am 18.10.2016 um 00:25 hat Tom=C3=A1=C5=A1 Golembiovsk=C3=BD geschrieben: > Added two new options 'offset' and 'size'. This makes it possible to us= e > only part of the file as a device. This can be used e.g. to limit the > access only to single partition in a disk image or use a disk inside a > tar archive (like OVA). >=20 > When 'size' is specified we do our best to honour it. >=20 > Signed-off-by: Tom=C3=A1=C5=A1 Golembiovsk=C3=BD > --- > block/raw_bsd.c | 169 +++++++++++++++++++++++++++++++++++++++++++= +++++++- > qapi/block-core.json | 16 ++++- > 2 files changed, 181 insertions(+), 4 deletions(-) >=20 > diff --git a/block/raw_bsd.c b/block/raw_bsd.c > index 588d408..3fb3f13 100644 > --- a/block/raw_bsd.c > +++ b/block/raw_bsd.c > @@ -31,6 +31,30 @@ > #include "qapi/error.h" > #include "qemu/option.h" > =20 > +typedef struct BDRVRawState { > + uint64_t offset; > + uint64_t size; > + bool has_size; > +} BDRVRawState; > + > +static QemuOptsList raw_runtime_opts =3D { > + .name =3D "raw", > + .head =3D QTAILQ_HEAD_INITIALIZER(raw_runtime_opts.head), > + .desc =3D { > + { > + .name =3D "offset", > + .type =3D QEMU_OPT_SIZE, > + .help =3D "offset in the disk where the image starts", > + }, > + { > + .name =3D "size", > + .type =3D QEMU_OPT_SIZE, > + .help =3D "virtual disk size", > + }, > + { /* end of list */ } > + }, > +}; > + > static QemuOptsList raw_create_opts =3D { > .name =3D "raw-create-opts", > .head =3D QTAILQ_HEAD_INITIALIZER(raw_create_opts.head), > @@ -44,17 +68,107 @@ static QemuOptsList raw_create_opts =3D { > } > }; > =20 > +static int raw_read_options(QDict *options, BlockDriverState *bs, > + BDRVRawState *s, Error **errp) > +{ > + Error *local_err =3D NULL; > + QemuOpts *opts =3D NULL; > + int64_t real_size =3D 0; > + int ret; > + > + real_size =3D bdrv_getlength(bs->file->bs); > + if (real_size < 0) { > + error_setg_errno(errp, -real_size, "Could not get image size")= ; > + return real_size; > + } > + > + opts =3D qemu_opts_create(&raw_runtime_opts, NULL, 0, &error_abort= ); > + qemu_opts_absorb_qdict(opts, options, &local_err); > + if (local_err) { > + error_propagate(errp, local_err); > + ret =3D -EINVAL; > + goto fail; > + } > + > + s->offset =3D qemu_opt_get_size(opts, "offset", 0); > + if (qemu_opt_find(opts, "size") !=3D NULL) { > + s->size =3D qemu_opt_get_size(opts, "size", 0); > + s->has_size =3D true; > + } else { > + s->has_size =3D false; > + s->size =3D real_size; > + } > + > + /* Check size and offset */ > + if (real_size < s->offset || (real_size - s->offset) < s->size) { > + error_setg(errp, "The sum of offset (%"PRIu64") and size " > + "(%"PRIu64") has to be smaller or equal to the " > + " actual size of the containing file (%"PRId64").", > + s->offset, s->size, real_size); > + ret =3D -EINVAL; > + goto fail; > + } > + > + /* Make sure size is multiple of BDRV_SECTOR_SIZE to prevent round= ing > + * up and leaking out of the specified area. */ > + if (s->size !=3D QEMU_ALIGN_DOWN(s->size, BDRV_SECTOR_SIZE)) { > + s->size =3D QEMU_ALIGN_DOWN(s->size, BDRV_SECTOR_SIZE); > + fprintf(stderr, > + "WARNING: Specified size is not multiple of %llu! " > + "Rounding down to %"PRIu64 ". (End of the image will be " > + "ignored.)\n", > + BDRV_SECTOR_SIZE, s->size); If we wanted this behaviour, this should use error_report() instead of fprintf() so that the message is printed to the monitor if that's where the request came from. But as I already replied on the cover letter, I think we should just make it a hard error. > + } > + > + ret =3D 0; > + > +fail: > + > + qemu_opts_del(opts); > + > + return ret; > +} > + > static int raw_reopen_prepare(BDRVReopenState *reopen_state, > BlockReopenQueue *queue, Error **errp) > { > - return 0; > + assert(reopen_state !=3D NULL); > + assert(reopen_state->bs !=3D NULL); > + > + reopen_state->opaque =3D g_new0(BDRVRawState, 1); > + > + return raw_read_options( > + reopen_state->options, > + reopen_state->bs, > + reopen_state->opaque, > + errp); > +} > + > +static void raw_reopen_commit(BDRVReopenState *state) > +{ > + BDRVRawState *new_s =3D state->opaque; > + BDRVRawState *s =3D state->bs->opaque; > + > + memcpy(s, new_s, sizeof(BDRVRawState)); > + > + g_free(state->opaque); > + state->opaque =3D NULL; > +} > + > +static void raw_reopen_abort(BDRVReopenState *state) > +{ > + g_free(state->opaque); > + state->opaque =3D NULL; > } > =20 > static int coroutine_fn raw_co_preadv(BlockDriverState *bs, uint64_t o= ffset, > uint64_t bytes, QEMUIOVector *qi= ov, > int flags) > { > + BDRVRawState *s =3D bs->opaque; > + > BLKDBG_EVENT(bs->file, BLKDBG_READ_AIO); > + offset +=3D s->offset; > return bdrv_co_preadv(bs->file, offset, bytes, qiov, flags); > } > =20 > @@ -62,11 +176,18 @@ static int coroutine_fn raw_co_pwritev(BlockDriver= State *bs, uint64_t offset, > uint64_t bytes, QEMUIOVector *q= iov, > int flags) > { > + BDRVRawState *s =3D bs->opaque; > void *buf =3D NULL; > BlockDriver *drv; > QEMUIOVector local_qiov; > int ret; > =20 > + if (s->has_size && (offset > s->size || bytes > (s->size - offset)= )) { > + /* There's not enough space for the data. Don't write anything= and just > + * fail to prevent leaking out of the size specified in option= s. */ > + return -ENOSPC; > + } > + > if (bs->probed && offset < BLOCK_PROBE_BUF_SIZE && bytes) { > /* Handling partial writes would be a pain - so we just > * require that guests have 512-byte request alignment if > @@ -101,6 +222,8 @@ static int coroutine_fn raw_co_pwritev(BlockDriverS= tate *bs, uint64_t offset, > qiov =3D &local_qiov; > } > =20 > + offset +=3D s->offset; > + > BLKDBG_EVENT(bs->file, BLKDBG_WRITE_AIO); > ret =3D bdrv_co_pwritev(bs->file, offset, bytes, qiov, flags); > =20 > @@ -117,8 +240,10 @@ static int64_t coroutine_fn raw_co_get_block_statu= s(BlockDriverState *bs, > int nb_sectors, int *pnum, > BlockDriverState **file) > { > + BDRVRawState *s =3D bs->opaque; > *pnum =3D nb_sectors; > *file =3D bs->file->bs; > + sector_num +=3D s->offset / BDRV_SECTOR_SIZE; > return BDRV_BLOCK_RAW | BDRV_BLOCK_OFFSET_VALID | BDRV_BLOCK_DATA = | > (sector_num << BDRV_SECTOR_BITS); > } > @@ -138,7 +263,28 @@ static int coroutine_fn raw_co_pdiscard(BlockDrive= rState *bs, > =20 > static int64_t raw_getlength(BlockDriverState *bs) > { > - return bdrv_getlength(bs->file->bs); > + int64_t len; > + BDRVRawState *s =3D bs->opaque; > + > + /* Update size. It should not change unles the file was externaly > + * modified. */ > + len =3D bdrv_getlength(bs->file->bs); > + if (len < 0) { > + return len; > + } > + > + if (len < s->offset) { > + s->size =3D 0; > + } else { > + if (s->has_size) { > + /* Try to honour the size */ > + s->size =3D MIN(s->size, len - s->offset); > + } else { > + s->size =3D len - s->offset; > + } > + } > + > + return s->size; > } > =20 > static int raw_get_info(BlockDriverState *bs, BlockDriverInfo *bdi) > @@ -158,6 +304,18 @@ static void raw_refresh_limits(BlockDriverState *b= s, Error **errp) > =20 > static int raw_truncate(BlockDriverState *bs, int64_t offset) > { > + BDRVRawState *s =3D bs->opaque; > + > + if (s->has_size) { > + return -ENOTSUP; > + } > + > + if (offset + s->offset < offset) { > + return -EINVAL; > + } I generally don't like overflow checks that use overflows because that works only on unsigned and it's not always obvious whether that's the case or not (and whether it will always stay the case). s->offset is unsigned, so techically I think we're good, though. > + s->size =3D offset; > + offset +=3D s->offset; > return bdrv_truncate(bs->file->bs, offset); > } > =20 > @@ -197,6 +355,8 @@ static int raw_create(const char *filename, QemuOpt= s *opts, Error **errp) > static int raw_open(BlockDriverState *bs, QDict *options, int flags, > Error **errp) > { > + BDRVRawState *s =3D bs->opaque; > + > bs->sg =3D bs->file->bs->sg; > bs->supported_write_flags =3D BDRV_REQ_FUA & > bs->file->bs->supported_write_flags; > @@ -214,7 +374,7 @@ static int raw_open(BlockDriverState *bs, QDict *op= tions, int flags, > bs->file->bs->filename); > } > =20 > - return 0; > + return raw_read_options(options, bs, s, errp); > } > =20 > static void raw_close(BlockDriverState *bs) > @@ -241,8 +401,11 @@ static int raw_probe_geometry(BlockDriverState *bs= , HDGeometry *geo) > =20 > BlockDriver bdrv_raw =3D { > .format_name =3D "raw", > + .instance_size =3D sizeof(BDRVRawState), > .bdrv_probe =3D &raw_probe, > .bdrv_reopen_prepare =3D &raw_reopen_prepare, > + .bdrv_reopen_commit =3D &raw_reopen_commit, > + .bdrv_reopen_abort =3D &raw_reopen_abort, > .bdrv_open =3D &raw_open, > .bdrv_close =3D &raw_close, > .bdrv_create =3D &raw_create, > diff --git a/qapi/block-core.json b/qapi/block-core.json > index 9d797b8..c1dde22 100644 > --- a/qapi/block-core.json > +++ b/qapi/block-core.json > @@ -2224,6 +2224,20 @@ > 'data': { 'filename': 'str' } } > =20 > ## > +# @BlockdevOptionsRaw > +# > +# Driver specific block device options for the raw driver. > +# > +# @offset: #optional position where the block device starts > +# @size: #optional the assumed size of the device > +# > +# Since: 2.8 > +## > +{ 'struct': 'BlockdevOptionsRaw', > + 'base': 'BlockdevOptionsGenericFormat', > + 'data': { 'offset': 'int', 'size': 'int' } } > + > +## > # @BlockdevOptions > # > # Options for creating a block device. Many options are available for= all > @@ -2277,7 +2291,7 @@ > 'qcow': 'BlockdevOptionsGenericCOWFormat', > 'qed': 'BlockdevOptionsGenericCOWFormat', > 'quorum': 'BlockdevOptionsQuorum', > - 'raw': 'BlockdevOptionsGenericFormat', > + 'raw': 'BlockdevOptionsRaw', > # TODO rbd: Wait for structured options > 'replication':'BlockdevOptionsReplication', > # TODO sheepdog: Wait for structured options This looks very close. Kevin