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

[Qemu-devel] [PATCH 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.


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 | 6 ++++++
 replication.h             | 1 -
 6 files changed, 9 insertions(+), 8 deletions(-)

--
2.13.6

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

[Fabiano Rosas]

The replication driver is only selected explicitly (via
driver=replication,mode=primary,...) so it is not a protocol driver.

This patch removes the protocol_name field from the brdv_replication
structure so that attempts to invoke this driver using protocol
syntax (i.e. replication:<filename:options:...>) 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>
---
 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

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

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 2450 bytes --]

On 2018-03-09 19:21, Fabiano Rosas wrote:
> The replication driver is only selected explicitly (via
> driver=replication,mode=primary,...) so it is not a protocol driver.

That's not really the point.  It isn't a protocol driver because it has
a "file" child (opened in replication_open).  All in all it's a filter
driver.

Apart from that, having a protocol_name might still be OK even for
non-protocol drivers; for instance, blkdebug and blkverify are both
filter drivers, but since they implement bdrv_parse_filename() it's OK
for them to specify protocol_name.

So your sentence is half correct.  Because it can only be selected
explicitly (for several reasons), giving a protocol_name doesn't make
sense.  (Independently of whether it is a protocol driver or not.)

But anyway:

Reviewed-by: Max Reitz <mreitz@redhat.com>

> This patch removes the protocol_name field from the brdv_replication
> structure so that attempts to invoke this driver using protocol
> syntax (i.e. replication:<filename:options:...>) 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>
> ---
>  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,
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 512 bytes --]

[Qemu-devel] [PATCH 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>
---
 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 2/5] block/quorum: Remove protocol-related fields

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 607 bytes --]

On 2018-03-09 19:21, 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>
> ---
>  block/quorum.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)

Reviewed-by: Max Reitz <mreitz@redhat.com>


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 512 bytes --]

[Qemu-devel] [PATCH 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>
---
 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] [PATCH 3/5] block/throttle: Remove protocol-related fields

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 619 bytes --]

On 2018-03-09 19:21, 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>
> ---
>  block/throttle.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)

Reviewed-by: Max Reitz <mreitz@redhat.com>


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 512 bytes --]

[Qemu-devel] [PATCH 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>
---
 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

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

[Pavel Dovgalyuk]

> From: Fabiano Rosas [mailto:farosas@linux.vnet.ibm.com]
> 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>
> ---
>  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

Reviewed-off-by: Pavel Dovgalyuk <pavel.dovgaluk@ispras.ru>

Pavel Dovgalyuk

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

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 625 bytes --]

On 2018-03-09 19:22, Fabiano Rosas wrote:
> 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>
> ---
>  block/blkreplay.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)

Reviewed-by: Max Reitz <mreitz@redhat.com>


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 512 bytes --]

[Qemu-devel] [PATCH 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 and that protocol_name is expected to be set.

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

diff --git a/include/block/block_int.h b/include/block/block_int.h
index 64a5700f2b..d5e864c2dc 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);
@@ -247,6 +249,10 @@ struct BlockDriver {
      */
     int coroutine_fn (*bdrv_co_flush_to_os)(BlockDriverState *bs);
 
+    /*
+     * Drivers that set this field should also provide a
+     * bdrv_file_open implementation
+     */
     const char *protocol_name;
     int (*bdrv_truncate)(BlockDriverState *bs, int64_t offset,
                          PreallocMode prealloc, Error **errp);
-- 
2.13.6

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

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 2265 bytes --]

On 2018-03-09 19:22, Fabiano Rosas wrote:
> Clarify that for protocols the brdv_file_open function is used instead
> of bdrv_open and that protocol_name is expected to be set.
> 
> Signed-off-by: Fabiano Rosas <farosas@linux.vnet.ibm.com>
> ---
>  include/block/block_int.h | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/include/block/block_int.h b/include/block/block_int.h
> index 64a5700f2b..d5e864c2dc 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);
> @@ -247,6 +249,10 @@ struct BlockDriver {
>       */
>      int coroutine_fn (*bdrv_co_flush_to_os)(BlockDriverState *bs);
>  
> +    /*
> +     * Drivers that set this field should also provide a
> +     * bdrv_file_open implementation

Well...  In a sense.

Drivers that implement this should be expected to be given only a
filename and no other options.  Therefore, they should generally
implement .bdrv_parse_filename() so they can extract options from it
(exception: gluster, which parses the whole filename in its
.bdrv_file_open() implementation).

(Or maybe they can just accept a URL and don't need to extract options
from it, like curl or file.)

So the stress lies on "Drivers setting this field must be able to work
with just a plain filename with this prefix, and no other options."
(Note that all of the drivers you removed this field from in this series
violated this.  They expect some other options and cannot work without.)

A driver doesn't need to be a protocol driver for this, and technically
a protocol driver doesn't need to set this.  Maybe we should rename it
to "filename_prefix"...?

Max

> +     */
>      const char *protocol_name;
>      int (*bdrv_truncate)(BlockDriverState *bs, int64_t offset,
>                           PreallocMode prealloc, Error **errp);
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 512 bytes --]

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

[Fabiano Rosas]

On 2018-03-12 10:50, Max Reitz wrote:

> A driver doesn't need to be a protocol driver for this, and technically
> a protocol driver doesn't need to set this.  Maybe we should rename it
> to "filename_prefix"...?

Yes, something that is closer to the filename string and farther from
the notion of what defines a protocol would be good. I see "protocol
prefix" mentioned in block.c, maybe that would be a good middle
ground.

Cheers