Re: [Qemu-devel] [PATCH 11/27] block: x-blockdev-create QMP command

2018-02-21 Thread Eric Blake 

On 02/21/2018 04:29 AM, Kevin Wolf wrote:


+++ b/include/block/block_int.h
@@ -130,6 +130,8 @@ struct BlockDriver {
   int (*bdrv_file_open)(BlockDriverState *bs, QDict *options, int flags,
 Error **errp);
   void (*bdrv_close)(BlockDriverState *bs);
+int coroutine_fn (*bdrv_co_create)(BlockdevCreateOptions *opts,
+   Error **errp);


I know we haven't been very good in the past, but can you add a comment here
on the contract that drivers are supposed to obey when implementing this
callback?


Anything specific you want to see here?

Essentially the meaning of BlockdevCreateOptions depends on the driver
and is documented in the QAPI schema, how Error works is common
knowledge, and I don't see much else to explain here.

I mean, I can add something like "Creates an image. See the QAPI
documentation for BlockdevCreateOptions for details." if you think this
is useful. But is it?


I guess my concern is whether this interface MUST overwrite any existing 
data in order to convert existing storage into a newly-created image of 
this driver's type (even if the overwritten data previously probed as a 
different image type), or if it is only called at a point when any 
existing data would be probed as raw, or any other useful tidbits that a 
driver might need to know in implementing it.  But if all you can think 
of is "See QAPI for BlockdevCreateOptions for details", then yeah, 
that's not worth a comment.





+/* Call callback if it exists */
+if (!drv->bdrv_co_create) {
+error_setg(errp, "Driver does not support blockdev-create");


Should this error message refer to 'x-blockdev-create' in the short term?


Hm, it would be more correct. On the other hand, I'm almost sure we'd
forget to rename it back when we remove the x- prefix...


Good point.  And being an x- prefix implies that inconsistency may be 
expected (not to mention short-lived, if we promote the interface); 
while being consistent now but risking long-term inconsistency down the 
road when it actually becomes a stable interface is indeed worse.  So 
keep this message as-is.


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

Re: [Qemu-devel] [PATCH 11/27] block: x-blockdev-create QMP command

2018-02-21 Thread Kevin Wolf 
Am 15.02.2018 um 20:58 hat Eric Blake geschrieben:
> On 02/08/2018 01:23 PM, Kevin Wolf wrote:
> > This adds a synchronous x-blockdev-create QMP command that can create
> > qcow2 images on a given node name.
> > 
> > We don't want to block while creating an image, so this is not the final
> > interface in all aspects, but BlockdevCreateOptionsQcow2 and
> > .bdrv_co_create() are what they actually might look like in the end. In
> > any case, this should be good enough to test whether we interpret
> > BlockdevCreateOptions as we should.
> > 
> > Signed-off-by: Kevin Wolf 
> > ---
> 
> > @@ -3442,6 +3442,18 @@
> > } }
> >   ##
> > +# @x-blockdev-create:
> > +#
> > +# Create an image format on a given node.
> > +# TODO Replace with something asynchronous (block job?)
> 
> We've learned our lesson - don't commit to the final name on an interface
> that we have not yet experimented with.
> 
> > +#
> > +# Since: 2.12
> > +##
> > +{ 'command': 'x-blockdev-create',
> > +  'data': 'BlockdevCreateOptions',
> > +  'boxed': true }
> > +
> 
> Lots of code packed in that little description ;)
> 
> 
> > +++ b/include/block/block_int.h
> > @@ -130,6 +130,8 @@ struct BlockDriver {
> >   int (*bdrv_file_open)(BlockDriverState *bs, QDict *options, int flags,
> > Error **errp);
> >   void (*bdrv_close)(BlockDriverState *bs);
> > +int coroutine_fn (*bdrv_co_create)(BlockdevCreateOptions *opts,
> > +   Error **errp);
> 
> I know we haven't been very good in the past, but can you add a comment here
> on the contract that drivers are supposed to obey when implementing this
> callback?

Anything specific you want to see here?

Essentially the meaning of BlockdevCreateOptions depends on the driver
and is documented in the QAPI schema, how Error works is common
knowledge, and I don't see much else to explain here.

I mean, I can add something like "Creates an image. See the QAPI
documentation for BlockdevCreateOptions for details." if you think this
is useful. But is it?

> > +/* Call callback if it exists */
> > +if (!drv->bdrv_co_create) {
> > +error_setg(errp, "Driver does not support blockdev-create");
> 
> Should this error message refer to 'x-blockdev-create' in the short term?

Hm, it would be more correct. On the other hand, I'm almost sure we'd
forget to rename it back when we remove the x- prefix...

Kevin

Re: [Qemu-devel] [PATCH 11/27] block: x-blockdev-create QMP command

2018-02-15 Thread Eric Blake 

On 02/08/2018 01:23 PM, Kevin Wolf wrote:

This adds a synchronous x-blockdev-create QMP command that can create
qcow2 images on a given node name.

We don't want to block while creating an image, so this is not the final
interface in all aspects, but BlockdevCreateOptionsQcow2 and
.bdrv_co_create() are what they actually might look like in the end. In
any case, this should be good enough to test whether we interpret
BlockdevCreateOptions as we should.

Signed-off-by: Kevin Wolf 
---



@@ -3442,6 +3442,18 @@
} }
  
  ##

