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

[Max Reitz <mreitz@redhat.com>]

[-- 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 --]