Re: [Qemu-block] [PATCH 3/7] qcow: Support .bdrv_co_create

[Eric Blake]

On 03/09/2018 03:46 PM, Kevin Wolf wrote:

This adds the .bdrv_co_create driver callback to qcow, which
enables image creation over QMP.

Signed-off-by: Kevin Wolf 
---
  qapi/block-core.json |  21 +-
  block/qcow.c | 196 ++-
  2 files changed, 150 insertions(+), 67 deletions(-)



  ##
+# @BlockdevCreateOptionsQcow:
+#
+# Driver specific image creation options for qcow.
+#
+# @file Node to create the image format on
+# @size Size of the virtual disk in bytes
+# @backing-file File name of the backing file if a backing file
+#   should be used
+# @encrypt  Encryption options if the image should be encrypted


Idea for followup patch - we should strongly document that encryption of 
qcow is discouraged as insecure, and/or mention that qcow2 is generally 
a better option than qcow when creating images over QMP.


--
Eric Blake, Principal Software Engineer
Red Hat, Inc.   +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Re: [Qemu-block] [PATCH 3/7] qcow: Support .bdrv_co_create

[Jeff Cody]

On Fri, Mar 09, 2018 at 10:46:07PM +0100, Kevin Wolf wrote:
> This adds the .bdrv_co_create driver callback to qcow, which
> enables image creation over QMP.
> 
> Signed-off-by: Kevin Wolf 
> ---
>  qapi/block-core.json |  21 +-
>  block/qcow.c | 196 
> ++-
>  2 files changed, 150 insertions(+), 67 deletions(-)
> 
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index d38058eeab..c81677c434 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -3497,6 +3497,25 @@
>  '*cluster-size':'size' } }
>  
>  ##
> +# @BlockdevCreateOptionsQcow:
> +#
> +# Driver specific image creation options for qcow.
> +#
> +# @file Node to create the image format on
> +# @size Size of the virtual disk in bytes
> +# @backing-file File name of the backing file if a backing file
> +#   should be used
> +# @encrypt  Encryption options if the image should be encrypted
> +#
> +# Since: 2.12
> +##
> +{ 'struct': 'BlockdevCreateOptionsQcow',
> +  'data': { 'file': 'BlockdevRef',
> +'size': 'size',
> +'*backing-file':'str',
> +'*encrypt': 'QCryptoBlockCreateOptions' } }
> +
> +##
>  # @BlockdevQcow2Version:
>  #
>  # @v2:  The original QCOW2 format as introduced in qemu 0.10 (version 2)
> @@ -3681,8 +3700,8 @@
>'null-co':'BlockdevCreateNotSupported',
>'nvme':   'BlockdevCreateNotSupported',
>'parallels':  'BlockdevCreateOptionsParallels',
> +  'qcow':   'BlockdevCreateOptionsQcow',
>'qcow2':  'BlockdevCreateOptionsQcow2',
> -  'qcow':   'BlockdevCreateNotSupported',
>'qed':'BlockdevCreateNotSupported',
>'quorum': 'BlockdevCreateNotSupported',
>'raw':'BlockdevCreateNotSupported',
> diff --git a/block/qcow.c b/block/qcow.c
> index 47a18d9a3a..2e3770ca63 100644
> --- a/block/qcow.c
> +++ b/block/qcow.c
> @@ -33,6 +33,8 @@
>  #include 
>  #include "qapi/qmp/qdict.h"
>  #include "qapi/qmp/qstring.h"
> +#include "qapi/qobject-input-visitor.h"
> +#include "qapi/qapi-visit-block-core.h"
>  #include "crypto/block.h"
>  #include "migration/blocker.h"
>  #include "block/crypto.h"
> @@ -86,6 +88,8 @@ typedef struct BDRVQcowState {
>  Error *migration_blocker;
>  } BDRVQcowState;
>  
> +static QemuOptsList qcow_create_opts;
> +
>  static int decompress_cluster(BlockDriverState *bs, uint64_t cluster_offset);
>  
>  static int qcow_probe(const uint8_t *buf, int buf_size, const char *filename)
> @@ -810,62 +814,50 @@ static void qcow_close(BlockDriverState *bs)
>  error_free(s->migration_blocker);
>  }
>  
> -static int coroutine_fn qcow_co_create_opts(const char *filename, QemuOpts 
> *opts,
> -Error **errp)
> +static int coroutine_fn qcow_co_create(BlockdevCreateOptions *opts,
> +   Error **errp)
>  {
> +BlockdevCreateOptionsQcow *qcow_opts;
>  int header_size, backing_filename_len, l1_size, shift, i;
>  QCowHeader header;
>  uint8_t *tmp;
>  int64_t total_size = 0;
> -char *backing_file = NULL;
> -Error *local_err = NULL;
>  int ret;
> +BlockDriverState *bs;
>  BlockBackend *qcow_blk;
> -char *encryptfmt = NULL;
> -QDict *options;
> -QDict *encryptopts = NULL;
> -QCryptoBlockCreateOptions *crypto_opts = NULL;
>  QCryptoBlock *crypto = NULL;
>  
> -/* Read out options */
> -total_size = ROUND_UP(qemu_opt_get_size_del(opts, BLOCK_OPT_SIZE, 0),
> -  BDRV_SECTOR_SIZE);
> +assert(opts->driver == BLOCKDEV_DRIVER_QCOW);
> +qcow_opts = >u.qcow;
> +
> +/* Sanity checks */
> +total_size = qcow_opts->size;
>  if (total_size == 0) {
>  error_setg(errp, "Image size is too small, cannot be zero length");
> -ret = -EINVAL;
> -goto cleanup;
> +return -EINVAL;
>  }
>  
> -backing_file = qemu_opt_get_del(opts, BLOCK_OPT_BACKING_FILE);
> -encryptfmt = qemu_opt_get_del(opts, BLOCK_OPT_ENCRYPT_FORMAT);
> -if (encryptfmt) {
> -if (qemu_opt_get(opts, BLOCK_OPT_ENCRYPT)) {
> -error_setg(errp, "Options " BLOCK_OPT_ENCRYPT " and "
> -   BLOCK_OPT_ENCRYPT_FORMAT " are mutually exclusive");
> -ret = -EINVAL;
> -goto cleanup;
> -}
> -} else if (qemu_opt_get_bool_del(opts, BLOCK_OPT_ENCRYPT, false)) {
> -encryptfmt = g_strdup("aes");
> +if (qcow_opts->has_encrypt &&
> +qcow_opts->encrypt->format != Q_CRYPTO_BLOCK_FORMAT_QCOW)
> +{
> +error_setg(errp, "Unsupported encryption format");
> +return -EINVAL;
>  }
>  
> -ret = bdrv_create_file(filename, opts, _err);
> -if (ret < 0) {
> -error_propagate(errp, local_err);
> -

Re: [Qemu-block] [PATCH 3/7] qcow: Support .bdrv_co_create

[Max Reitz]

On 2018-03-09 22:46, Kevin Wolf wrote:
> This adds the .bdrv_co_create driver callback to qcow, which
> enables image creation over QMP.
> 
> Signed-off-by: Kevin Wolf 
> ---
>  qapi/block-core.json |  21 +-
>  block/qcow.c | 196 
> ++-
>  2 files changed, 150 insertions(+), 67 deletions(-)

Reviewed-by: Max Reitz 

(still not sure about the crumpling)



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [PATCH 3/7] qcow: Support .bdrv_co_create

[Kevin Wolf]

Am 09.03.2018 um 22:58 hat Eric Blake geschrieben:
> On 03/09/2018 03:46 PM, Kevin Wolf wrote:
> > This adds the .bdrv_co_create driver callback to qcow, which
> > enables image creation over QMP.
> > 
> > Signed-off-by: Kevin Wolf 
> > ---
> >   qapi/block-core.json |  21 +-
> >   block/qcow.c | 196 
> > ++-
> >   2 files changed, 150 insertions(+), 67 deletions(-)
> 
> Pre-review question: do we REALLY want to support creation of new qcow
> images from QMP?  Or are we at the point where we want to declare qcow
> a read-only format where we only support it to the extent that you can
> convert an existing qcow file into a better supported format like
> qcow2?

I don't think we want read-only formats if it can be avoided, because
we're in a much worse position to run tests then.

The other option you mentioned in your reply to the qed patch, just not
implementing .bdrv_co_create, but keeping the old callback, would mean
that we'd be stuck in a half-converted state forever. My goal is to get
rid of .bdrv_co_create_opts in the long run.

And actually, qcow and qed were two of the simpler conversions where
little remains to be done before the logic in .bdrv_co_create_opts can
be generalised in block.c.

So I'd just do the conversion.

Kevin

Re: [Qemu-block] [PATCH 3/7] qcow: Support .bdrv_co_create

[Eric Blake]

On 03/09/2018 03:46 PM, Kevin Wolf wrote:

This adds the .bdrv_co_create driver callback to qcow, which
enables image creation over QMP.

Signed-off-by: Kevin Wolf 
---
  qapi/block-core.json |  21 +-
  block/qcow.c | 196 ++-
  2 files changed, 150 insertions(+), 67 deletions(-)


Pre-review question: do we REALLY want to support creation of new qcow 
images from QMP?  Or are we at the point where we want to declare qcow a 
read-only format where we only support it to the extent that you can 
convert an existing qcow file into a better supported format like qcow2?


--
Eric Blake, Principal Software Engineer
Red Hat, Inc.   +1-919-301-3266
Virtualization:  qemu.org | libvirt.org