+# @x-blockdev-create:
+#
+# Create an image format on a given node.
+# TODO Replace with something asynchronous (block job?)


We've learned our lesson - don't commit to the final name on an 
interface that we have not yet experimented with.



+#
+# Since: 2.12
+##
+{ 'command': 'x-blockdev-create',
+  'data': 'BlockdevCreateOptions',
+  'boxed': true }
+


Lots of code packed in that little description ;)



+++ b/include/block/block_int.h
@@ -130,6 +130,8 @@ struct BlockDriver {
  int (*bdrv_file_open)(BlockDriverState *bs, QDict *options, int flags,
Error **errp);
  void (*bdrv_close)(BlockDriverState *bs);
+int coroutine_fn (*bdrv_co_create)(BlockdevCreateOptions *opts,
+   Error **errp);


I know we haven't been very good in the past, but can you add a comment 
here on the contract that drivers are supposed to obey when implementing 
this callback?



+++ b/block.c
@@ -369,7 +369,7 @@ BlockDriver *bdrv_find_format(const char *format_name)
  return bdrv_do_find_format(format_name);
  }
  
-static int bdrv_is_whitelisted(BlockDriver *drv, bool read_only)

+int bdrv_is_whitelisted(BlockDriver *drv, bool read_only)


Worth mentioning that bdrv_is_whitelisted had to be exported as part of 
the commit message?  (Or even promoting it to public in a separate commit?)



+++ b/block/create.c
@@ -0,0 +1,75 @@
+/*
+ * Block layer code related to image creation
+ *
+ * Copyright (c) 2018 Kevin Wolf 


The question came up in another thread, but I didn't hear your answer 
there; I know Red Hat permits you to claim personal copyright while 
still using a redhat.com address for code written on personal time, but 
should this claim belong to Red Hat instead of you?



+/* Call callback if it exists */
+if (!drv->bdrv_co_create) {
+error_setg(errp, "Driver does not support blockdev-create");


Should this error message refer to 'x-blockdev-create' in the short term?

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

Re: [Qemu-devel] [PATCH 11/27] block: x-blockdev-create QMP command

2018-02-12 Thread Max Reitz 
On 2018-02-08 20:23, Kevin Wolf wrote:
> This adds a synchronous x-blockdev-create QMP command that can create
> qcow2 images on a given node name.
> 
> We don't want to block while creating an image, so this is not the final
> interface in all aspects, but BlockdevCreateOptionsQcow2 and
> .bdrv_co_create() are what they actually might look like in the end. In
> any case, this should be good enough to test whether we interpret
> BlockdevCreateOptions as we should.
> 
> Signed-off-by: Kevin Wolf 
> ---
>  qapi/block-core.json  | 12 
>  include/block/block.h |  1 +
>  include/block/block_int.h |  2 ++
>  block.c   |  2 +-
>  block/create.c| 75 
> +++
>  block/qcow2.c |  3 +-
>  block/Makefile.objs   |  2 +-
>  7 files changed, 94 insertions(+), 3 deletions(-)
>  create mode 100644 block/create.c

[...]

> diff --git a/block/create.c b/block/create.c
> new file mode 100644
> index 00..e95446a0f3
> --- /dev/null
> +++ b/block/create.c
> @@ -0,0 +1,75 @@

[...]

> +void qmp_x_blockdev_create(BlockdevCreateOptions *options, Error **errp)
> +{
> +const char *fmt = BlockdevDriver_str(options->driver);
> +BlockDriver *drv = bdrv_find_format(fmt);
> +Coroutine *co;
> +BlockdevCreateCo cco;
> +
> +/* If the driver is in the schema, we know that it exists. But it may not
> + * be whitelisted. */
> +assert(drv);
> +if (bdrv_uses_whitelist() && !bdrv_is_whitelisted(drv, true)) {

Isn't this more of an R/W case than RO?

Max

> +error_setg(errp, "Driver is not whitelisted");
> +return;
> +}



signature.asc
Description: OpenPGP digital signature

[Qemu-devel] [PATCH 11/27] block: x-blockdev-create QMP command

2018-02-08 Thread Kevin Wolf 
This adds a synchronous x-blockdev-create QMP command that can create
qcow2 images on a given node name.

We don't want to block while creating an image, so this is not the final
interface in all aspects, but BlockdevCreateOptionsQcow2 and
.bdrv_co_create() are what they actually might look like in the end. In
any case, this should be good enough to test whether we interpret
BlockdevCreateOptions as we should.

Signed-off-by: Kevin Wolf 
---
 qapi/block-core.json  | 12 
 include/block/block.h |  1 +
 include/block/block_int.h |  2 ++
 block.c   |  2 +-
 block/create.c| 75 +++
 block/qcow2.c |  3 +-
 block/Makefile.objs   |  2 +-
 7 files changed, 94 insertions(+), 3 deletions(-)
 create mode 100644 block/create.c

diff --git a/qapi/block-core.json b/qapi/block-core.json
index aade602a04..c0e61483af 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -3442,6 +3442,18 @@
   } }
 
 ##
