[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH] raw-posix: add 'offset' and 'size' options
From: |
Daniel P. Berrange |
Subject: |
Re: [Qemu-devel] [PATCH] raw-posix: add 'offset' and 'size' options |
Date: |
Mon, 3 Oct 2016 09:52:13 +0100 |
User-agent: |
Mutt/1.7.0 (2016-08-17) |
On Sun, Oct 02, 2016 at 09:13:29PM +0200, Tomáš Golembiovský wrote:
> Added two new options 'offset' and 'size'. This makes it possible to use
> 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).
>
> 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 probably
> require that the size of the device is kept constant (i.e. no resizing).
>
> Signed-off-by: Tomáš Golembiovský <address@hidden>
> ---
> block/raw-posix.c | 97
> +++++++++++++++++++++++++++++++++++++++++++++++++++----
> 1 file changed, 91 insertions(+), 6 deletions(-)
An equivalent change is needed to raw-win32.c
>
> 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, QDict
> *options,
> goto fail;
> }
>
> + s->offset = qemu_opt_get_size(opts, "offset", 0);
> + s->assumed_size = 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 != &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 = -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, QDict
> *options,
> }
> s->fd = fd;
>
> + /* Check size and offset */
> + real_size = raw_getlength_real(bs);
> + if (real_size < (s->offset + s->assumed_size)) {
There's risk of integer overflow here.
> + if (s->assumed_size == 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) has to "
> + "be smaller than actual size of the containing "
> + "file (%ld) ",
> + s->offset, s->assumed_size, real_size);
> + }
Not sure I see the point in special-casing assumed_size == 0 here, as
the second error message is clearer IMHO.
> + ret = -EINVAL;
> + goto fail;
> + }
> +
> #ifdef CONFIG_LINUX_AIO
> if (!raw_use_aio(bdrv_flags) && (bdrv_flags & BDRV_O_NATIVE_AIO)) {
> error_setg(errp, "aio=native was specified, but it requires "
> @@ -1271,6 +1313,19 @@ static int coroutine_fn raw_co_preadv(BlockDriverState
> *bs, uint64_t offset,
> uint64_t bytes, QEMUIOVector *qiov,
> int flags)
> {
> + BDRVRawState *s = 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 = s->assumed_size - offset;
> + }
> + }
> +
> + offset += s->offset;
> +
> return raw_co_prw(bs, offset, bytes, qiov, QEMU_AIO_READ);
> }
>
> @@ -1516,6 +1571,28 @@ static int64_t raw_getlength(BlockDriverState *bs)
> }
> #endif
>
> +static int64_t raw_getlength(BlockDriverState *bs)
> +{
> + BDRVRawState *s = bs->opaque;
> +
> + if (s->assumed_size > 0) {
> + return (int64_t)s->assumed_size;
> + }
> +
> + int64_t size = raw_getlength_real(bs);
> + if (s->offset > 0) {
> + if (s->offset > size) {
> + /* The size has changed! We didn't expect that. */
> + return -EIO;
> + }
> + size -= 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(BlockDriverState *bs)
> if (fstat(s->fd, &st) < 0) {
> return -errno;
> }
> - return (int64_t)st.st_blocks * 512;
> + uint64_t size = 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 value is
> + * not greater than what we're working with.
> + */
> + if (s->assumed_size > 0 && s->assumed_size < size) {
> + size = s->assumed_size;
> + }
> + return (int64_t)size;
> }
>
> static int raw_create(const char *filename, QemuOpts *opts, Error **errp)
> --
> 2.10.0
>
>
Regards,
Daniel
--
|: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org -o- http://virt-manager.org :|
|: http://entangle-photo.org -o- http://search.cpan.org/~danberr/ :|
- [Qemu-devel] [PATCH] raw-posix: add 'offset' and 'size' options, Tomáš Golembiovský, 2016/10/02
- Re: [Qemu-devel] [PATCH] raw-posix: add 'offset' and 'size' options,
Daniel P. Berrange <=
- Re: [Qemu-devel] [PATCH] raw-posix: add 'offset' and 'size' options, Tomáš Golembiovský, 2016/10/03
- Re: [Qemu-devel] [PATCH] raw-posix: add 'offset' and 'size' options, Daniel P. Berrange, 2016/10/03
- Re: [Qemu-devel] [PATCH] raw-posix: add 'offset' and 'size' options, Tomáš Golembiovský, 2016/10/03
- Re: [Qemu-devel] [PATCH] raw-posix: add 'offset' and 'size' options, Daniel P. Berrange, 2016/10/03
- Re: [Qemu-devel] [PATCH] raw-posix: add 'offset' and 'size' options, Kevin Wolf, 2016/10/04
- Re: [Qemu-devel] [PATCH] raw-posix: add 'offset' and 'size' options, Daniel P. Berrange, 2016/10/04
- Re: [Qemu-devel] [PATCH] raw-posix: add 'offset' and 'size' options, Eric Blake, 2016/10/04