[Qemu-devel] [PATCH v2] Add 'offset' and 'size' options

[Qemu-devel] [PATCH v2] Add 'offset' and 'size' options

[Tomáš Golembiovský]

This is a follow-up to the patch:
    [PATCH] raw-posix: add 'offset' and 'size' options

The main changes are:
 -  options were moved from 'file' driver into 'raw' driver as suggested
 -  added support for writing, reopen and truncate when possible

If I forgot to address somebody's comments feel free to raise them again,
please.

Some general notes to the code:

1)  The size is rounded *down* to the 512 byte boundary. It's not that
    the raw driver really cares about this, but if we don't do it then 
    bdrv_getlength() will do that instead of us. The problem is that
    bdrv_getlength() does round *up* and this can lead to reads/writes
    outside the specified 'size'.

2)  We don't provide '.bdrv_get_allocated_file_size' function. As a
    result the information about allocated disk size reports size of the
    whole file. This is, rather confusingly, larger than the provided
    'size'. But I don't think this matters much. Note that we don't have
    any easy way how to get the correct information here apart from
    checking all the block with bdrv_co_get_block_status() (as suggested
    by Kevin Wolf).

3)  No options for raw_create(). The 'size' and 'offset' options were
    added only to open/reopen. In my opinion there is no real reason for
    them there. AFAIK you cannot create embeded QCOW2/VMDK/etc. image
    that way anyway.


Tomáš Golembiovský (1):
  raw_bsd: add offset and size options

 block/raw_bsd.c      | 169 ++++++++++++++++++++++++++++++++++++++++++++++++++-
 qapi/block-core.json |  16 ++++-
 2 files changed, 181 insertions(+), 4 deletions(-)

-- 
2.10.0

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

[Tomáš Golembiovský]

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);
+    }
+
+    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;
+    }
+
+    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
-- 
2.10.0

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

[Daniel P. Berrange]

On Tue, Oct 18, 2016 at 12:25:17AM +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).
> 
> 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);

IMHO you should be using error_setg here - I don't see a compelling
reason to let execution continue with incorrect values set.


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

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

[Kevin Wolf]

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

Re: [Qemu-devel] [PATCH v2] Add 'offset' and 'size' options

[Kevin Wolf]

Am 18.10.2016 um 00:25 hat Tomáš Golembiovský geschrieben:
> This is a follow-up to the patch:
>     [PATCH] raw-posix: add 'offset' and 'size' options
> 
> The main changes are:
>  -  options were moved from 'file' driver into 'raw' driver as suggested
>  -  added support for writing, reopen and truncate when possible
> 
> If I forgot to address somebody's comments feel free to raise them again,
> please.
> 
> Some general notes to the code:
> 
> 1)  The size is rounded *down* to the 512 byte boundary. It's not that
>     the raw driver really cares about this, but if we don't do it then 
>     bdrv_getlength() will do that instead of us. The problem is that
>     bdrv_getlength() does round *up* and this can lead to reads/writes
>     outside the specified 'size'.

I think it might be better to just check whether offset/size are
correctly aligned and error out if they aren't.

Then once we made the necessary changes to allow byte alignment (I think
what prevents it is mostly bs->total_sectors, right?) we can allow that
in the raw format driver, which will only make previously failing
options work rather than changing the behaviour of already working
configurations (we can't do the latter without a very good justification
because of compatibility).

> 2)  We don't provide '.bdrv_get_allocated_file_size' function. As a
>     result the information about allocated disk size reports size of the
>     whole file. This is, rather confusingly, larger than the provided
>     'size'. But I don't think this matters much. Note that we don't have
>     any easy way how to get the correct information here apart from
>     checking all the block with bdrv_co_get_block_status() (as suggested
>     by Kevin Wolf).
> 
> 3)  No options for raw_create(). The 'size' and 'offset' options were
>     added only to open/reopen. In my opinion there is no real reason for
>     them there. AFAIK you cannot create embeded QCOW2/VMDK/etc. image
>     that way anyway.

These two things are fine with me.

Kevin