Re: [Qemu-block] [PATCH v4] raw_bsd: add offset and size options
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ý
> > ---
> > 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
pgpfo2HKwnNCl.pgp
Description: PGP signature
Re: [Qemu-block] [PATCH v4] raw_bsd: add offset and size options
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ý
> ---
> 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
signature.asc
Description: OpenPGP digital signature
[Qemu-block] [PATCH v4] raw_bsd: add offset and size options
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ý
---
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
