From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:42885) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bwswe-0007Dg-Ax for qemu-devel@nongnu.org; Wed, 19 Oct 2016 11:38:21 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bwswc-0002dE-Tm for qemu-devel@nongnu.org; Wed, 19 Oct 2016 11:38:20 -0400 References: From: Eric Blake Message-ID: Date: Wed, 19 Oct 2016 10:38:10 -0500 MIME-Version: 1.0 In-Reply-To: Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="CAlIcAeAK5LhSDubG6Ub4RqlArkOMpmEU" Subject: Re: [Qemu-devel] [PATCH v4] raw_bsd: add offset and size options List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: =?UTF-8?B?VG9tw6HFoSBHb2xlbWJpb3Zza8O9?= , qemu-devel@nongnu.org Cc: Kevin Wolf , Max Reitz , Markus Armbruster , qemu-block@nongnu.org, "Daniel P . Berrange" This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --CAlIcAeAK5LhSDubG6Ub4RqlArkOMpmEU From: Eric Blake To: =?UTF-8?B?VG9tw6HFoSBHb2xlbWJpb3Zza8O9?= , qemu-devel@nongnu.org Cc: Kevin Wolf , Max Reitz , Markus Armbruster , qemu-block@nongnu.org, "Daniel P . Berrange" Message-ID: Subject: Re: [PATCH v4] raw_bsd: add offset and size options References: In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable On 10/19/2016 07:27 AM, Tom=C3=A1=C5=A1 Golembiovsk=C3=BD wrote: > 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 | 168 +++++++++++++++++++++++++++++++++++++++++++= +++++++- > qapi/block-core.json | 16 ++++- > 2 files changed, 180 insertions(+), 4 deletions(-) >=20 > diff --git a/block/raw_bsd.c b/block/raw_bsd.c > index 588d408..25b5ba8 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; This seems like it is duplicating a struct that QAPI should be able to give us for free, if we would just use it. > + /* 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); No trailing '.' in error_setg() error messages. Also, most uses of PRIu64 and PRId64 are separated by spaces from the rest of the "", rather than adjacent. > + 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)) { Better would be: if (!QEMU_IS_ALIGNED(s->size, BDRV_SECTOR_SIZE)) { > + s->size =3D QEMU_ALIGN_DOWN(s->size, BDRV_SECTOR_SIZE); Dead assignment, since... > + error_setg(errp, "Specified size is not multiple of %llu!", > + BDRV_SECTOR_SIZE); > + ret =3D -EINVAL; > + goto fail; =2E..you are failing anyways. > + } > + > + ret =3D 0; > + > +fail: Naming a label fail: when it is intended to be reached by fallthrough on the success path is annoying. end: might be a better name. > + > + qemu_opts_del(opts); > + > + return ret; > +} > + > @@ -117,8 +239,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; Not your fault (nor necessarily for you to fix), but we should really switch block status code to be byte-based. > return BDRV_BLOCK_RAW | BDRV_BLOCK_OFFSET_VALID | BDRV_BLOCK_DATA = | > (sector_num << BDRV_SECTOR_BITS); > } > @@ -138,7 +262,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 s/unles/unless/ s/externaly/externally/ If the file is being externally modified, the user deserves broken behavi= or. > +++ 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' } } Umm, if you want these to actually be optional, you have to spell them '*offset' and '*size'. --=20 Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org --CAlIcAeAK5LhSDubG6Ub4RqlArkOMpmEU Content-Type: application/pgp-signature; name="signature.asc" Content-Description: OpenPGP digital signature Content-Disposition: attachment; filename="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 Comment: Public key at http://people.redhat.com/eblake/eblake.gpg Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/ iQEcBAEBCAAGBQJYB5NiAAoJEKeha0olJ0Nq0mkIAJu4960a/I8ogEcUv6grxuPB KcroGMEpm7vyuNdDfXxYuQZvEtAePwLnAWT7lnvyenJjjzAxMS7icN3J+cNVQhTV cqq1Nihy29nrQfuUPTNpfPzldF139qm5wy/3Js1xbhV4DomfF18FyZO76UJnjmVs v9M17sK6RDAXOn0th079Ti+jdXhwl4+aGOGm2CrE2SUxsWhvXB265xgWVw1/eEuz Qvj1v9wXPJZw3YDcXV0wWtVxJGNkWeMgTerUMu5hy2EJhywLbh7Y5oXxCCohcyY6 Sz+eVQtxDanwZDJeaMkB7HH1iusJZmNO9xWXSI94OZMNvQW0ChTnDiLWInej5j8= =Htvh -----END PGP SIGNATURE----- --CAlIcAeAK5LhSDubG6Ub4RqlArkOMpmEU--