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

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

[Tomáš Golembiovský]

v3 -> v4:
- fix stupid compilation error and formatting issue

v2 -> v3:
- changed overflow check to make it clearer
- produce error instead of warning when size is not multiple of sector
  size

v1 -> v2:
- options were moved from 'file' driver into 'raw' driver as suggested
- added support for writing, reopen and truncate when possible

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

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

-- 
2.10.0

[Qemu-devel] [PATCH v4] 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      | 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;
+
+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,106 @@ 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);
+        error_setg(errp, "Specified size is not multiple of %llu!",
+            BDRV_SECTOR_SIZE);
+        ret = -EINVAL;
+        goto fail;
+    }
+
+    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 +175,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 +221,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 +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;
     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
+     * 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 +303,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 (INT64_MAX - offset < s->offset) {
+        return -EINVAL;
+    }
+
+    s->size = offset;
+    offset += s->offset;
     return bdrv_truncate(bs->file->bs, offset);
 }
 
@@ -197,6 +354,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 +373,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 +400,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 v4] raw_bsd: add offset and size options

[Eric Blake]

[-- 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 --]

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

[Kevin Wolf]

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

Am 19.10.2016 um 17:38 hat Eric Blake geschrieben:
> 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.

Possibly it has the same fields as BlockdevOptionsRaw, but I would use
QAPI structs only for configuring a block device and not for its state
after opening it (i.e. bs->opaque).

So I agree with the explicit definition here.

Kevin

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]