+# @x-blockdev-create:
+#
+# Create an image format on a given node.
+# TODO Replace with something asynchronous (block job?)
+#
+# Since: 2.12
+##
+{ 'command': 'x-blockdev-create',
+  'data': 'BlockdevCreateOptions',
+  'boxed': true }
+
+##
 # @blockdev-open-tray:
 #
 # Opens a block device's tray. If there is a block driver state tree inserted 
as
diff --git a/include/block/block.h b/include/block/block.h
index 4b11f814a8..fdc76f1735 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -238,6 +238,7 @@ char *bdrv_perm_names(uint64_t perm);
 void bdrv_init(void);
 void bdrv_init_with_whitelist(void);
 bool bdrv_uses_whitelist(void);
+int bdrv_is_whitelisted(BlockDriver *drv, bool read_only);
 BlockDriver *bdrv_find_protocol(const char *filename,
 bool allow_protocol_prefix,
 Error **errp);
diff --git a/include/block/block_int.h b/include/block/block_int.h
index 29cafa4236..a9f144d7bd 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -130,6 +130,8 @@ struct BlockDriver {
 int (*bdrv_file_open)(BlockDriverState *bs, QDict *options, int flags,
   Error **errp);
 void (*bdrv_close)(BlockDriverState *bs);
+int coroutine_fn (*bdrv_co_create)(BlockdevCreateOptions *opts,
+   Error **errp);
 int (*bdrv_create)(const char *filename, QemuOpts *opts, Error **errp);
 int (*bdrv_make_empty)(BlockDriverState *bs);
 
diff --git a/block.c b/block.c
index f24d89e7de..725c33e53f 100644
--- a/block.c
+++ b/block.c
@@ -369,7 +369,7 @@ BlockDriver *bdrv_find_format(const char *format_name)
 return bdrv_do_find_format(format_name);
 }
 
-static int bdrv_is_whitelisted(BlockDriver *drv, bool read_only)
+int bdrv_is_whitelisted(BlockDriver *drv, bool read_only)
 {
 static const char *whitelist_rw[] = {
 CONFIG_BDRV_RW_WHITELIST
diff --git a/block/create.c b/block/create.c
new file mode 100644
index 00..e95446a0f3
--- /dev/null
+++ b/block/create.c
@@ -0,0 +1,75 @@
+/*
+ * Block layer code related to image creation
+ *
+ * Copyright (c) 2018 Kevin Wolf 
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to 
deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING 
FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+ * THE SOFTWARE.
+ */
+
+#include "qemu/osdep.h"
+#include "block/block_int.h"
+#include "qmp-commands.h"
+
+typedef struct BlockdevCreateCo {
+BlockDriver *drv;
+BlockdevCreateOptions *opts;
+int ret;
+Error **errp;
+} BlockdevCreateCo;
+
+static void coroutine_fn bdrv_co_create_co_entry(void *opaque)
+{
+BlockdevCreateCo *cco = opaque;
+cco->ret = cco->drv->bdrv_co_create(cco->opts, cco->errp);
+}
+
+void qmp_x_blockdev_create(BlockdevCreateOptions *options, Error **errp)
+{
+const char *fmt = BlockdevDriver_str(options->driver);
+BlockDriver *drv = bdrv_find_format(fmt);
+Coroutine *co;
+BlockdevCreateCo cco;
+
+/* If the driver is in the schema,