Re: [Qemu-devel] [PATCH] raw-posix: add 'offset' and 'size' options

["Tomáš Golembiovský" <tgolembi@redhat.com>]

On Mon, 3 Oct 2016 09:52:13 +0100
"Daniel P. Berrange" <berrange@redhat.com> wrote:

> 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ý <tgolembi@redhat.com>
> > ---
> >  block/raw-posix.c | 97 +++++++++++++++++++++++++++++++++++++++++++++++++++----
> >  1 file changed, 91 insertions(+), 6 deletions(-)  
> 
> An equivalent change is needed to raw-win32.c

That's what I feared.


> > 
> > 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.

I rather wanted to distinguish  the case when no offset or size was
specified to be able to limit the scope of introduced changes.


> > +
> > +    if (((bs->drv != &bdrv_file) || !bs->read_only) &&  
> 
> Why the check against bdrv_file ?

To limit it only to files. Maybe there is better way to do that? The
devices have a nasty habit to change the size. Sure, this can happen to
file too, e.g. if somebody truncates the file outside QEMU. But that's
rather a bad behaviour. For devices changing the size may be perfectly
valid operation, e.g. replacing CD in drive or card in a card reader.


> 
> > +        ((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.

I still plan to do that, but I didn't feel confident enough to do it in
the first run.

We need to decide what is the correct behaviour here first. Since the
image we're working with is contained in some larger unit it cannot be
resized freely. There are two option that come to my mind:

1) block truncate/grow completely,
2) allow image to be truncated and grown only if the new size does not
   go over the original size.

If we go with the second option then I have a another question. How
strict is (or should be) qemu about the sizes being block aligned? I'm
still little bit fuzzy on that. Somewhere it is assumed that the size is
multiple of 512, sometimes qemu automatically rounds up if it isn't and
sometimes it seems to me that the size can be arbitrary.


> 
> > @@ -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.

Ok, I'll keep just the second one then.

> 
> > +        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.

You're right. I'll fix that.

> 
> > @@ -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/ :|


-- 
Tomáš Golembiovský <tgolembi@redhat.com>