[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

From: Eric Blake
Subject: Re: [Qemu-block] [PATCH 11/27] block: x-blockdev-create QMP command
Date: Wed, 21 Feb 2018 10:21:52 -0600
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.6.0

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

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

reply via email to

[Prev in Thread] Current Thread [Next in Thread]