From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:58796) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bqyz6-0003LJ-OE for qemu-devel@nongnu.org; Mon, 03 Oct 2016 04:52:30 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bqyz4-000095-8K for qemu-devel@nongnu.org; Mon, 03 Oct 2016 04:52:27 -0400 Date: Mon, 3 Oct 2016 09:52:13 +0100 From: "Daniel P. Berrange" Message-ID: <20161003085213.GA13491@redhat.com> Reply-To: "Daniel P. Berrange" References: <598de7ff27e32fcb1b7f677f40fb8da4f0a1f512.1475434971.git.tgolembi@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <598de7ff27e32fcb1b7f677f40fb8da4f0a1f512.1475434971.git.tgolembi@redhat.com> Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH] raw-posix: add 'offset' and 'size' options List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: =?utf-8?B?VG9tw6HFoSBHb2xlbWJpb3Zza8O9?= Cc: qemu-block@nongnu.org, Kevin Wolf , qemu-devel@nongnu.org, Max Reitz On Sun, Oct 02, 2016 at 09:13:29PM +0200, 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 > For now this is only possible for files in read-only mode. It should be > possible to extend it later to allow read-write mode, but would probabl= y > require that the size of the device is kept constant (i.e. no resizing)= . >=20 > Signed-off-by: Tom=C3=A1=C5=A1 Golembiovsk=C3=BD > --- > block/raw-posix.c | 97 +++++++++++++++++++++++++++++++++++++++++++++++= ++++---- > 1 file changed, 91 insertions(+), 6 deletions(-) An equivalent change is needed to raw-win32.c >=20 > diff --git a/block/raw-posix.c b/block/raw-posix.c > index 6ed7547..36f2712 100644 > --- a/block/raw-posix.c > +++ b/block/raw-posix.c > @@ -421,6 +435,17 @@ static int raw_open_common(BlockDriverState *bs, Q= Dict *options, > goto fail; > } > =20 > + s->offset =3D qemu_opt_get_size(opts, "offset", 0); > + s->assumed_size =3D qemu_opt_get_size(opts, BLOCK_OPT_SIZE, 0); If the user didn't provide "size", then we should initialize it to the actual size of the underlying storage, rather than to 0. > + > + if (((bs->drv !=3D &bdrv_file) || !bs->read_only) && Why the check against bdrv_file ? > + ((s->offset > 0) || (s->assumed_size > 0))) { > + error_setg(errp, "offset and size options are allowed only for= " > + "files in read-only mode"); > + ret =3D -EINVAL; > + goto fail; > + } Why did you restrict it to read-only instead of adding the offset logic to the write & truncate methods. IMHO if we're going to add this feature we should make it work in all scenarios, not just do 1/2 the job. > @@ -443,6 +468,23 @@ static int raw_open_common(BlockDriverState *bs, Q= Dict *options, > } > s->fd =3D fd; > =20 > + /* Check size and offset */ > + real_size =3D raw_getlength_real(bs); > + if (real_size < (s->offset + s->assumed_size)) { There's risk of integer overflow here. > + if (s->assumed_size =3D=3D 0) { > + error_setg(errp, "The offset has to be smaller than actual= size " > + "of the containing file (%ld) ", > + real_size); > + } else { > + error_setg(errp, "The sum of offset (%lu) and size (%lu) h= as to " > + "be smaller than actual size of the conta= ining " > + "file (%ld) ", > + s->offset, s->assumed_size, real_size); > + } Not sure I see the point in special-casing assumed_size =3D=3D 0 here, as the second error message is clearer IMHO. > + ret =3D -EINVAL; > + goto fail; > + } > + > #ifdef CONFIG_LINUX_AIO > if (!raw_use_aio(bdrv_flags) && (bdrv_flags & BDRV_O_NATIVE_AIO)) = { > error_setg(errp, "aio=3Dnative was specified, but it requires = " > @@ -1271,6 +1313,19 @@ static int coroutine_fn raw_co_preadv(BlockDrive= rState *bs, uint64_t offset, > uint64_t bytes, QEMUIOVector *qi= ov, > int flags) > { > + BDRVRawState *s =3D bs->opaque; > + if (s->assumed_size > 0) { > + if (offset > s->assumed_size) { > + /* Attempt to read beyond EOF */ > + return 0; > + } else if ((offset + bytes) > s->assumed_size) { Integer overflow risk again here > + /* Trying to read more than is available */ > + bytes =3D s->assumed_size - offset; > + } > + } > + > + offset +=3D s->offset; > + > return raw_co_prw(bs, offset, bytes, qiov, QEMU_AIO_READ); > } > =20 > @@ -1516,6 +1571,28 @@ static int64_t raw_getlength(BlockDriverState *b= s) > } > #endif > =20 > +static int64_t raw_getlength(BlockDriverState *bs) > +{ > + BDRVRawState *s =3D bs->opaque; > + > + if (s->assumed_size > 0) { > + return (int64_t)s->assumed_size; > + } > + > + int64_t size =3D raw_getlength_real(bs); > + if (s->offset > 0) { > + if (s->offset > size) { > + /* The size has changed! We didn't expect that. */ > + return -EIO; > + } > + size -=3D s->offset; > + } The 'if (s->offset > 0)' check doesn't seem to do anything useful here - if offset is zero, then the if body is a no-op already. > @@ -1524,7 +1601,15 @@ static int64_t raw_get_allocated_file_size(Block= DriverState *bs) > if (fstat(s->fd, &st) < 0) { > return -errno; > } > - return (int64_t)st.st_blocks * 512; > + uint64_t size =3D st.st_blocks * 512; > + /* If the file is sparse we have no idea which part of the file is > + * allocated and which is not. So we just make sure the returned v= alue is > + * not greater than what we're working with. > + */ > + if (s->assumed_size > 0 && s->assumed_size < size) { > + size =3D s->assumed_size; > + } > + return (int64_t)size; > } > =20 > static int raw_create(const char *filename, QemuOpts *opts, Error **er= rp) > --=20 > 2.10.0 >=20 >=20 Regards, Daniel --=20 |: http://berrange.com -o- http://www.flickr.com/photos/dberrange= / :| |: http://libvirt.org -o- http://virt-manager.or= g :| |: http://entangle-photo.org -o- http://search.cpan.org/~danberr= / :|