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

2016-10-18 Thread 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ý 
> ---
>  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 = b

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

2016-10-18 Thread 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ý 
> ---
>  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/ :|

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

2016-10-17 Thread 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ý 
---
 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 spac