[Qemu-devel] [PATCH v2 0/5] block: Ensure non-protocol drivers can only be selected explicitly

[Qemu-devel] [PATCH v2 0/5] block: Ensure non-protocol drivers can only be selected explicitly

[Fabiano Rosas]

Block drivers can be selected by either protocol syntax:

  <protocol>:<filename>[:options]

or explicitly:

  driver=<driver>[,option=<opt1>...]

For the protocol syntax to work, drivers should set the protocol_name
field of the BlockDriver structure and provide bdrv_file_open and
bdrv_parse_filename implementations.

Conversely, block drivers that do not support the protocol syntax
should instead implement bdrv_open and not have a protocol_name field.

Some drivers do not currently adhere to this and errors arise when
trying to select them using the protocol syntax. For instance:

  $ qemu-img info replication:foo
  qemu-img: block.c:2401: bdrv_open_inherit: \
  Assertion `!!(flags & BDRV_O_PROTOCOL) == !!drv->bdrv_file_open' failed.
  Aborted (core dumped)

This patch-set ensures that the following drivers are meeting the
above criteria:
- blkreplay
- quorum
- replication
- throttle

Aside from that, documentation was added to make the above more
explicit.

v1 -> v2:
 - patch 1: updated commit message
 - patch 5: improved protocol_name documentation

 https://lists.nongnu.org/archive/html/qemu-devel/2018-03/msg02998.html

Fabiano Rosas (5):
  block/replication: Remove protocol_name field
  block/quorum: Remove protocol-related fields
  block/throttle: Remove protocol-related fields
  block/blkreplay: Remove protocol-related fields
  include/block/block_int: Document protocol related functions

 block/blkreplay.c         | 3 +--
 block/quorum.c            | 3 +--
 block/replication.c       | 1 -
 block/throttle.c          | 3 +--
 include/block/block_int.h | 8 ++++++++
 replication.h             | 1 -
 6 files changed, 11 insertions(+), 8 deletions(-)

--
2.13.6

[Qemu-devel] [PATCH v2 1/5] block/replication: Remove protocol_name field

[Fabiano Rosas]

The protocol_name field is used when selecting a driver via protocol
syntax (i.e. <protocol_name>:<filename:options:...>). Drivers that are
only selected explicitly (e.g. driver=replication,mode=primary,...)
should not have a protocol_name.

This patch removes the protocol_name field from the brdv_replication
structure so that attempts to invoke this driver using protocol syntax
will fail gracefully:

  $ qemu-img info replication:foo
  qemu-img: Could not open 'replication:': Unknown protocol 'replication'

Buglink: https://bugs.launchpad.net/qemu/+bug/1726733
Signed-off-by: Fabiano Rosas <farosas@linux.vnet.ibm.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
---
 block/replication.c | 1 -
 replication.h       | 1 -
 2 files changed, 2 deletions(-)

diff --git a/block/replication.c b/block/replication.c
index f98ef094b9..6c0c7186d9 100644
--- a/block/replication.c
+++ b/block/replication.c
@@ -703,7 +703,6 @@ static void replication_stop(ReplicationState *rs, bool failover, Error **errp)
 
 BlockDriver bdrv_replication = {
     .format_name                = "replication",
-    .protocol_name              = "replication",
     .instance_size              = sizeof(BDRVReplicationState),
 
     .bdrv_open                  = replication_open,
diff --git a/replication.h b/replication.h
index 8faefe005f..4c8354de23 100644
--- a/replication.h
+++ b/replication.h
@@ -67,7 +67,6 @@ typedef struct ReplicationState ReplicationState;
  *
  * BlockDriver bdrv_replication = {
  *     .format_name                = "replication",
- *     .protocol_name              = "replication",
  *     .instance_size              = sizeof(BDRVReplicationState),
  *
  *     .bdrv_open                  = replication_open,
-- 
2.13.6

[Qemu-devel] [PATCH v2 2/5] block/quorum: Remove protocol-related fields

[Fabiano Rosas]

The quorum driver is not a protocol so it should implement bdrv_open
instead of bdrv_file_open and not provide a protocol_name.

Attempts to invoke this driver using protocol syntax
(i.e. quorum:<filename:options:...>) will now fail gracefully:

  $ qemu-img info quorum:foo
  qemu-img: Could not open 'quorum:foo': Unknown protocol 'quorum'

Signed-off-by: Fabiano Rosas <farosas@linux.vnet.ibm.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
---
 block/quorum.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/block/quorum.c b/block/quorum.c
index 14333c18aa..cfe484a945 100644
--- a/block/quorum.c
+++ b/block/quorum.c
@@ -1098,11 +1098,10 @@ static void quorum_refresh_filename(BlockDriverState *bs, QDict *options)
 
 static BlockDriver bdrv_quorum = {
     .format_name                        = "quorum",
-    .protocol_name                      = "quorum",
 
     .instance_size                      = sizeof(BDRVQuorumState),
 
-    .bdrv_file_open                     = quorum_open,
+    .bdrv_open                          = quorum_open,
     .bdrv_close                         = quorum_close,
     .bdrv_refresh_filename              = quorum_refresh_filename,
 
-- 
2.13.6

Re: [Qemu-devel] [PATCH v2 2/5] block/quorum: Remove protocol-related fields

[Alberto Garcia]

On Mon 12 Mar 2018 11:07:50 PM CET, Fabiano Rosas wrote:
> The quorum driver is not a protocol so it should implement bdrv_open
> instead of bdrv_file_open and not provide a protocol_name.
>
> Attempts to invoke this driver using protocol syntax
> (i.e. quorum:<filename:options:...>) will now fail gracefully:
>
>   $ qemu-img info quorum:foo
>   qemu-img: Could not open 'quorum:foo': Unknown protocol 'quorum'
>
> Signed-off-by: Fabiano Rosas <farosas@linux.vnet.ibm.com>
> Reviewed-by: Max Reitz <mreitz@redhat.com>

Reviewed-by: Alberto Garcia <berto@igalia.com>

Berto

[Qemu-devel] [PATCH v2 3/5] block/throttle: Remove protocol-related fields

[Fabiano Rosas]

The throttle driver is not a protocol so it should implement bdrv_open
instead of bdrv_file_open and not provide a protocol_name.

Attempts to invoke this driver using protocol syntax
(i.e. throttle:<filename:options:...>) will now fail gracefully:

  $ qemu-img info throttle:foo
  qemu-img: Could not open 'throttle:foo': Unknown protocol 'throttle'

Signed-off-by: Fabiano Rosas <farosas@linux.vnet.ibm.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
---
 block/throttle.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/block/throttle.c b/block/throttle.c
index 5f4d43d0fc..95ed06acd8 100644
--- a/block/throttle.c
+++ b/block/throttle.c
@@ -215,10 +215,9 @@ static void coroutine_fn throttle_co_drain_end(BlockDriverState *bs)
 
 static BlockDriver bdrv_throttle = {
     .format_name                        =   "throttle",
-    .protocol_name                      =   "throttle",
     .instance_size                      =   sizeof(ThrottleGroupMember),
 
-    .bdrv_file_open                     =   throttle_open,
+    .bdrv_open                          =   throttle_open,
     .bdrv_close                         =   throttle_close,
     .bdrv_co_flush                      =   throttle_co_flush,
 
-- 
2.13.6

Re: [Qemu-devel] [Qemu-block] [PATCH v2 3/5] block/throttle: Remove protocol-related fields

[Alberto Garcia]

On Mon 12 Mar 2018 11:07:51 PM CET, Fabiano Rosas wrote:
> The throttle driver is not a protocol so it should implement bdrv_open
> instead of bdrv_file_open and not provide a protocol_name.
>
> Attempts to invoke this driver using protocol syntax
> (i.e. throttle:<filename:options:...>) will now fail gracefully:
>
>   $ qemu-img info throttle:foo
>   qemu-img: Could not open 'throttle:foo': Unknown protocol 'throttle'
>
> Signed-off-by: Fabiano Rosas <farosas@linux.vnet.ibm.com>
> Reviewed-by: Max Reitz <mreitz@redhat.com>

Reviewed-by: Alberto Garcia <berto@igalia.com>

Berto

[Qemu-devel] [PATCH v2 4/5] block/blkreplay: Remove protocol-related fields

[Fabiano Rosas]

The blkreplay driver is not a protocol so it should implement bdrv_open
instead of bdrv_file_open and not provide a protocol_name.

Attempts to invoke this driver using protocol syntax
(i.e. blkreplay:<filename:options:...>) will now fail gracefully:

  $ qemu-img info blkreplay:foo
  qemu-img: Could not open 'blkreplay:foo': Unknown protocol 'blkreplay'

Signed-off-by: Fabiano Rosas <farosas@linux.vnet.ibm.com>
Reviewed-by: Pavel Dovgalyuk <pavel.dovgaluk@ispras.ru>
Reviewed-by: Max Reitz <mreitz@redhat.com>
---
 block/blkreplay.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/block/blkreplay.c b/block/blkreplay.c
index 61e44a1949..fe5a9b4a98 100755
--- a/block/blkreplay.c
+++ b/block/blkreplay.c
@@ -129,10 +129,9 @@ static int coroutine_fn blkreplay_co_flush(BlockDriverState *bs)
 
 static BlockDriver bdrv_blkreplay = {
     .format_name            = "blkreplay",
-    .protocol_name          = "blkreplay",
     .instance_size          = 0,
 
-    .bdrv_file_open         = blkreplay_open,
+    .bdrv_open              = blkreplay_open,
     .bdrv_close             = blkreplay_close,
     .bdrv_child_perm        = bdrv_filter_default_perms,
     .bdrv_getlength         = blkreplay_getlength,
-- 
2.13.6

[Qemu-devel] [PATCH v2 5/5] include/block/block_int: Document protocol related functions

[Fabiano Rosas]

Clarify that:

- for protocols the brdv_file_open function is used instead
of bdrv_open;

- when protocol_name is set, a driver should expect
to be given only a filename and no other options.

Signed-off-by: Fabiano Rosas <farosas@linux.vnet.ibm.com>
---
 include/block/block_int.h | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/include/block/block_int.h b/include/block/block_int.h
index 27e17addba..c4dd1d4bb8 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -126,6 +126,8 @@ struct BlockDriver {
 
     int (*bdrv_open)(BlockDriverState *bs, QDict *options, int flags,
                      Error **errp);
+
+    /* Protocol drivers should implement this instead of bdrv_open */
     int (*bdrv_file_open)(BlockDriverState *bs, QDict *options, int flags,
                           Error **errp);
     void (*bdrv_close)(BlockDriverState *bs);
@@ -251,6 +253,12 @@ struct BlockDriver {
      */
     int coroutine_fn (*bdrv_co_flush_to_os)(BlockDriverState *bs);
 
+    /*
+     * Drivers setting this field must be able to work with just a plain
+     * filename with '<protocol_name>:' as a prefix, and no other options.
+     * Options may be extracted from the filename by implementing
+     * bdrv_parse_filename.
+     */
     const char *protocol_name;
     int (*bdrv_truncate)(BlockDriverState *bs, int64_t offset,
                          PreallocMode prealloc, Error **errp);
-- 
2.13.6

Re: [Qemu-devel] [PATCH v2 0/5] block: Ensure non-protocol drivers can only be selected explicitly

[Kevin Wolf]

Am 12.03.2018 um 23:07 hat Fabiano Rosas geschrieben:
> Block drivers can be selected by either protocol syntax:
> 
>   <protocol>:<filename>[:options]
> 
> or explicitly:
> 
>   driver=<driver>[,option=<opt1>...]
> 
> For the protocol syntax to work, drivers should set the protocol_name
> field of the BlockDriver structure and provide bdrv_file_open and
> bdrv_parse_filename implementations.
> 
> Conversely, block drivers that do not support the protocol syntax
> should instead implement bdrv_open and not have a protocol_name field.
> 
> Some drivers do not currently adhere to this and errors arise when
> trying to select them using the protocol syntax. For instance:
> 
>   $ qemu-img info replication:foo
>   qemu-img: block.c:2401: bdrv_open_inherit: \
>   Assertion `!!(flags & BDRV_O_PROTOCOL) == !!drv->bdrv_file_open' failed.
>   Aborted (core dumped)
> 
> This patch-set ensures that the following drivers are meeting the
> above criteria:
> - blkreplay
> - quorum
> - replication
> - throttle
> 
> Aside from that, documentation was added to make the above more
> explicit.

Thanks, applied to the block branch.

Kevin