Re: [Qemu-devel] [PATCH v4] raw_bsd: add offset and size options

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 4316 bytes --]

On 10/19/2016 07:27 AM, 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).
> 
> When 'size' is specified we do our best to honour it.
> 
> Signed-off-by: Tomáš Golembiovský <tgolembi@redhat.com>
> ---
>  block/raw_bsd.c      | 168 ++++++++++++++++++++++++++++++++++++++++++++++++++-
>  qapi/block-core.json |  16 ++++-
>  2 files changed, 180 insertions(+), 4 deletions(-)
> 
> 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"
>  
> +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 = -EINVAL;
> +        goto fail;
> +    }
> +
> +    /* Make sure size is multiple of BDRV_SECTOR_SIZE to prevent rounding
> +     * up and leaking out of the specified area. */
> +    if (s->size != QEMU_ALIGN_DOWN(s->size, BDRV_SECTOR_SIZE)) {

Better would be:

if (!QEMU_IS_ALIGNED(s->size, BDRV_SECTOR_SIZE)) {

> +        s->size = 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 = -EINVAL;
> +        goto fail;

...you are failing anyways.

> +    }
> +
> +    ret = 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_status(BlockDriverState *bs,
>                                              int nb_sectors, int *pnum,
>                                              BlockDriverState **file)
>  {
> +    BDRVRawState *s = bs->opaque;
>      *pnum = nb_sectors;
>      *file = bs->file->bs;
> +    sector_num += 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(BlockDriverState *bs,
>  
>  static int64_t raw_getlength(BlockDriverState *bs)
>  {
> -    return bdrv_getlength(bs->file->bs);
> +    int64_t len;
> +    BDRVRawState *s = 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 behavior.

> +++ b/qapi/block-core.json
> @@ -2224,6 +2224,20 @@
>    'data': { 'filename': 'str' } }
>  
>  ##
> +# @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'.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]