[Qemu-devel] [PATCH] block/null: Remove 'filename' option

[Qemu-devel] [PATCH] block/null: Remove 'filename' option

[Kevin Wolf]

This option was only added to allow 'null-co://' and 'null-aio://' as
filenames, its value never served any actual purpose and was ignored.
Nevertheless it was accepted as '-drive driver=null,filename=foo'.

The correct way to enable the protocol prefixes (and that without adding
a useless -drive option) is implementing .bdrv_parse_filename. This is
what this patch does.

Technically, this is an incompatible change, but the null block driver
is only used for benchmarking, testing and debugging, and an option
without effect isn't likely to be used by anyone anyway, so no bad
effects are to be expected.

Reported-by: Markus Armbruster <armbru@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/null.c | 31 ++++++++++++++++++++++++++-----
 1 file changed, 26 insertions(+), 5 deletions(-)

diff --git a/block/null.c b/block/null.c
index 876f90965b..dd9c13f9ba 100644
--- a/block/null.c
+++ b/block/null.c
@@ -30,11 +30,6 @@ static QemuOptsList runtime_opts = {
     .head = QTAILQ_HEAD_INITIALIZER(runtime_opts.head),
     .desc = {
         {
-            .name = "filename",
-            .type = QEMU_OPT_STRING,
-            .help = "",
-        },
-        {
             .name = BLOCK_OPT_SIZE,
             .type = QEMU_OPT_SIZE,
             .help = "size of the null block",
@@ -54,6 +49,30 @@ static QemuOptsList runtime_opts = {
     },
 };
 
+static void null_co_parse_filename(const char *filename, QDict *options,
+                                   Error **errp)
+{
+    /* This functions only exists so that a null-co:// filename is accepted
+     * with the null-co driver. */
+    if (strcmp(filename, "null-co://")) {
+        error_setg(errp, "The only allowed filename for this driver is "
+                         "'null-co://'");
+        return;
+    }
+}
+
+static void null_aio_parse_filename(const char *filename, QDict *options,
+                                    Error **errp)
+{
+    /* This functions only exists so that a null-aio:// filename is accepted
+     * with the null-aio driver. */
+    if (strcmp(filename, "null-aio://")) {
+        error_setg(errp, "The only allowed filename for this driver is "
+                         "'null-aio://'");
+        return;
+    }
+}
+
 static int null_file_open(BlockDriverState *bs, QDict *options, int flags,
                           Error **errp)
 {
@@ -242,6 +261,7 @@ static BlockDriver bdrv_null_co = {
     .instance_size          = sizeof(BDRVNullState),
 
     .bdrv_file_open         = null_file_open,
+    .bdrv_parse_filename    = null_co_parse_filename,
     .bdrv_close             = null_close,
     .bdrv_getlength         = null_getlength,
 
@@ -261,6 +281,7 @@ static BlockDriver bdrv_null_aio = {
     .instance_size          = sizeof(BDRVNullState),
 
     .bdrv_file_open         = null_file_open,
+    .bdrv_parse_filename    = null_aio_parse_filename,
     .bdrv_close             = null_close,
     .bdrv_getlength         = null_getlength,
 
-- 
2.13.3

Re: [Qemu-devel] [Qemu-block] [PATCH] block/null: Remove 'filename' option

[Eric Blake]

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

On 08/04/2017 09:43 AM, Kevin Wolf wrote:
> This option was only added to allow 'null-co://' and 'null-aio://' as
> filenames, its value never served any actual purpose and was ignored.
> Nevertheless it was accepted as '-drive driver=null,filename=foo'.
> 
> The correct way to enable the protocol prefixes (and that without adding
> a useless -drive option) is implementing .bdrv_parse_filename. This is
> what this patch does.
> 
> Technically, this is an incompatible change, but the null block driver
> is only used for benchmarking, testing and debugging, and an option
> without effect isn't likely to be used by anyone anyway, so no bad
> effects are to be expected.

Agreed with the analysis. Still, better to get it into 2.10 rather than
going yet another release with the option available.

> 
> Reported-by: Markus Armbruster <armbru@redhat.com>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block/null.c | 31 ++++++++++++++++++++++++++-----
>  1 file changed, 26 insertions(+), 5 deletions(-)
> 

Reviewed-by: Eric Blake <eblake@redhat.com>

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


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

Re: [Qemu-devel] [Qemu-block] [PATCH] block/null: Remove 'filename' option

[Kevin Wolf]

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

Am 04.08.2017 um 16:56 hat Eric Blake geschrieben:
> On 08/04/2017 09:43 AM, Kevin Wolf wrote:
> > This option was only added to allow 'null-co://' and 'null-aio://' as
> > filenames, its value never served any actual purpose and was ignored.
> > Nevertheless it was accepted as '-drive driver=null,filename=foo'.
> > 
> > The correct way to enable the protocol prefixes (and that without adding
> > a useless -drive option) is implementing .bdrv_parse_filename. This is
> > what this patch does.
> > 
> > Technically, this is an incompatible change, but the null block driver
> > is only used for benchmarking, testing and debugging, and an option
> > without effect isn't likely to be used by anyone anyway, so no bad
> > effects are to be expected.
> 
> Agreed with the analysis. Still, better to get it into 2.10 rather than
> going yet another release with the option available.

Makes sense. Applied to the block branch.

Kevin

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [Qemu-devel] [Qemu-block] [PATCH] block/null: Remove 'filename' option

[Jeff Cody]

On Fri, Aug 04, 2017 at 04:43:54PM +0200, Kevin Wolf wrote:
> This option was only added to allow 'null-co://' and 'null-aio://' as
> filenames, its value never served any actual purpose and was ignored.
> Nevertheless it was accepted as '-drive driver=null,filename=foo'.
> 
> The correct way to enable the protocol prefixes (and that without adding
> a useless -drive option) is implementing .bdrv_parse_filename. This is
> what this patch does.
> 
> Technically, this is an incompatible change, but the null block driver
> is only used for benchmarking, testing and debugging, and an option
> without effect isn't likely to be used by anyone anyway, so no bad
> effects are to be expected.
> 
> Reported-by: Markus Armbruster <armbru@redhat.com>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block/null.c | 31 ++++++++++++++++++++++++++-----
>  1 file changed, 26 insertions(+), 5 deletions(-)
> 
> diff --git a/block/null.c b/block/null.c
> index 876f90965b..dd9c13f9ba 100644
> --- a/block/null.c
> +++ b/block/null.c
> @@ -30,11 +30,6 @@ static QemuOptsList runtime_opts = {
>      .head = QTAILQ_HEAD_INITIALIZER(runtime_opts.head),
>      .desc = {
>          {
> -            .name = "filename",
> -            .type = QEMU_OPT_STRING,
> -            .help = "",
> -        },
> -        {
>              .name = BLOCK_OPT_SIZE,
>              .type = QEMU_OPT_SIZE,
>              .help = "size of the null block",
> @@ -54,6 +49,30 @@ static QemuOptsList runtime_opts = {
>      },
>  };
>  
> +static void null_co_parse_filename(const char *filename, QDict *options,
> +                                   Error **errp)
> +{
> +    /* This functions only exists so that a null-co:// filename is accepted
> +     * with the null-co driver. */
> +    if (strcmp(filename, "null-co://")) {
> +        error_setg(errp, "The only allowed filename for this driver is "
> +                         "'null-co://'");
> +        return;
> +    }
> +}
> +
> +static void null_aio_parse_filename(const char *filename, QDict *options,
> +                                    Error **errp)
> +{
> +    /* This functions only exists so that a null-aio:// filename is accepted
> +     * with the null-aio driver. */
> +    if (strcmp(filename, "null-aio://")) {
> +        error_setg(errp, "The only allowed filename for this driver is "
> +                         "'null-aio://'");
> +        return;
> +    }
> +}
> +
>  static int null_file_open(BlockDriverState *bs, QDict *options, int flags,
>                            Error **errp)
>  {
> @@ -242,6 +261,7 @@ static BlockDriver bdrv_null_co = {
>      .instance_size          = sizeof(BDRVNullState),
>  
>      .bdrv_file_open         = null_file_open,
> +    .bdrv_parse_filename    = null_co_parse_filename,
>      .bdrv_close             = null_close,
>      .bdrv_getlength         = null_getlength,
>  
> @@ -261,6 +281,7 @@ static BlockDriver bdrv_null_aio = {
>      .instance_size          = sizeof(BDRVNullState),
>  
>      .bdrv_file_open         = null_file_open,
> +    .bdrv_parse_filename    = null_aio_parse_filename,
>      .bdrv_close             = null_close,
>      .bdrv_getlength         = null_getlength,
>  
> -- 
> 2.13.3
> 
> 

Reviewed-by: Jeff Cody <jcody@redhat.com>

Re: [Qemu-devel] [Qemu-block] [PATCH] block/null: Remove 'filename' option

[Stefan Hajnoczi]

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

On Fri, Aug 04, 2017 at 04:43:54PM +0200, Kevin Wolf wrote:
> This option was only added to allow 'null-co://' and 'null-aio://' as
> filenames, its value never served any actual purpose and was ignored.
> Nevertheless it was accepted as '-drive driver=null,filename=foo'.
> 
> The correct way to enable the protocol prefixes (and that without adding
> a useless -drive option) is implementing .bdrv_parse_filename. This is
> what this patch does.
> 
> Technically, this is an incompatible change, but the null block driver
> is only used for benchmarking, testing and debugging, and an option
> without effect isn't likely to be used by anyone anyway, so no bad
> effects are to be expected.
> 
> Reported-by: Markus Armbruster <armbru@redhat.com>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block/null.c | 31 ++++++++++++++++++++++++++-----
>  1 file changed, 26 insertions(+), 5 deletions(-)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]