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

[Kevin Wolf <kwolf@redhat.com>]

Am 18.10.2016 um 00:25 hat Tomáš Golembiovský geschrieben:
> 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      | 169 ++++++++++++++++++++++++++++++++++++++++++++++++++-
>  qapi/block-core.json |  16 ++++-
>  2 files changed, 181 insertions(+), 4 deletions(-)
> 
> diff --git a/block/raw_bsd.c b/block/raw_bsd.c
> index 588d408..3fb3f13 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;
> +
> +static QemuOptsList raw_runtime_opts = {
> +    .name = "raw",
> +    .head = QTAILQ_HEAD_INITIALIZER(raw_runtime_opts.head),
> +    .desc = {
> +        {
> +            .name = "offset",
> +            .type = QEMU_OPT_SIZE,
> +            .help = "offset in the disk where the image starts",
> +        },
> +        {
> +            .name = "size",
> +            .type = QEMU_OPT_SIZE,
> +            .help = "virtual disk size",
> +        },
> +        { /* end of list */ }
> +    },
> +};
> +
>  static QemuOptsList raw_create_opts = {
>      .name = "raw-create-opts",
>      .head = QTAILQ_HEAD_INITIALIZER(raw_create_opts.head),
> @@ -44,17 +68,107 @@ static QemuOptsList raw_create_opts = {
>      }
>  };
>  
> +static int raw_read_options(QDict *options, BlockDriverState *bs,
> +    BDRVRawState *s, Error **errp)
> +{
> +    Error *local_err = NULL;
> +    QemuOpts *opts = NULL;
> +    int64_t real_size = 0;
> +    int ret;
> +
> +    real_size = bdrv_getlength(bs->file->bs);
> +    if (real_size < 0) {
> +        error_setg_errno(errp, -real_size, "Could not get image size");
> +        return real_size;
> +    }
> +
> +    opts = qemu_opts_create(&raw_runtime_opts, NULL, 0, &error_abort);
> +    qemu_opts_absorb_qdict(opts, options, &local_err);
> +    if (local_err) {
> +        error_propagate(errp, local_err);
> +        ret = -EINVAL;
> +        goto fail;
> +    }
> +
> +    s->offset = qemu_opt_get_size(opts, "offset", 0);
> +    if (qemu_opt_find(opts, "size") != NULL) {
> +        s->size = qemu_opt_get_size(opts, "size", 0);
> +        s->has_size = true;
> +    } else {
> +        s->has_size = false;
> +        s->size = real_size;
> +    }
> +
> +    /* 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);
> +        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)) {
> +        s->size = QEMU_ALIGN_DOWN(s->size, BDRV_SECTOR_SIZE);
> +        fprintf(stderr,
> +            "WARNING: Specified size is not multiple of %llu! "
> +            "Rounding down to %"PRIu64 ". (End of the image will be "
> +            "ignored.)\n",
> +            BDRV_SECTOR_SIZE, s->size);

If we wanted this behaviour, this should use error_report() instead of
fprintf() so that the message is printed to the monitor if that's where
the request came from.

But as I already replied on the cover letter, I think we should just
make it a hard error.

> +    }
> +
> +    ret = 0;
> +
> +fail:
> +
> +    qemu_opts_del(opts);
> +
> +    return ret;
> +}
> +
>  static int raw_reopen_prepare(BDRVReopenState *reopen_state,
>                                BlockReopenQueue *queue, Error **errp)
>  {
> -    return 0;
> +    assert(reopen_state != NULL);
> +    assert(reopen_state->bs != NULL);
> +
> +    reopen_state->opaque = g_new0(BDRVRawState, 1);
> +
> +    return raw_read_options(
> +        reopen_state->options,
> +        reopen_state->bs,
> +        reopen_state->opaque,
> +        errp);
> +}
> +
> +static void raw_reopen_commit(BDRVReopenState *state)
> +{
> +    BDRVRawState *new_s = state->opaque;
> +    BDRVRawState *s = state->bs->opaque;
> +
> +    memcpy(s, new_s, sizeof(BDRVRawState));
> +
> +    g_free(state->opaque);
> +    state->opaque = NULL;
> +}
> +
> +static void raw_reopen_abort(BDRVReopenState *state)
> +{
> +    g_free(state->opaque);
> +    state->opaque = NULL;
>  }
>  
>  static int coroutine_fn raw_co_preadv(BlockDriverState *bs, uint64_t offset,
>                                        uint64_t bytes, QEMUIOVector *qiov,
>                                        int flags)
>  {
> +    BDRVRawState *s = bs->opaque;
> +
>      BLKDBG_EVENT(bs->file, BLKDBG_READ_AIO);
> +    offset += s->offset;
>      return bdrv_co_preadv(bs->file, offset, bytes, qiov, flags);
>  }
>  
> @@ -62,11 +176,18 @@ static int coroutine_fn raw_co_pwritev(BlockDriverState *bs, uint64_t offset,
>                                         uint64_t bytes, QEMUIOVector *qiov,
>                                         int flags)
>  {
> +    BDRVRawState *s = bs->opaque;
>      void *buf = NULL;
>      BlockDriver *drv;
>      QEMUIOVector local_qiov;
>      int ret;
>  
> +    if (s->has_size && (offset > s->size || bytes > (s->size - offset))) {
> +        /* There's not enough space for the data. Don't write anything and just
> +         * fail to prevent leaking out of the size specified in options. */
> +        return -ENOSPC;
> +    }
> +
>      if (bs->probed && offset < BLOCK_PROBE_BUF_SIZE && bytes) {
>          /* Handling partial writes would be a pain - so we just
>           * require that guests have 512-byte request alignment if
> @@ -101,6 +222,8 @@ static int coroutine_fn raw_co_pwritev(BlockDriverState *bs, uint64_t offset,
>          qiov = &local_qiov;
>      }
>  
> +    offset += s->offset;
> +
>      BLKDBG_EVENT(bs->file, BLKDBG_WRITE_AIO);
>      ret = bdrv_co_pwritev(bs->file, offset, bytes, qiov, flags);
>  
> @@ -117,8 +240,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;
>      return BDRV_BLOCK_RAW | BDRV_BLOCK_OFFSET_VALID | BDRV_BLOCK_DATA |
>             (sector_num << BDRV_SECTOR_BITS);
>  }
> @@ -138,7 +263,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
> +     * modified. */
> +    len = bdrv_getlength(bs->file->bs);
> +    if (len < 0) {
> +        return len;
> +    }
> +
> +    if (len < s->offset) {
> +        s->size = 0;
> +    } else {
> +        if (s->has_size) {
> +            /* Try to honour the size */
> +            s->size = MIN(s->size, len - s->offset);
> +        } else {
> +            s->size = len - s->offset;
> +        }
> +    }
> +
> +    return s->size;
>  }
>  
>  static int raw_get_info(BlockDriverState *bs, BlockDriverInfo *bdi)
> @@ -158,6 +304,18 @@ static void raw_refresh_limits(BlockDriverState *bs, Error **errp)
>  
>  static int raw_truncate(BlockDriverState *bs, int64_t offset)
>  {
> +    BDRVRawState *s = bs->opaque;
> +
> +    if (s->has_size) {
> +        return -ENOTSUP;
> +    }
> +
> +    if (offset + s->offset < offset) {
> +        return -EINVAL;
> +    }

I generally don't like overflow checks that use overflows because that
works only on unsigned and it's not always obvious whether that's the
case or not (and whether it will always stay the case).

s->offset is unsigned, so techically I think we're good, though.

> +    s->size = offset;
> +    offset += s->offset;
>      return bdrv_truncate(bs->file->bs, offset);
>  }
>  
> @@ -197,6 +355,8 @@ static int raw_create(const char *filename, QemuOpts *opts, Error **errp)
>  static int raw_open(BlockDriverState *bs, QDict *options, int flags,
>                      Error **errp)
>  {
> +    BDRVRawState *s = bs->opaque;
> +
>      bs->sg = bs->file->bs->sg;
>      bs->supported_write_flags = BDRV_REQ_FUA &
>          bs->file->bs->supported_write_flags;
> @@ -214,7 +374,7 @@ static int raw_open(BlockDriverState *bs, QDict *options, int flags,
>                  bs->file->bs->filename);
>      }
>  
> -    return 0;
> +    return raw_read_options(options, bs, s, errp);
>  }
>  
>  static void raw_close(BlockDriverState *bs)
> @@ -241,8 +401,11 @@ static int raw_probe_geometry(BlockDriverState *bs, HDGeometry *geo)
>  
>  BlockDriver bdrv_raw = {
>      .format_name          = "raw",
> +    .instance_size        = sizeof(BDRVRawState),
>      .bdrv_probe           = &raw_probe,
>      .bdrv_reopen_prepare  = &raw_reopen_prepare,
> +    .bdrv_reopen_commit   = &raw_reopen_commit,
> +    .bdrv_reopen_abort    = &raw_reopen_abort,
>      .bdrv_open            = &raw_open,
>      .bdrv_close           = &raw_close,
>      .bdrv_create          = &raw_create,
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index 9d797b8..c1dde22 100644
> --- a/qapi/block-core.json
> +++ 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' } }
> +
> +##
>  # @BlockdevOptions
>  #
>  # Options for creating a block device.  Many options are available for all
> @@ -2277,7 +2291,7 @@
>        'qcow':       'BlockdevOptionsGenericCOWFormat',
>        'qed':        'BlockdevOptionsGenericCOWFormat',
>        'quorum':     'BlockdevOptionsQuorum',
> -      'raw':        'BlockdevOptionsGenericFormat',
> +      'raw':        'BlockdevOptionsRaw',
>  # TODO rbd: Wait for structured options
>        'replication':'BlockdevOptionsReplication',
>  # TODO sheepdog: Wait for structured options

This looks very close.

Kevin