[Qemu-devel] [PATCH 0/9] bdrv_open() cleanups, part 1

[Qemu-devel] [PATCH 0/9] bdrv_open() cleanups, part 1

[Kevin Wolf]

This is the first part of an attempt for disentangling bdrv_open(). At the end
of this series, bdrv_open() code is somewhat easier to read, but the real
changes (including some bug fixes and changes of behaviour) haven't happened
yet.

Just sending out the first part now to get this merged early and avoid
conflicts.

Kevin Wolf (9):
  block: Create bdrv_fill_options()
  block: Move bdrv_fill_options() call to bdrv_open()
  block: Move json: parsing to bdrv_fill_options()
  block: Always pass driver name through options QDict
  block: Use common driver selection code for bdrv_open_file()
  block: Inline bdrv_file_open()
  block: Remove second bdrv_open() recursion
  block: Catch backing files assigned to non-COW drivers
  block: Remove a special case for protocols

 block.c                    | 283 ++++++++++++++++++++++-----------------------
 block/cow.c                |   1 +
 block/qcow.c               |   1 +
 block/qcow2.c              |   1 +
 block/qed.c                |   1 +
 block/vmdk.c               |   1 +
 include/block/block_int.h  |   3 +
 tests/qemu-iotests/051     |   6 +
 tests/qemu-iotests/051.out |  14 ++-
 9 files changed, 166 insertions(+), 145 deletions(-)

-- 
1.8.3.1

[Qemu-devel] [PATCH 1/9] block: Create bdrv_fill_options()

[Kevin Wolf]

The idea of bdrv_fill_options() is to convert every parameter for
opening images, in particular the filename and flags, to entries in the
options QDict.

This patch starts with moving the filename parsing and driver probing
part from bdrv_file_open() to the new function.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block.c | 116 ++++++++++++++++++++++++++++++++++++++++------------------------
 1 file changed, 73 insertions(+), 43 deletions(-)

diff --git a/block.c b/block.c
index 17f763d..c5707e8 100644
--- a/block.c
+++ b/block.c
@@ -1007,77 +1007,107 @@ free_and_fail:
 }
 
 /*
- * Opens a file using a protocol (file, host_device, nbd, ...)
- *
- * options is an indirect pointer to a QDict of options to pass to the block
- * drivers, or pointer to NULL for an empty set of options. If this function
- * takes ownership of the QDict reference, it will set *options to NULL;
- * otherwise, it will contain unused/unrecognized options after this function
- * returns. Then, the caller is responsible for freeing it. If it intends to
- * reuse the QDict, QINCREF() should be called beforehand.
+ * Fills in default options for opening images and converts the legacy
+ * filename/flags pair to option QDict entries.
  */
-static int bdrv_file_open(BlockDriverState *bs, const char *filename,
-                          QDict **options, int flags, Error **errp)
+static int bdrv_fill_options(QDict **options, const char *filename,
+                             Error **errp)
 {
-    BlockDriver *drv;
     const char *drvname;
     bool parse_filename = false;
     Error *local_err = NULL;
-    int ret;
+    BlockDriver *drv;
 
     /* Fetch the file name from the options QDict if necessary */
-    if (!filename) {
-        filename = qdict_get_try_str(*options, "filename");
-    } else if (filename && !qdict_haskey(*options, "filename")) {
-        qdict_put(*options, "filename", qstring_from_str(filename));
-        parse_filename = true;
-    } else {
-        error_setg(errp, "Can't specify 'file' and 'filename' options at the "
-                   "same time");
-        ret = -EINVAL;
-        goto fail;
+    if (filename) {
+        if (filename && !qdict_haskey(*options, "filename")) {
+            qdict_put(*options, "filename", qstring_from_str(filename));
+            parse_filename = true;
+        } else {
+            error_setg(errp, "Can't specify 'file' and 'filename' options at "
+                             "the same time");
+            return -EINVAL;
+        }
     }
 
     /* Find the right block driver */
+    filename = qdict_get_try_str(*options, "filename");
     drvname = qdict_get_try_str(*options, "driver");
-    if (drvname) {
-        drv = bdrv_find_format(drvname);
-        if (!drv) {
-            error_setg(errp, "Unknown driver '%s'", drvname);
-        }
-        qdict_del(*options, "driver");
-    } else if (filename) {
-        drv = bdrv_find_protocol(filename, parse_filename);
-        if (!drv) {
-            error_setg(errp, "Unknown protocol");
+
+    if (!drvname) {
+        if (filename) {
+            drv = bdrv_find_protocol(filename, parse_filename);
+            if (!drv) {
+                error_setg(errp, "Unknown protocol");
+                return -EINVAL;
+            }
+
+            drvname = drv->format_name;
+            qdict_put(*options, "driver", qstring_from_str(drvname));
+        } else {
+            error_setg(errp, "Must specify either driver or file");
+            return -EINVAL;
         }
-    } else {
-        error_setg(errp, "Must specify either driver or file");
-        drv = NULL;
     }
 
+    drv = bdrv_find_format(drvname);
     if (!drv) {
-        /* errp has been set already */
-        ret = -ENOENT;
-        goto fail;
+        error_setg(errp, "Unknown driver '%s'", drvname);
+        return -ENOENT;
     }
 
-    /* Parse the filename and open it */
+    /* Driver-specific filename parsing */
     if (drv->bdrv_parse_filename && parse_filename) {
         drv->bdrv_parse_filename(filename, *options, &local_err);
         if (local_err) {
             error_propagate(errp, local_err);
-            ret = -EINVAL;
-            goto fail;
+            return -EINVAL;
         }
 
         if (!drv->bdrv_needs_filename) {
             qdict_del(*options, "filename");
-        } else {
-            filename = qdict_get_str(*options, "filename");
         }
     }
 
+    return 0;
+}
+
+/*
+ * Opens a file using a protocol (file, host_device, nbd, ...)
+ *
+ * options is an indirect pointer to a QDict of options to pass to the block
+ * drivers, or pointer to NULL for an empty set of options. If this function
+ * takes ownership of the QDict reference, it will set *options to NULL;
+ * otherwise, it will contain unused/unrecognized options after this function
+ * returns. Then, the caller is responsible for freeing it. If it intends to
+ * reuse the QDict, QINCREF() should be called beforehand.
+ */
+static int bdrv_file_open(BlockDriverState *bs, const char *filename,
+                          QDict **options, int flags, Error **errp)
+{
+    BlockDriver *drv;
+    const char *drvname;
+    Error *local_err = NULL;
+    int ret;
+
+    ret = bdrv_fill_options(options, filename, &local_err);
+    if (local_err) {
+        error_propagate(errp, local_err);
+        return ret;
+    }
+
+    filename = qdict_get_try_str(*options, "filename");
+    drvname = qdict_get_str(*options, "driver");
+
+    drv = bdrv_find_format(drvname);
+    if (!drv) {
+        error_setg(errp, "Unknown driver '%s'", drvname);
+        ret = -ENOENT;
+        goto fail;
+    }
+    qdict_del(*options, "driver");
+
+    /* Open the file */
     if (!drv->bdrv_file_open) {
         ret = bdrv_open(&bs, filename, NULL, *options, flags, drv, &local_err);
         *options = NULL;
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH 1/9] block: Create bdrv_fill_options()

[Eric Blake]

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

On 06/11/2014 08:04 AM, Kevin Wolf wrote:
> The idea of bdrv_fill_options() is to convert every parameter for
> opening images, in particular the filename and flags, to entries in the
> options QDict.
> 
> This patch starts with moving the filename parsing and driver probing
> part from bdrv_file_open() to the new function.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block.c | 116 ++++++++++++++++++++++++++++++++++++++++------------------------
>  1 file changed, 73 insertions(+), 43 deletions(-)
> 
> + * Fills in default options for opening images and converts the legacy
> + * filename/flags pair to option QDict entries.
>   */
> -static int bdrv_file_open(BlockDriverState *bs, const char *filename,
> -                          QDict **options, int flags, Error **errp)
> +static int bdrv_fill_options(QDict **options, const char *filename,
> +                             Error **errp)

The comment mentions filename/flags, but I only see filename as a
parameter.  Is this something changed later in the series?

>  {
> -    BlockDriver *drv;
>      const char *drvname;
>      bool parse_filename = false;
>      Error *local_err = NULL;
> -    int ret;
> +    BlockDriver *drv;
>  
>      /* Fetch the file name from the options QDict if necessary */
> -    if (!filename) {
> -        filename = qdict_get_try_str(*options, "filename");
> -    } else if (filename && !qdict_haskey(*options, "filename")) {
> -        qdict_put(*options, "filename", qstring_from_str(filename));
> -        parse_filename = true;
> -    } else {
> -        error_setg(errp, "Can't specify 'file' and 'filename' options at the "
> -                   "same time");
> -        ret = -EINVAL;
> -        goto fail;
> +    if (filename) {
> +        if (filename && !qdict_haskey(*options, "filename")) {

The 'filename &&' is redundant.

> +            qdict_put(*options, "filename", qstring_from_str(filename));
> +            parse_filename = true;
> +        } else {
> +            error_setg(errp, "Can't specify 'file' and 'filename' options at "
> +                             "the same time");
> +            return -EINVAL;
> +        }
>      }
>  
>      /* Find the right block driver */
> +    filename = qdict_get_try_str(*options, "filename");
>      drvname = qdict_get_try_str(*options, "driver");
> -    if (drvname) {
> -        drv = bdrv_find_format(drvname);
> -        if (!drv) {
> -            error_setg(errp, "Unknown driver '%s'", drvname);
> -        }
> -        qdict_del(*options, "driver");
> -    } else if (filename) {
> -        drv = bdrv_find_protocol(filename, parse_filename);
> -        if (!drv) {
> -            error_setg(errp, "Unknown protocol");
> +
> +    if (!drvname) {
> +        if (filename) {
> +            drv = bdrv_find_protocol(filename, parse_filename);

This assigns drv...

> +            if (!drv) {
> +                error_setg(errp, "Unknown protocol");
> +                return -EINVAL;
> +            }
> +
> +            drvname = drv->format_name;
> +            qdict_put(*options, "driver", qstring_from_str(drvname));
> +        } else {
> +            error_setg(errp, "Must specify either driver or file");
> +            return -EINVAL;
>          }
> -    } else {
> -        error_setg(errp, "Must specify either driver or file");
> -        drv = NULL;
>      }
>  
> +    drv = bdrv_find_format(drvname);

...and this does it again.  Should this second assignment be inside an
else{}?


> +static int bdrv_file_open(BlockDriverState *bs, const char *filename,
> +                          QDict **options, int flags, Error **errp)

> +
> +    drv = bdrv_find_format(drvname);
> +    if (!drv) {
> +        error_setg(errp, "Unknown driver '%s'", drvname);
> +        ret = -ENOENT;
> +        goto fail;
> +    }

Isn't this error check redundant with the one already done in
bdrv_fill_options()?  You could probably just use assert(drv)


-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


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

Re: [Qemu-devel] [PATCH 1/9] block: Create bdrv_fill_options()

[Benoît Canet]

The Wednesday 11 Jun 2014 à 16:04:55 (+0200), Kevin Wolf wrote :
> The idea of bdrv_fill_options() is to convert every parameter for
> opening images, in particular the filename and flags, to entries in the
> options QDict.
> 
> This patch starts with moving the filename parsing and driver probing
> part from bdrv_file_open() to the new function.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block.c | 116 ++++++++++++++++++++++++++++++++++++++++------------------------
>  1 file changed, 73 insertions(+), 43 deletions(-)
> 
> diff --git a/block.c b/block.c
> index 17f763d..c5707e8 100644
> --- a/block.c
> +++ b/block.c
> @@ -1007,77 +1007,107 @@ free_and_fail:
>  }
>  
>  /*
> - * Opens a file using a protocol (file, host_device, nbd, ...)
> - *
> - * options is an indirect pointer to a QDict of options to pass to the block
> - * drivers, or pointer to NULL for an empty set of options. If this function
> - * takes ownership of the QDict reference, it will set *options to NULL;
> - * otherwise, it will contain unused/unrecognized options after this function
> - * returns. Then, the caller is responsible for freeing it. If it intends to
> - * reuse the QDict, QINCREF() should be called beforehand.
> + * Fills in default options for opening images and converts the legacy
> + * filename/flags pair to option QDict entries.
>   */
> -static int bdrv_file_open(BlockDriverState *bs, const char *filename,
> -                          QDict **options, int flags, Error **errp)
> +static int bdrv_fill_options(QDict **options, const char *filename,
> +                             Error **errp)
>  {
> -    BlockDriver *drv;
>      const char *drvname;
>      bool parse_filename = false;
>      Error *local_err = NULL;
> -    int ret;
> +    BlockDriver *drv;
>  
>      /* Fetch the file name from the options QDict if necessary */
> -    if (!filename) {
> -        filename = qdict_get_try_str(*options, "filename");
> -    } else if (filename && !qdict_haskey(*options, "filename")) {
> -        qdict_put(*options, "filename", qstring_from_str(filename));
> -        parse_filename = true;
> -    } else {
> -        error_setg(errp, "Can't specify 'file' and 'filename' options at the "
> -                   "same time");
> -        ret = -EINVAL;
> -        goto fail;
> +    if (filename) {
> +        if (filename && !qdict_haskey(*options, "filename")) {

The inner filename testing is redundant.

> +            qdict_put(*options, "filename", qstring_from_str(filename));
> +            parse_filename = true;
> +        } else {
> +            error_setg(errp, "Can't specify 'file' and 'filename' options at "
> +                             "the same time");
> +            return -EINVAL;
> +        }
>      }
>  
>      /* Find the right block driver */
> +    filename = qdict_get_try_str(*options, "filename");
>      drvname = qdict_get_try_str(*options, "driver");
> -    if (drvname) {
> -        drv = bdrv_find_format(drvname);
> -        if (!drv) {
> -            error_setg(errp, "Unknown driver '%s'", drvname);
> -        }
> -        qdict_del(*options, "driver");
> -    } else if (filename) {
> -        drv = bdrv_find_protocol(filename, parse_filename);
> -        if (!drv) {
> -            error_setg(errp, "Unknown protocol");
> +
> +    if (!drvname) {
> +        if (filename) {
> +            drv = bdrv_find_protocol(filename, parse_filename);
> +            if (!drv) {
> +                error_setg(errp, "Unknown protocol");
> +                return -EINVAL;
> +            }
> +
> +            drvname = drv->format_name;
> +            qdict_put(*options, "driver", qstring_from_str(drvname));
> +        } else {
> +            error_setg(errp, "Must specify either driver or file");

Isn't it "Must specify either driver or _filename_ ?

> +            return -EINVAL;
>          }
> -    } else {
> -        error_setg(errp, "Must specify either driver or file");
> -        drv = NULL;
>      }
>  
> +    drv = bdrv_find_format(drvname);
>      if (!drv) {
> -        /* errp has been set already */
> -        ret = -ENOENT;
> -        goto fail;
> +        error_setg(errp, "Unknown driver '%s'", drvname);
> +        return -ENOENT;
>      }
>  
> -    /* Parse the filename and open it */
> +    /* Driver-specific filename parsing */
>      if (drv->bdrv_parse_filename && parse_filename) {
>          drv->bdrv_parse_filename(filename, *options, &local_err);
>          if (local_err) {
>              error_propagate(errp, local_err);
> -            ret = -EINVAL;
> -            goto fail;
> +            return -EINVAL;
>          }
>  
>          if (!drv->bdrv_needs_filename) {
>              qdict_del(*options, "filename");
> -        } else {
> -            filename = qdict_get_str(*options, "filename");
>          }
>      }
>  
> +    return 0;
> +}
> +
> +/*
> + * Opens a file using a protocol (file, host_device, nbd, ...)
> + *
> + * options is an indirect pointer to a QDict of options to pass to the block
> + * drivers, or pointer to NULL for an empty set of options. If this function
> + * takes ownership of the QDict reference, it will set *options to NULL;
> + * otherwise, it will contain unused/unrecognized options after this function
> + * returns. Then, the caller is responsible for freeing it. If it intends to
> + * reuse the QDict, QINCREF() should be called beforehand.
> + */
> +static int bdrv_file_open(BlockDriverState *bs, const char *filename,
> +                          QDict **options, int flags, Error **errp)
> +{
> +    BlockDriver *drv;
> +    const char *drvname;
> +    Error *local_err = NULL;
> +    int ret;
> +
> +    ret = bdrv_fill_options(options, filename, &local_err);
> +    if (local_err) {
> +        error_propagate(errp, local_err);
> +        return ret;
Why not goto fail for consistency with the rest of the function ?

> +    }
> +
> +    filename = qdict_get_try_str(*options, "filename");
> +    drvname = qdict_get_str(*options, "driver");
> +
> +    drv = bdrv_find_format(drvname);
> +    if (!drv) {
> +        error_setg(errp, "Unknown driver '%s'", drvname);
> +        ret = -ENOENT;
> +        goto fail;
> +    }
> +    qdict_del(*options, "driver");
> +
> +    /* Open the file */
>      if (!drv->bdrv_file_open) {

Don't we need:
           qdict_del(*options, "filename");
Here so bdrv_open don't see an extra filename option ?

>          ret = bdrv_open(&bs, filename, NULL, *options, flags, drv, &local_err);
>          *options = NULL;
> -- 
> 1.8.3.1
> 
> 

Re: [Qemu-devel] [PATCH 1/9] block: Create bdrv_fill_options()

[Kevin Wolf]

Am 12.06.2014 um 14:08 hat Benoît Canet geschrieben:
> The Wednesday 11 Jun 2014 à 16:04:55 (+0200), Kevin Wolf wrote :
> > The idea of bdrv_fill_options() is to convert every parameter for
> > opening images, in particular the filename and flags, to entries in the
> > options QDict.
> > 
> > This patch starts with moving the filename parsing and driver probing
> > part from bdrv_file_open() to the new function.
> > 
> > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> > ---
> >  block.c | 116 ++++++++++++++++++++++++++++++++++++++++------------------------
> >  1 file changed, 73 insertions(+), 43 deletions(-)
> > 
> > diff --git a/block.c b/block.c
> > index 17f763d..c5707e8 100644
> > --- a/block.c
> > +++ b/block.c
> > @@ -1007,77 +1007,107 @@ free_and_fail:
> >  }
> >  
> >  /*
> > - * Opens a file using a protocol (file, host_device, nbd, ...)
> > - *
> > - * options is an indirect pointer to a QDict of options to pass to the block
> > - * drivers, or pointer to NULL for an empty set of options. If this function
> > - * takes ownership of the QDict reference, it will set *options to NULL;
> > - * otherwise, it will contain unused/unrecognized options after this function
> > - * returns. Then, the caller is responsible for freeing it. If it intends to
> > - * reuse the QDict, QINCREF() should be called beforehand.
> > + * Fills in default options for opening images and converts the legacy
> > + * filename/flags pair to option QDict entries.
> >   */
> > -static int bdrv_file_open(BlockDriverState *bs, const char *filename,
> > -                          QDict **options, int flags, Error **errp)
> > +static int bdrv_fill_options(QDict **options, const char *filename,
> > +                             Error **errp)
> >  {
> > -    BlockDriver *drv;
> >      const char *drvname;
> >      bool parse_filename = false;
> >      Error *local_err = NULL;
> > -    int ret;
> > +    BlockDriver *drv;
> >  
> >      /* Fetch the file name from the options QDict if necessary */
> > -    if (!filename) {
> > -        filename = qdict_get_try_str(*options, "filename");
> > -    } else if (filename && !qdict_haskey(*options, "filename")) {
> > -        qdict_put(*options, "filename", qstring_from_str(filename));
> > -        parse_filename = true;
> > -    } else {
> > -        error_setg(errp, "Can't specify 'file' and 'filename' options at the "
> > -                   "same time");
> > -        ret = -EINVAL;
> > -        goto fail;
> > +    if (filename) {
> > +        if (filename && !qdict_haskey(*options, "filename")) {
> 
> The inner filename testing is redundant.

Will fix.

> > +            qdict_put(*options, "filename", qstring_from_str(filename));
> > +            parse_filename = true;
> > +        } else {
> > +            error_setg(errp, "Can't specify 'file' and 'filename' options at "
> > +                             "the same time");
> > +            return -EINVAL;
> > +        }
> >      }
> >  
> >      /* Find the right block driver */
> > +    filename = qdict_get_try_str(*options, "filename");
> >      drvname = qdict_get_try_str(*options, "driver");
> > -    if (drvname) {
> > -        drv = bdrv_find_format(drvname);
> > -        if (!drv) {
> > -            error_setg(errp, "Unknown driver '%s'", drvname);
> > -        }
> > -        qdict_del(*options, "driver");
> > -    } else if (filename) {
> > -        drv = bdrv_find_protocol(filename, parse_filename);
> > -        if (!drv) {
> > -            error_setg(errp, "Unknown protocol");
> > +
> > +    if (!drvname) {
> > +        if (filename) {
> > +            drv = bdrv_find_protocol(filename, parse_filename);
> > +            if (!drv) {
> > +                error_setg(errp, "Unknown protocol");
> > +                return -EINVAL;
> > +            }
> > +
> > +            drvname = drv->format_name;
> > +            qdict_put(*options, "driver", qstring_from_str(drvname));
> > +        } else {
> > +            error_setg(errp, "Must specify either driver or file");
> 
> Isn't it "Must specify either driver or _filename_ ?

This is only code motion, but changing it in a separate patch would make
sense, I guess.

> > +            return -EINVAL;
> >          }
> > -    } else {
> > -        error_setg(errp, "Must specify either driver or file");
> > -        drv = NULL;
> >      }
> >  
> > +    drv = bdrv_find_format(drvname);
> >      if (!drv) {
> > -        /* errp has been set already */
> > -        ret = -ENOENT;
> > -        goto fail;
> > +        error_setg(errp, "Unknown driver '%s'", drvname);
> > +        return -ENOENT;
> >      }
> >  
> > -    /* Parse the filename and open it */
> > +    /* Driver-specific filename parsing */
> >      if (drv->bdrv_parse_filename && parse_filename) {
> >          drv->bdrv_parse_filename(filename, *options, &local_err);
> >          if (local_err) {
> >              error_propagate(errp, local_err);
> > -            ret = -EINVAL;
> > -            goto fail;
> > +            return -EINVAL;
> >          }
> >  
> >          if (!drv->bdrv_needs_filename) {
> >              qdict_del(*options, "filename");
> > -        } else {
> > -            filename = qdict_get_str(*options, "filename");
> >          }
> >      }
> >  
> > +    return 0;
> > +}
> > +
> > +/*
> > + * Opens a file using a protocol (file, host_device, nbd, ...)
> > + *
> > + * options is an indirect pointer to a QDict of options to pass to the block
> > + * drivers, or pointer to NULL for an empty set of options. If this function
> > + * takes ownership of the QDict reference, it will set *options to NULL;
> > + * otherwise, it will contain unused/unrecognized options after this function
> > + * returns. Then, the caller is responsible for freeing it. If it intends to
> > + * reuse the QDict, QINCREF() should be called beforehand.
> > + */
> > +static int bdrv_file_open(BlockDriverState *bs, const char *filename,
> > +                          QDict **options, int flags, Error **errp)
> > +{
> > +    BlockDriver *drv;
> > +    const char *drvname;
> > +    Error *local_err = NULL;
> > +    int ret;
> > +
> > +    ret = bdrv_fill_options(options, filename, &local_err);
> > +    if (local_err) {
> > +        error_propagate(errp, local_err);
> > +        return ret;
> Why not goto fail for consistency with the rest of the function ?

Okay. Rather pointless and the function will be gone at the end of the
series, but more consistent indeed.

> > +    }
> > +
> > +    filename = qdict_get_try_str(*options, "filename");
> > +    drvname = qdict_get_str(*options, "driver");
> > +
> > +    drv = bdrv_find_format(drvname);
> > +    if (!drv) {
> > +        error_setg(errp, "Unknown driver '%s'", drvname);
> > +        ret = -ENOENT;
> > +        goto fail;
> > +    }
> > +    qdict_del(*options, "driver");
> > +
> > +    /* Open the file */
> >      if (!drv->bdrv_file_open) {
> 
> Don't we need:
>            qdict_del(*options, "filename");
> Here so bdrv_open don't see an extra filename option ?

I think it doesn't matter because bdrv_open() would put it back again.
Do you have an example where it makes a difference?

> >          ret = bdrv_open(&bs, filename, NULL, *options, flags, drv, &local_err);
> >          *options = NULL;

Kevin

Re: [Qemu-devel] [PATCH 1/9] block: Create bdrv_fill_options()

[Benoît Canet]

The Monday 23 Jun 2014 à 17:30:45 (+0200), Kevin Wolf wrote :
> Am 12.06.2014 um 14:08 hat Benoît Canet geschrieben:
> > The Wednesday 11 Jun 2014 à 16:04:55 (+0200), Kevin Wolf wrote :
> > > The idea of bdrv_fill_options() is to convert every parameter for
> > > opening images, in particular the filename and flags, to entries in the
> > > options QDict.
> > > 
> > > This patch starts with moving the filename parsing and driver probing
> > > part from bdrv_file_open() to the new function.
> > > 
> > > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> > > ---
> > >  block.c | 116 ++++++++++++++++++++++++++++++++++++++++------------------------
> > >  1 file changed, 73 insertions(+), 43 deletions(-)
> > > 
> > > diff --git a/block.c b/block.c
> > > index 17f763d..c5707e8 100644
> > > --- a/block.c
> > > +++ b/block.c
> > > @@ -1007,77 +1007,107 @@ free_and_fail:
> > >  }
> > >  
> > >  /*
> > > - * Opens a file using a protocol (file, host_device, nbd, ...)
> > > - *
> > > - * options is an indirect pointer to a QDict of options to pass to the block
> > > - * drivers, or pointer to NULL for an empty set of options. If this function
> > > - * takes ownership of the QDict reference, it will set *options to NULL;
> > > - * otherwise, it will contain unused/unrecognized options after this function
> > > - * returns. Then, the caller is responsible for freeing it. If it intends to
> > > - * reuse the QDict, QINCREF() should be called beforehand.
> > > + * Fills in default options for opening images and converts the legacy
> > > + * filename/flags pair to option QDict entries.
> > >   */
> > > -static int bdrv_file_open(BlockDriverState *bs, const char *filename,
> > > -                          QDict **options, int flags, Error **errp)
> > > +static int bdrv_fill_options(QDict **options, const char *filename,
> > > +                             Error **errp)
> > >  {
> > > -    BlockDriver *drv;
> > >      const char *drvname;
> > >      bool parse_filename = false;
> > >      Error *local_err = NULL;
> > > -    int ret;
> > > +    BlockDriver *drv;
> > >  
> > >      /* Fetch the file name from the options QDict if necessary */
> > > -    if (!filename) {
> > > -        filename = qdict_get_try_str(*options, "filename");
> > > -    } else if (filename && !qdict_haskey(*options, "filename")) {
> > > -        qdict_put(*options, "filename", qstring_from_str(filename));
> > > -        parse_filename = true;
> > > -    } else {
> > > -        error_setg(errp, "Can't specify 'file' and 'filename' options at the "
> > > -                   "same time");
> > > -        ret = -EINVAL;
> > > -        goto fail;
> > > +    if (filename) {
> > > +        if (filename && !qdict_haskey(*options, "filename")) {
> > 
> > The inner filename testing is redundant.
> 
> Will fix.
> 
> > > +            qdict_put(*options, "filename", qstring_from_str(filename));
> > > +            parse_filename = true;
> > > +        } else {
> > > +            error_setg(errp, "Can't specify 'file' and 'filename' options at "
> > > +                             "the same time");
> > > +            return -EINVAL;
> > > +        }
> > >      }
> > >  
> > >      /* Find the right block driver */
> > > +    filename = qdict_get_try_str(*options, "filename");
> > >      drvname = qdict_get_try_str(*options, "driver");
> > > -    if (drvname) {
> > > -        drv = bdrv_find_format(drvname);
> > > -        if (!drv) {
> > > -            error_setg(errp, "Unknown driver '%s'", drvname);
> > > -        }
> > > -        qdict_del(*options, "driver");
> > > -    } else if (filename) {
> > > -        drv = bdrv_find_protocol(filename, parse_filename);
> > > -        if (!drv) {
> > > -            error_setg(errp, "Unknown protocol");
> > > +
> > > +    if (!drvname) {
> > > +        if (filename) {
> > > +            drv = bdrv_find_protocol(filename, parse_filename);
> > > +            if (!drv) {
> > > +                error_setg(errp, "Unknown protocol");
> > > +                return -EINVAL;
> > > +            }
> > > +
> > > +            drvname = drv->format_name;
> > > +            qdict_put(*options, "driver", qstring_from_str(drvname));
> > > +        } else {
> > > +            error_setg(errp, "Must specify either driver or file");
> > 
> > Isn't it "Must specify either driver or _filename_ ?
> 
> This is only code motion, but changing it in a separate patch would make
> sense, I guess.
> 
> > > +            return -EINVAL;
> > >          }
> > > -    } else {
> > > -        error_setg(errp, "Must specify either driver or file");
> > > -        drv = NULL;
> > >      }
> > >  
> > > +    drv = bdrv_find_format(drvname);
> > >      if (!drv) {
> > > -        /* errp has been set already */
> > > -        ret = -ENOENT;
> > > -        goto fail;
> > > +        error_setg(errp, "Unknown driver '%s'", drvname);
> > > +        return -ENOENT;
> > >      }
> > >  
> > > -    /* Parse the filename and open it */
> > > +    /* Driver-specific filename parsing */
> > >      if (drv->bdrv_parse_filename && parse_filename) {
> > >          drv->bdrv_parse_filename(filename, *options, &local_err);
> > >          if (local_err) {
> > >              error_propagate(errp, local_err);
> > > -            ret = -EINVAL;
> > > -            goto fail;
> > > +            return -EINVAL;
> > >          }
> > >  
> > >          if (!drv->bdrv_needs_filename) {
> > >              qdict_del(*options, "filename");
> > > -        } else {
> > > -            filename = qdict_get_str(*options, "filename");
> > >          }
> > >      }
> > >  
> > > +    return 0;
> > > +}
> > > +
> > > +/*
> > > + * Opens a file using a protocol (file, host_device, nbd, ...)
> > > + *
> > > + * options is an indirect pointer to a QDict of options to pass to the block
> > > + * drivers, or pointer to NULL for an empty set of options. If this function
> > > + * takes ownership of the QDict reference, it will set *options to NULL;
> > > + * otherwise, it will contain unused/unrecognized options after this function
> > > + * returns. Then, the caller is responsible for freeing it. If it intends to
> > > + * reuse the QDict, QINCREF() should be called beforehand.
> > > + */
> > > +static int bdrv_file_open(BlockDriverState *bs, const char *filename,
> > > +                          QDict **options, int flags, Error **errp)
> > > +{
> > > +    BlockDriver *drv;
> > > +    const char *drvname;
> > > +    Error *local_err = NULL;
> > > +    int ret;
> > > +
> > > +    ret = bdrv_fill_options(options, filename, &local_err);
> > > +    if (local_err) {
> > > +        error_propagate(errp, local_err);
> > > +        return ret;
> > Why not goto fail for consistency with the rest of the function ?
> 
> Okay. Rather pointless and the function will be gone at the end of the
> series, but more consistent indeed.
> 
> > > +    }
> > > +
> > > +    filename = qdict_get_try_str(*options, "filename");
> > > +    drvname = qdict_get_str(*options, "driver");
> > > +
> > > +    drv = bdrv_find_format(drvname);
> > > +    if (!drv) {
> > > +        error_setg(errp, "Unknown driver '%s'", drvname);
> > > +        ret = -ENOENT;
> > > +        goto fail;
> > > +    }
> > > +    qdict_del(*options, "driver");
> > > +
> > > +    /* Open the file */
> > >      if (!drv->bdrv_file_open) {
> > 
> > Don't we need:
> >            qdict_del(*options, "filename");
> > Here so bdrv_open don't see an extra filename option ?
> 
> I think it doesn't matter because bdrv_open() would put it back again.
> Do you have an example where it makes a difference?
No, I don't understand the code that much.
I was just thinking outloud betting it would point to something. It's not :)
> 
> > >          ret = bdrv_open(&bs, filename, NULL, *options, flags, drv, &local_err);
> > >          *options = NULL;
> 
> Kevin

[Qemu-devel] [PATCH 2/9] block: Move bdrv_fill_options() call to bdrv_open()

[Kevin Wolf]

bs->options now contains the modified version of the options.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block.c | 26 ++++++++++++++++----------
 1 file changed, 16 insertions(+), 10 deletions(-)

diff --git a/block.c b/block.c
index c5707e8..effb3e6 100644
--- a/block.c
+++ b/block.c
@@ -1010,14 +1010,19 @@ free_and_fail:
  * Fills in default options for opening images and converts the legacy
  * filename/flags pair to option QDict entries.
  */
-static int bdrv_fill_options(QDict **options, const char *filename,
+static int bdrv_fill_options(QDict **options, const char *filename, int flags,
                              Error **errp)
 {
     const char *drvname;
+    bool protocol = flags & BDRV_O_PROTOCOL;
     bool parse_filename = false;
     Error *local_err = NULL;
     BlockDriver *drv;
 
+    if (!protocol) {
+        return 0;
+    }
+
     /* Fetch the file name from the options QDict if necessary */
     if (filename) {
         if (filename && !qdict_haskey(*options, "filename")) {
@@ -1082,20 +1087,15 @@ static int bdrv_fill_options(QDict **options, const char *filename,
  * returns. Then, the caller is responsible for freeing it. If it intends to
  * reuse the QDict, QINCREF() should be called beforehand.
  */
-static int bdrv_file_open(BlockDriverState *bs, const char *filename,
-                          QDict **options, int flags, Error **errp)
+static int bdrv_file_open(BlockDriverState *bs, QDict **options, int flags,
+                          Error **errp)
 {
     BlockDriver *drv;
+    const char *filename;
     const char *drvname;
     Error *local_err = NULL;
     int ret;
 
-    ret = bdrv_fill_options(options, filename, &local_err);
-    if (local_err) {
-        error_propagate(errp, local_err);
-        return ret;
-    }
-
     filename = qdict_get_try_str(*options, "filename");
     drvname = qdict_get_str(*options, "driver");
 
@@ -1443,12 +1443,18 @@ int bdrv_open(BlockDriverState **pbs, const char *filename,
         filename = NULL;
     }
 
+    ret = bdrv_fill_options(&options, filename, flags, &local_err);
+    if (local_err) {
+        error_propagate(errp, local_err);
+        return ret;
+    }
+
     bs->options = options;
     options = qdict_clone_shallow(options);
 
     if (flags & BDRV_O_PROTOCOL) {
         assert(!drv);
-        ret = bdrv_file_open(bs, filename, &options, flags & ~BDRV_O_PROTOCOL,
+        ret = bdrv_file_open(bs, &options, flags & ~BDRV_O_PROTOCOL,
                              &local_err);
         if (!ret) {
             drv = bs->drv;
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH 2/9] block: Move bdrv_fill_options() call to bdrv_open()

[Eric Blake]

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

On 06/11/2014 08:04 AM, Kevin Wolf wrote:
> bs->options now contains the modified version of the options.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block.c | 26 ++++++++++++++++----------
>  1 file changed, 16 insertions(+), 10 deletions(-)
> 
> diff --git a/block.c b/block.c
> index c5707e8..effb3e6 100644
> --- a/block.c
> +++ b/block.c
> @@ -1010,14 +1010,19 @@ free_and_fail:
>   * Fills in default options for opening images and converts the legacy
>   * filename/flags pair to option QDict entries.
>   */
> -static int bdrv_fill_options(QDict **options, const char *filename,
> +static int bdrv_fill_options(QDict **options, const char *filename, int flags,
>                               Error **errp)

Ah, this fixes one of my findings on 1/9.

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

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


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

Re: [Qemu-devel] [PATCH 2/9] block: Move bdrv_fill_options() call to bdrv_open()

[Benoît Canet]

The Wednesday 11 Jun 2014 à 16:04:56 (+0200), Kevin Wolf wrote :
> bs->options now contains the modified version of the options.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block.c | 26 ++++++++++++++++----------
>  1 file changed, 16 insertions(+), 10 deletions(-)
> 
> diff --git a/block.c b/block.c
> index c5707e8..effb3e6 100644
> --- a/block.c
> +++ b/block.c
> @@ -1010,14 +1010,19 @@ free_and_fail:
>   * Fills in default options for opening images and converts the legacy
>   * filename/flags pair to option QDict entries.
>   */
> -static int bdrv_fill_options(QDict **options, const char *filename,
> +static int bdrv_fill_options(QDict **options, const char *filename, int flags,
>                               Error **errp)
>  {
>      const char *drvname;
> +    bool protocol = flags & BDRV_O_PROTOCOL;
>      bool parse_filename = false;
>      Error *local_err = NULL;
>      BlockDriver *drv;
>  
> +    if (!protocol) {
> +        return 0;
> +    }
> +

Does this mean we are sure that we don't have image format with baroque parameters
in the filename ?
This does make sense.

>      /* Fetch the file name from the options QDict if necessary */
>      if (filename) {
>          if (filename && !qdict_haskey(*options, "filename")) {
> @@ -1082,20 +1087,15 @@ static int bdrv_fill_options(QDict **options, const char *filename,
>   * returns. Then, the caller is responsible for freeing it. If it intends to
>   * reuse the QDict, QINCREF() should be called beforehand.
>   */
> -static int bdrv_file_open(BlockDriverState *bs, const char *filename,
> -                          QDict **options, int flags, Error **errp)
> +static int bdrv_file_open(BlockDriverState *bs, QDict **options, int flags,
> +                          Error **errp)
>  {
>      BlockDriver *drv;
> +    const char *filename;
>      const char *drvname;
>      Error *local_err = NULL;
>      int ret;
>  
> -    ret = bdrv_fill_options(options, filename, &local_err);
> -    if (local_err) {
> -        error_propagate(errp, local_err);
> -        return ret;
> -    }
> -
>      filename = qdict_get_try_str(*options, "filename");
>      drvname = qdict_get_str(*options, "driver");
>  
> @@ -1443,12 +1443,18 @@ int bdrv_open(BlockDriverState **pbs, const char *filename,
>          filename = NULL;
>      }
>  
> +    ret = bdrv_fill_options(&options, filename, flags, &local_err);
> +    if (local_err) {
> +        error_propagate(errp, local_err);
> +        return ret;
Chance are that bs and option where created above.

goto fail ?

> +    }
> +
>      bs->options = options;
>      options = qdict_clone_shallow(options);
>  
>      if (flags & BDRV_O_PROTOCOL) {
>          assert(!drv);
> -        ret = bdrv_file_open(bs, filename, &options, flags & ~BDRV_O_PROTOCOL,
> +        ret = bdrv_file_open(bs, &options, flags & ~BDRV_O_PROTOCOL,
>                               &local_err);
>          if (!ret) {
>              drv = bs->drv;
> -- 
> 1.8.3.1
> 
> 

[Qemu-devel] [PATCH 3/9] block: Move json: parsing to bdrv_fill_options()

[Kevin Wolf]

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block.c | 88 +++++++++++++++++++++++++++++++++--------------------------------
 1 file changed, 45 insertions(+), 43 deletions(-)

diff --git a/block.c b/block.c
index effb3e6..b9c2b41 100644
--- a/block.c
+++ b/block.c
@@ -1006,19 +1006,62 @@ free_and_fail:
     return ret;
 }
 
+static QDict *parse_json_filename(const char *filename, Error **errp)
+{
+    QObject *options_obj;
+    QDict *options;
+    int ret;
+
+    ret = strstart(filename, "json:", &filename);
+    assert(ret);
+
+    options_obj = qobject_from_json(filename);
+    if (!options_obj) {
+        error_setg(errp, "Could not parse the JSON options");
+        return NULL;
+    }
+
+    if (qobject_type(options_obj) != QTYPE_QDICT) {
+        qobject_decref(options_obj);
+        error_setg(errp, "Invalid JSON object given");
+        return NULL;
+    }
+
+    options = qobject_to_qdict(options_obj);
+    qdict_flatten(options);
+
+    return options;
+}
+
 /*
  * Fills in default options for opening images and converts the legacy
  * filename/flags pair to option QDict entries.
  */
-static int bdrv_fill_options(QDict **options, const char *filename, int flags,
+static int bdrv_fill_options(QDict **options, const char **pfilename, int flags,
                              Error **errp)
 {
+    const char *filename = *pfilename;
     const char *drvname;
     bool protocol = flags & BDRV_O_PROTOCOL;
     bool parse_filename = false;
     Error *local_err = NULL;
     BlockDriver *drv;
 
+    /* Parse json: pseudo-protocol */
+    if (filename && g_str_has_prefix(filename, "json:")) {
+        QDict *json_options = parse_json_filename(filename, &local_err);
+        if (local_err) {
+            error_propagate(errp, local_err);
+            return -EINVAL;
+        }
+
+        /* Options given in the filename have lower priority than options
+         * specified directly */
+        qdict_join(*options, json_options, false);
+        QDECREF(json_options);
+        *pfilename = filename = NULL;
+    }
+
     if (!protocol) {
         return 0;
     }
@@ -1339,33 +1382,6 @@ out:
     g_free(tmp_filename);
 }
 
-static QDict *parse_json_filename(const char *filename, Error **errp)
-{
-    QObject *options_obj;
-    QDict *options;
-    int ret;
-
-    ret = strstart(filename, "json:", &filename);
-    assert(ret);
-
-    options_obj = qobject_from_json(filename);
-    if (!options_obj) {
-        error_setg(errp, "Could not parse the JSON options");
-        return NULL;
-    }
-
-    if (qobject_type(options_obj) != QTYPE_QDICT) {
-        qobject_decref(options_obj);
-        error_setg(errp, "Invalid JSON object given");
-        return NULL;
-    }
-
-    options = qobject_to_qdict(options_obj);
-    qdict_flatten(options);
-
-    return options;
-}
-
 /*
  * Opens a disk image (raw, qcow2, vmdk, ...)
  *
@@ -1429,21 +1445,7 @@ int bdrv_open(BlockDriverState **pbs, const char *filename,
         options = qdict_new();
     }
 
-    if (filename && g_str_has_prefix(filename, "json:")) {
-        QDict *json_options = parse_json_filename(filename, &local_err);
-        if (local_err) {
-            ret = -EINVAL;
-            goto fail;
-        }
-
-        /* Options given in the filename have lower priority than options
-         * specified directly */
-        qdict_join(options, json_options, false);
-        QDECREF(json_options);
-        filename = NULL;
-    }
-
-    ret = bdrv_fill_options(&options, filename, flags, &local_err);
+    ret = bdrv_fill_options(&options, &filename, flags, &local_err);
     if (local_err) {
         error_propagate(errp, local_err);
         return ret;
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH 3/9] block: Move json: parsing to bdrv_fill_options()

[Eric Blake]

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

On 06/11/2014 08:04 AM, Kevin Wolf wrote:
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block.c | 88 +++++++++++++++++++++++++++++++++--------------------------------
>  1 file changed, 45 insertions(+), 43 deletions(-)

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

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


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

Re: [Qemu-devel] [PATCH 3/9] block: Move json: parsing to bdrv_fill_options()

[Benoît Canet]

The Wednesday 11 Jun 2014 à 16:04:57 (+0200), Kevin Wolf wrote :
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block.c | 88 +++++++++++++++++++++++++++++++++--------------------------------
>  1 file changed, 45 insertions(+), 43 deletions(-)
> 
> diff --git a/block.c b/block.c
> index effb3e6..b9c2b41 100644
> --- a/block.c
> +++ b/block.c
> @@ -1006,19 +1006,62 @@ free_and_fail:
>      return ret;
>  }
>  
> +static QDict *parse_json_filename(const char *filename, Error **errp)
> +{
> +    QObject *options_obj;
> +    QDict *options;
> +    int ret;
> +
> +    ret = strstart(filename, "json:", &filename);
> +    assert(ret);
> +
> +    options_obj = qobject_from_json(filename);
> +    if (!options_obj) {
> +        error_setg(errp, "Could not parse the JSON options");
> +        return NULL;
> +    }
> +
> +    if (qobject_type(options_obj) != QTYPE_QDICT) {
> +        qobject_decref(options_obj);
> +        error_setg(errp, "Invalid JSON object given");
> +        return NULL;
> +    }
> +
> +    options = qobject_to_qdict(options_obj);
> +    qdict_flatten(options);
> +
> +    return options;
> +}

I am under the impression that this code move could be avoided by using a function
prototype: the patch would be less cluttered.

> +
>  /*
>   * Fills in default options for opening images and converts the legacy
>   * filename/flags pair to option QDict entries.
>   */
> -static int bdrv_fill_options(QDict **options, const char *filename, int flags,
> +static int bdrv_fill_options(QDict **options, const char **pfilename, int flags,
>                               Error **errp)
>  {
> +    const char *filename = *pfilename;
>      const char *drvname;
>      bool protocol = flags & BDRV_O_PROTOCOL;
>      bool parse_filename = false;
>      Error *local_err = NULL;
>      BlockDriver *drv;
>  
> +    /* Parse json: pseudo-protocol */
> +    if (filename && g_str_has_prefix(filename, "json:")) {
> +        QDict *json_options = parse_json_filename(filename, &local_err);
> +        if (local_err) {
> +            error_propagate(errp, local_err);
> +            return -EINVAL;
> +        }
> +
> +        /* Options given in the filename have lower priority than options
> +         * specified directly */
> +        qdict_join(*options, json_options, false);
> +        QDECREF(json_options);
> +        *pfilename = filename = NULL;
> +    }
> +
>      if (!protocol) {
>          return 0;
>      }
> @@ -1339,33 +1382,6 @@ out:
>      g_free(tmp_filename);
>  }
>  
> -static QDict *parse_json_filename(const char *filename, Error **errp)
> -{
> -    QObject *options_obj;
> -    QDict *options;
> -    int ret;
> -
> -    ret = strstart(filename, "json:", &filename);
> -    assert(ret);
> -
> -    options_obj = qobject_from_json(filename);
> -    if (!options_obj) {
> -        error_setg(errp, "Could not parse the JSON options");
> -        return NULL;
> -    }
> -
> -    if (qobject_type(options_obj) != QTYPE_QDICT) {
> -        qobject_decref(options_obj);
> -        error_setg(errp, "Invalid JSON object given");
> -        return NULL;
> -    }
> -
> -    options = qobject_to_qdict(options_obj);
> -    qdict_flatten(options);
> -
> -    return options;
> -}
Second part of the uneeded move.
Maybe you could declare bdrv_fill_options below parse_json_filename in the first
patch to avoid the ulterior move.

> -
>  /*
>   * Opens a disk image (raw, qcow2, vmdk, ...)
>   *
> @@ -1429,21 +1445,7 @@ int bdrv_open(BlockDriverState **pbs, const char *filename,
>          options = qdict_new();
>      }
>  
> -    if (filename && g_str_has_prefix(filename, "json:")) {
> -        QDict *json_options = parse_json_filename(filename, &local_err);
> -        if (local_err) {
> -            ret = -EINVAL;
> -            goto fail;
> -        }
> -
> -        /* Options given in the filename have lower priority than options
> -         * specified directly */
> -        qdict_join(options, json_options, false);
> -        QDECREF(json_options);
> -        filename = NULL;
> -    }
> -
> -    ret = bdrv_fill_options(&options, filename, flags, &local_err);
> +    ret = bdrv_fill_options(&options, &filename, flags, &local_err);
>      if (local_err) {
>          error_propagate(errp, local_err);
>          return ret;
> -- 
> 1.8.3.1
> 
> 

Re: [Qemu-devel] [PATCH 3/9] block: Move json: parsing to bdrv_fill_options()

[Eric Blake]

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

On 06/12/2014 06:26 AM, Benoît Canet wrote:

>> +static QDict *parse_json_filename(const char *filename, Error **errp)
>> +{

> 
> I am under the impression that this code move could be avoided by using a function
> prototype: the patch would be less cluttered.

I'm a fan of avoiding function prototypes for static functions when
possible (listing things in topological order is nicer for someone
reading the file for the first time - prototypes only help for recursive
functions), so I'm in favor of keeping the code motion.  But splitting
code motion into its own patch rather than mixing it with other changes
is nicer on reviewers.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


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

[Qemu-devel] [PATCH 4/9] block: Always pass driver name through options QDict

[Kevin Wolf]

The "driver" entry in the options QDict is now only missing if we're
opening an image with format probing.

We also catch cases now where both the drv argument and a "driver"
option is specified, e.g. by specifying -drive format=qcow2,driver=raw

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block.c                    | 76 ++++++++++++++++++++++++----------------------
 tests/qemu-iotests/051     |  1 +
 tests/qemu-iotests/051.out |  5 ++-
 3 files changed, 45 insertions(+), 37 deletions(-)

diff --git a/block.c b/block.c
index b9c2b41..011cfc4 100644
--- a/block.c
+++ b/block.c
@@ -1038,14 +1038,13 @@ static QDict *parse_json_filename(const char *filename, Error **errp)
  * filename/flags pair to option QDict entries.
  */
 static int bdrv_fill_options(QDict **options, const char **pfilename, int flags,
-                             Error **errp)
+                             BlockDriver *drv, Error **errp)
 {
     const char *filename = *pfilename;
     const char *drvname;
     bool protocol = flags & BDRV_O_PROTOCOL;
     bool parse_filename = false;
     Error *local_err = NULL;
-    BlockDriver *drv;
 
     /* Parse json: pseudo-protocol */
     if (filename && g_str_has_prefix(filename, "json:")) {
@@ -1062,12 +1061,8 @@ static int bdrv_fill_options(QDict **options, const char **pfilename, int flags,
         *pfilename = filename = NULL;
     }
 
-    if (!protocol) {
-        return 0;
-    }
-
     /* Fetch the file name from the options QDict if necessary */
-    if (filename) {
+    if (protocol && filename) {
         if (filename && !qdict_haskey(*options, "filename")) {
             qdict_put(*options, "filename", qstring_from_str(filename));
             parse_filename = true;
@@ -1082,30 +1077,41 @@ static int bdrv_fill_options(QDict **options, const char **pfilename, int flags,
     filename = qdict_get_try_str(*options, "filename");
     drvname = qdict_get_try_str(*options, "driver");
 
-    if (!drvname) {
-        if (filename) {
-            drv = bdrv_find_protocol(filename, parse_filename);
-            if (!drv) {
-                error_setg(errp, "Unknown protocol");
+    if (drv) {
+        if (drvname) {
+            error_setg(errp, "Driver specified twice");
+            return -EINVAL;
+        }
+        drvname = drv->format_name;
+        qdict_put(*options, "driver", qstring_from_str(drvname));
+    } else {
+        if (!drvname && protocol) {
+            if (filename) {
+                drv = bdrv_find_protocol(filename, parse_filename);
+                if (!drv) {
+                    error_setg(errp, "Unknown protocol");
+                    return -EINVAL;
+                }
+
+                drvname = drv->format_name;
+                qdict_put(*options, "driver", qstring_from_str(drvname));
+            } else {
+                error_setg(errp, "Must specify either driver or file");
                 return -EINVAL;
             }
-
-            drvname = drv->format_name;
-            qdict_put(*options, "driver", qstring_from_str(drvname));
-        } else {
-            error_setg(errp, "Must specify either driver or file");
-            return -EINVAL;
+        } else if (drvname) {
+            drv = bdrv_find_format(drvname);
+            if (!drv) {
+                error_setg(errp, "Unknown driver '%s'", drvname);
+                return -ENOENT;
+            }
         }
     }
 
-    drv = bdrv_find_format(drvname);
-    if (!drv) {
-        error_setg(errp, "Unknown driver '%s'", drvname);
-        return -ENOENT;
-    }
+    assert(drv || !protocol);
 
     /* Driver-specific filename parsing */
-    if (drv->bdrv_parse_filename && parse_filename) {
+    if (drv && drv->bdrv_parse_filename && parse_filename) {
         drv->bdrv_parse_filename(filename, *options, &local_err);
         if (local_err) {
             error_propagate(errp, local_err);
@@ -1445,7 +1451,7 @@ int bdrv_open(BlockDriverState **pbs, const char *filename,
         options = qdict_new();
     }
 
-    ret = bdrv_fill_options(&options, &filename, flags, &local_err);
+    ret = bdrv_fill_options(&options, &filename, flags, drv, &local_err);
     if (local_err) {
         error_propagate(errp, local_err);
         return ret;
@@ -1486,7 +1492,10 @@ int bdrv_open(BlockDriverState **pbs, const char *filename,
     }
 
     /* Find the right image format driver */
+    drv = NULL;
     drvname = qdict_get_try_str(options, "driver");
+    assert(drvname || !(flags & BDRV_O_PROTOCOL));
+
     if (drvname) {
         drv = bdrv_find_format(drvname);
         qdict_del(options, "driver");
@@ -1495,19 +1504,14 @@ int bdrv_open(BlockDriverState **pbs, const char *filename,
             ret = -EINVAL;
             goto fail;
         }
-    }
-
-    if (!drv) {
-        if (file) {
-            ret = find_image_format(file, filename, &drv, &local_err);
-        } else {
-            error_setg(errp, "Must specify either driver or file");
-            ret = -EINVAL;
+    } else if (file) {
+        ret = find_image_format(file, filename, &drv, &local_err);
+        if (ret < 0) {
             goto fail;
         }
-    }
-
-    if (!drv) {
+    } else {
+        error_setg(errp, "Must specify either driver or file");
+        ret = -EINVAL;
         goto fail;
     }
 
diff --git a/tests/qemu-iotests/051 b/tests/qemu-iotests/051
index c4af131..30a712f 100755
--- a/tests/qemu-iotests/051
+++ b/tests/qemu-iotests/051
@@ -92,6 +92,7 @@ echo
 
 run_qemu -drive file="$TEST_IMG",format=foo
 run_qemu -drive file="$TEST_IMG",driver=foo
+run_qemu -drive file="$TEST_IMG",driver=raw,format=qcow2
 
 echo
 echo === Overriding backing file ===
diff --git a/tests/qemu-iotests/051.out b/tests/qemu-iotests/051.out
index ad60107..37cb049 100644
--- a/tests/qemu-iotests/051.out
+++ b/tests/qemu-iotests/051.out
@@ -38,7 +38,10 @@ Testing: -drive file=TEST_DIR/t.qcow2,format=foo
 QEMU_PROG: -drive file=TEST_DIR/t.qcow2,format=foo: 'foo' invalid format
 
 Testing: -drive file=TEST_DIR/t.qcow2,driver=foo
-QEMU_PROG: -drive file=TEST_DIR/t.qcow2,driver=foo: could not open disk image TEST_DIR/t.qcow2: Invalid driver: 'foo'
+QEMU_PROG: -drive file=TEST_DIR/t.qcow2,driver=foo: could not open disk image TEST_DIR/t.qcow2: Unknown driver 'foo'
+
+Testing: -drive file=TEST_DIR/t.qcow2,driver=raw,format=qcow2
+QEMU_PROG: -drive file=TEST_DIR/t.qcow2,driver=raw,format=qcow2: could not open disk image TEST_DIR/t.qcow2: Driver specified twice
 
 
 === Overriding backing file ===
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH 4/9] block: Always pass driver name through options QDict

[Eric Blake]

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

On 06/11/2014 08:04 AM, Kevin Wolf wrote:
> The "driver" entry in the options QDict is now only missing if we're
> opening an image with format probing.
> 
> We also catch cases now where both the drv argument and a "driver"
> option is specified, e.g. by specifying -drive format=qcow2,driver=raw
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block.c                    | 76 ++++++++++++++++++++++++----------------------
>  tests/qemu-iotests/051     |  1 +
>  tests/qemu-iotests/051.out |  5 ++-
>  3 files changed, 45 insertions(+), 37 deletions(-)
> 

> @@ -1062,12 +1061,8 @@ static int bdrv_fill_options(QDict **options, const char **pfilename, int flags,
>          *pfilename = filename = NULL;
>      }
>  
> -    if (!protocol) {
> -        return 0;
> -    }
> -
>      /* Fetch the file name from the options QDict if necessary */
> -    if (filename) {
> +    if (protocol && filename) {
>          if (filename && !qdict_haskey(*options, "filename")) {

Slight conflict here when you clean up the dead 'filename &&' in patch
1; obvious resolution.

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

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


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

Re: [Qemu-devel] [PATCH 4/9] block: Always pass driver name through options QDict

[Benoît Canet]

The Wednesday 11 Jun 2014 à 16:04:58 (+0200), Kevin Wolf wrote :
> The "driver" entry in the options QDict is now only missing if we're
> opening an image with format probing.
> 
> We also catch cases now where both the drv argument and a "driver"
> option is specified, e.g. by specifying -drive format=qcow2,driver=raw
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block.c                    | 76 ++++++++++++++++++++++++----------------------
>  tests/qemu-iotests/051     |  1 +
>  tests/qemu-iotests/051.out |  5 ++-
>  3 files changed, 45 insertions(+), 37 deletions(-)
> 
> diff --git a/block.c b/block.c
> index b9c2b41..011cfc4 100644
> --- a/block.c
> +++ b/block.c
> @@ -1038,14 +1038,13 @@ static QDict *parse_json_filename(const char *filename, Error **errp)
>   * filename/flags pair to option QDict entries.
>   */
>  static int bdrv_fill_options(QDict **options, const char **pfilename, int flags,
> -                             Error **errp)
> +                             BlockDriver *drv, Error **errp)
>  {
>      const char *filename = *pfilename;
>      const char *drvname;
>      bool protocol = flags & BDRV_O_PROTOCOL;
>      bool parse_filename = false;
>      Error *local_err = NULL;
> -    BlockDriver *drv;
>  
>      /* Parse json: pseudo-protocol */
>      if (filename && g_str_has_prefix(filename, "json:")) {
> @@ -1062,12 +1061,8 @@ static int bdrv_fill_options(QDict **options, const char **pfilename, int flags,
>          *pfilename = filename = NULL;
>      }
>  
> -    if (!protocol) {
> -        return 0;
> -    }
> -
>      /* Fetch the file name from the options QDict if necessary */
> -    if (filename) {
> +    if (protocol && filename) {
>          if (filename && !qdict_haskey(*options, "filename")) {
>              qdict_put(*options, "filename", qstring_from_str(filename));
>              parse_filename = true;
> @@ -1082,30 +1077,41 @@ static int bdrv_fill_options(QDict **options, const char **pfilename, int flags,
>      filename = qdict_get_try_str(*options, "filename");
>      drvname = qdict_get_try_str(*options, "driver");
>  
> -    if (!drvname) {
> -        if (filename) {
> -            drv = bdrv_find_protocol(filename, parse_filename);
> -            if (!drv) {
> -                error_setg(errp, "Unknown protocol");
> +    if (drv) {
> +        if (drvname) {
> +            error_setg(errp, "Driver specified twice");
> +            return -EINVAL;
> +        }
> +        drvname = drv->format_name;
> +        qdict_put(*options, "driver", qstring_from_str(drvname));
> +    } else {
> +        if (!drvname && protocol) {
> +            if (filename) {
> +                drv = bdrv_find_protocol(filename, parse_filename);
> +                if (!drv) {
> +                    error_setg(errp, "Unknown protocol");
> +                    return -EINVAL;
> +                }
> +
> +                drvname = drv->format_name;
> +                qdict_put(*options, "driver", qstring_from_str(drvname));
> +            } else {
> +                error_setg(errp, "Must specify either driver or file");
>                  return -EINVAL;
>              }
> -
> -            drvname = drv->format_name;
> -            qdict_put(*options, "driver", qstring_from_str(drvname));
> -        } else {
> -            error_setg(errp, "Must specify either driver or file");
> -            return -EINVAL;
> +        } else if (drvname) {
> +            drv = bdrv_find_format(drvname);
> +            if (!drv) {
> +                error_setg(errp, "Unknown driver '%s'", drvname);
> +                return -ENOENT;
> +            }
>          }
>      }
>  
> -    drv = bdrv_find_format(drvname);
> -    if (!drv) {
> -        error_setg(errp, "Unknown driver '%s'", drvname);
> -        return -ENOENT;
> -    }
> +    assert(drv || !protocol);
>  
>      /* Driver-specific filename parsing */
> -    if (drv->bdrv_parse_filename && parse_filename) {
> +    if (drv && drv->bdrv_parse_filename && parse_filename) {
>          drv->bdrv_parse_filename(filename, *options, &local_err);
>          if (local_err) {
>              error_propagate(errp, local_err);
> @@ -1445,7 +1451,7 @@ int bdrv_open(BlockDriverState **pbs, const char *filename,
>          options = qdict_new();
>      }
>  
> -    ret = bdrv_fill_options(&options, &filename, flags, &local_err);
> +    ret = bdrv_fill_options(&options, &filename, flags, drv, &local_err);
>      if (local_err) {
>          error_propagate(errp, local_err);
>          return ret;
> @@ -1486,7 +1492,10 @@ int bdrv_open(BlockDriverState **pbs, const char *filename,
>      }
>  
>      /* Find the right image format driver */
> +    drv = NULL;
>      drvname = qdict_get_try_str(options, "driver");
> +    assert(drvname || !(flags & BDRV_O_PROTOCOL));
> +
>      if (drvname) {
>          drv = bdrv_find_format(drvname);
>          qdict_del(options, "driver");
> @@ -1495,19 +1504,14 @@ int bdrv_open(BlockDriverState **pbs, const char *filename,
>              ret = -EINVAL;
>              goto fail;
>          }
> -    }
> -
> -    if (!drv) {
> -        if (file) {
> -            ret = find_image_format(file, filename, &drv, &local_err);
> -        } else {
> -            error_setg(errp, "Must specify either driver or file");
> -            ret = -EINVAL;
> +    } else if (file) {
> +        ret = find_image_format(file, filename, &drv, &local_err);
> +        if (ret < 0) {
>              goto fail;
>          }
> -    }
> -
> -    if (!drv) {
> +    } else {
> +        error_setg(errp, "Must specify either driver or file");
> +        ret = -EINVAL;
>          goto fail;
>      }
>  
> diff --git a/tests/qemu-iotests/051 b/tests/qemu-iotests/051
> index c4af131..30a712f 100755
> --- a/tests/qemu-iotests/051
> +++ b/tests/qemu-iotests/051
> @@ -92,6 +92,7 @@ echo
>  
>  run_qemu -drive file="$TEST_IMG",format=foo
>  run_qemu -drive file="$TEST_IMG",driver=foo
> +run_qemu -drive file="$TEST_IMG",driver=raw,format=qcow2
>  
>  echo
>  echo === Overriding backing file ===
> diff --git a/tests/qemu-iotests/051.out b/tests/qemu-iotests/051.out
> index ad60107..37cb049 100644
> --- a/tests/qemu-iotests/051.out
> +++ b/tests/qemu-iotests/051.out
> @@ -38,7 +38,10 @@ Testing: -drive file=TEST_DIR/t.qcow2,format=foo
>  QEMU_PROG: -drive file=TEST_DIR/t.qcow2,format=foo: 'foo' invalid format
>  
>  Testing: -drive file=TEST_DIR/t.qcow2,driver=foo
> -QEMU_PROG: -drive file=TEST_DIR/t.qcow2,driver=foo: could not open disk image TEST_DIR/t.qcow2: Invalid driver: 'foo'
> +QEMU_PROG: -drive file=TEST_DIR/t.qcow2,driver=foo: could not open disk image TEST_DIR/t.qcow2: Unknown driver 'foo'
> +
> +Testing: -drive file=TEST_DIR/t.qcow2,driver=raw,format=qcow2
> +QEMU_PROG: -drive file=TEST_DIR/t.qcow2,driver=raw,format=qcow2: could not open disk image TEST_DIR/t.qcow2: Driver specified twice
>  
>  
>  === Overriding backing file ===
> -- 
> 1.8.3.1
> 
> 
That's faily huge patch. I didn't understood everything.

[Qemu-devel] [PATCH 5/9] block: Use common driver selection code for bdrv_open_file()

[Kevin Wolf]

This moves the bdrv_open_file() call a bit down so that it can use the
bdrv_open() code that selects the right block driver.

The code between the old and the new call site is either common code
(the error message for an unknown driver has been unified now) or
doesn't run with cleared BDRV_O_PROTOCOL (added an if block in one
place, whereas the right path was already asserted in another place)

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block.c | 71 ++++++++++++++++++++++++++++-------------------------------------
 1 file changed, 30 insertions(+), 41 deletions(-)

diff --git a/block.c b/block.c
index 011cfc4..2aa0a56 100644
--- a/block.c
+++ b/block.c
@@ -1136,25 +1136,14 @@ static int bdrv_fill_options(QDict **options, const char **pfilename, int flags,
  * returns. Then, the caller is responsible for freeing it. If it intends to
  * reuse the QDict, QINCREF() should be called beforehand.
  */
-static int bdrv_file_open(BlockDriverState *bs, QDict **options, int flags,
-                          Error **errp)
+static int bdrv_file_open(BlockDriverState *bs, BlockDriver *drv,
+                          QDict **options, int flags, Error **errp)
 {
-    BlockDriver *drv;
     const char *filename;
-    const char *drvname;
     Error *local_err = NULL;
     int ret;
 
     filename = qdict_get_try_str(*options, "filename");
-    drvname = qdict_get_str(*options, "driver");
-
-    drv = bdrv_find_format(drvname);
-    if (!drv) {
-        error_setg(errp, "Unknown driver '%s'", drvname);
-        ret = -ENOENT;
-        goto fail;
-    }
-    qdict_del(*options, "driver");
 
     /* Open the file */
     if (!drv->bdrv_file_open) {
@@ -1460,35 +1449,23 @@ int bdrv_open(BlockDriverState **pbs, const char *filename,
     bs->options = options;
     options = qdict_clone_shallow(options);
 
-    if (flags & BDRV_O_PROTOCOL) {
-        assert(!drv);
-        ret = bdrv_file_open(bs, &options, flags & ~BDRV_O_PROTOCOL,
-                             &local_err);
-        if (!ret) {
-            drv = bs->drv;
-            goto done;
-        } else if (bs->drv) {
-            goto close_and_fail;
-        } else {
-            goto fail;
-        }
-    }
-
     /* Open image file without format layer */
-    if (flags & BDRV_O_RDWR) {
-        flags |= BDRV_O_ALLOW_RDWR;
-    }
-    if (flags & BDRV_O_SNAPSHOT) {
-        snapshot_flags = bdrv_temp_snapshot_flags(flags);
-        flags = bdrv_backing_flags(flags);
-    }
+    if ((flags & BDRV_O_PROTOCOL) == 0) {
+        if (flags & BDRV_O_RDWR) {
+            flags |= BDRV_O_ALLOW_RDWR;
+        }
+        if (flags & BDRV_O_SNAPSHOT) {
+            snapshot_flags = bdrv_temp_snapshot_flags(flags);
+            flags = bdrv_backing_flags(flags);
+        }
 
-    assert(file == NULL);
-    ret = bdrv_open_image(&file, filename, options, "file",
-                          bdrv_inherited_flags(flags),
-                          true, &local_err);
-    if (ret < 0) {
-        goto fail;
+        assert(file == NULL);
+        ret = bdrv_open_image(&file, filename, options, "file",
+                              bdrv_inherited_flags(flags),
+                              true, &local_err);
+        if (ret < 0) {
+            goto fail;
+        }
     }
 
     /* Find the right image format driver */
@@ -1500,7 +1477,7 @@ int bdrv_open(BlockDriverState **pbs, const char *filename,
         drv = bdrv_find_format(drvname);
         qdict_del(options, "driver");
         if (!drv) {
-            error_setg(errp, "Invalid driver: '%s'", drvname);
+            error_setg(errp, "Unknown driver: '%s'", drvname);
             ret = -EINVAL;
             goto fail;
         }
@@ -1516,6 +1493,18 @@ int bdrv_open(BlockDriverState **pbs, const char *filename,
     }
 
     /* Open the image */
+    if (flags & BDRV_O_PROTOCOL) {
+        ret = bdrv_file_open(bs, drv, &options, flags & ~BDRV_O_PROTOCOL,
+                             &local_err);
+        if (!ret) {
+            goto done;
+        } else if (bs->drv) {
+            goto close_and_fail;
+        } else {
+            goto fail;
+        }
+    }
+
     ret = bdrv_open_common(bs, file, options, flags, drv, &local_err);
     if (ret < 0) {
         goto fail;
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH 5/9] block: Use common driver selection code for bdrv_open_file()

[Eric Blake]

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

On 06/11/2014 08:04 AM, Kevin Wolf wrote:
> This moves the bdrv_open_file() call a bit down so that it can use the
> bdrv_open() code that selects the right block driver.
> 
> The code between the old and the new call site is either common code
> (the error message for an unknown driver has been unified now) or
> doesn't run with cleared BDRV_O_PROTOCOL (added an if block in one
> place, whereas the right path was already asserted in another place)
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block.c | 71 ++++++++++++++++++++++++++++-------------------------------------
>  1 file changed, 30 insertions(+), 41 deletions(-)
> 

> diff --git a/block.c b/block.c
> index 011cfc4..2aa0a56 100644
> --- a/block.c
> +++ b/block.c
> @@ -1136,25 +1136,14 @@ static int bdrv_fill_options(QDict **options, const char **pfilename, int flags,
>   * returns. Then, the caller is responsible for freeing it. If it intends to
>   * reuse the QDict, QINCREF() should be called beforehand.
>   */
> -static int bdrv_file_open(BlockDriverState *bs, QDict **options, int flags,
> -                          Error **errp)
> +static int bdrv_file_open(BlockDriverState *bs, BlockDriver *drv,
> +                          QDict **options, int flags, Error **errp)
>  {
> -    BlockDriver *drv;
>      const char *filename;
> -    const char *drvname;
>      Error *local_err = NULL;
>      int ret;
>  
>      filename = qdict_get_try_str(*options, "filename");
> -    drvname = qdict_get_str(*options, "driver");
> -
> -    drv = bdrv_find_format(drvname);
> -    if (!drv) {
> -        error_setg(errp, "Unknown driver '%s'", drvname);
> -        ret = -ENOENT;
> -        goto fail;
> -    }
> -    qdict_del(*options, "driver");

Ah, this addresses another question I asked earlier.

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

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


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

Re: [Qemu-devel] [PATCH 5/9] block: Use common driver selection code for bdrv_open_file()

[Benoît Canet]

The Wednesday 11 Jun 2014 à 16:04:59 (+0200), Kevin Wolf wrote :
> This moves the bdrv_open_file() call a bit down so that it can use the
> bdrv_open() code that selects the right block driver.
> 
> The code between the old and the new call site is either common code
> (the error message for an unknown driver has been unified now) or
> doesn't run with cleared BDRV_O_PROTOCOL (added an if block in one
> place, whereas the right path was already asserted in another place)
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block.c | 71 ++++++++++++++++++++++++++++-------------------------------------
>  1 file changed, 30 insertions(+), 41 deletions(-)
> 
> diff --git a/block.c b/block.c
> index 011cfc4..2aa0a56 100644
> --- a/block.c
> +++ b/block.c
> @@ -1136,25 +1136,14 @@ static int bdrv_fill_options(QDict **options, const char **pfilename, int flags,
>   * returns. Then, the caller is responsible for freeing it. If it intends to
>   * reuse the QDict, QINCREF() should be called beforehand.
>   */
> -static int bdrv_file_open(BlockDriverState *bs, QDict **options, int flags,
> -                          Error **errp)
> +static int bdrv_file_open(BlockDriverState *bs, BlockDriver *drv,
> +                          QDict **options, int flags, Error **errp)
>  {
> -    BlockDriver *drv;
>      const char *filename;
> -    const char *drvname;
>      Error *local_err = NULL;
>      int ret;
>  
>      filename = qdict_get_try_str(*options, "filename");
> -    drvname = qdict_get_str(*options, "driver");
> -
> -    drv = bdrv_find_format(drvname);
> -    if (!drv) {
> -        error_setg(errp, "Unknown driver '%s'", drvname);
> -        ret = -ENOENT;
> -        goto fail;
> -    }
> -    qdict_del(*options, "driver");
>  
>      /* Open the file */
>      if (!drv->bdrv_file_open) {
> @@ -1460,35 +1449,23 @@ int bdrv_open(BlockDriverState **pbs, const char *filename,
>      bs->options = options;
>      options = qdict_clone_shallow(options);
>  
> -    if (flags & BDRV_O_PROTOCOL) {
maybe you could use a bool protocol = flags & BDRV_O_PROTOCOL at the begining of
the function.
> -        assert(!drv);
> -        ret = bdrv_file_open(bs, &options, flags & ~BDRV_O_PROTOCOL,
> -                             &local_err);
> -        if (!ret) {
> -            drv = bs->drv;
> -            goto done;
> -        } else if (bs->drv) {
> -            goto close_and_fail;
> -        } else {
> -            goto fail;
> -        }
> -    }
> -
>      /* Open image file without format layer */
> -    if (flags & BDRV_O_RDWR) {
> -        flags |= BDRV_O_ALLOW_RDWR;
> -    }
> -    if (flags & BDRV_O_SNAPSHOT) {
> -        snapshot_flags = bdrv_temp_snapshot_flags(flags);
> -        flags = bdrv_backing_flags(flags);
> -    }
> +    if ((flags & BDRV_O_PROTOCOL) == 0) {
> +        if (flags & BDRV_O_RDWR) {
> +            flags |= BDRV_O_ALLOW_RDWR;
> +        }
> +        if (flags & BDRV_O_SNAPSHOT) {
> +            snapshot_flags = bdrv_temp_snapshot_flags(flags);
> +            flags = bdrv_backing_flags(flags);
> +        }
>  
> -    assert(file == NULL);
> -    ret = bdrv_open_image(&file, filename, options, "file",
> -                          bdrv_inherited_flags(flags),
> -                          true, &local_err);
> -    if (ret < 0) {
> -        goto fail;
> +        assert(file == NULL);
> +        ret = bdrv_open_image(&file, filename, options, "file",
> +                              bdrv_inherited_flags(flags),
> +                              true, &local_err);
> +        if (ret < 0) {
> +            goto fail;
> +        }
>      }
>  
>      /* Find the right image format driver */
> @@ -1500,7 +1477,7 @@ int bdrv_open(BlockDriverState **pbs, const char *filename,
>          drv = bdrv_find_format(drvname);
>          qdict_del(options, "driver");
>          if (!drv) {
> -            error_setg(errp, "Invalid driver: '%s'", drvname);
> +            error_setg(errp, "Unknown driver: '%s'", drvname);
>              ret = -EINVAL;
>              goto fail;
>          }
> @@ -1516,6 +1493,18 @@ int bdrv_open(BlockDriverState **pbs, const char *filename,
>      }
>  
>      /* Open the image */
> +    if (flags & BDRV_O_PROTOCOL) {
> +        ret = bdrv_file_open(bs, drv, &options, flags & ~BDRV_O_PROTOCOL,
> +                             &local_err);
> +        if (!ret) {
> +            goto done;
> +        } else if (bs->drv) {
> +            goto close_and_fail;
> +        } else {
> +            goto fail;
> +        }
> +    }
> +
>      ret = bdrv_open_common(bs, file, options, flags, drv, &local_err);
>      if (ret < 0) {
>          goto fail;
> -- 
> 1.8.3.1
> 
> 
Reviewed-by: Benoit Canet <benoit@irqsave.net>

[Qemu-devel] [PATCH 6/9] block: Inline bdrv_file_open()

[Kevin Wolf]

It doesn't do much any more, we can move the code to bdrv_open() now.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block.c | 51 +++++++++++----------------------------------------
 1 file changed, 11 insertions(+), 40 deletions(-)

diff --git a/block.c b/block.c
index 2aa0a56..034656f 100644
--- a/block.c
+++ b/block.c
@@ -1126,44 +1126,6 @@ static int bdrv_fill_options(QDict **options, const char **pfilename, int flags,
     return 0;
 }
 
-/*
- * Opens a file using a protocol (file, host_device, nbd, ...)
- *
- * options is an indirect pointer to a QDict of options to pass to the block
- * drivers, or pointer to NULL for an empty set of options. If this function
- * takes ownership of the QDict reference, it will set *options to NULL;
- * otherwise, it will contain unused/unrecognized options after this function
- * returns. Then, the caller is responsible for freeing it. If it intends to
- * reuse the QDict, QINCREF() should be called beforehand.
- */
-static int bdrv_file_open(BlockDriverState *bs, BlockDriver *drv,
-                          QDict **options, int flags, Error **errp)
-{
-    const char *filename;
-    Error *local_err = NULL;
-    int ret;
-
-    filename = qdict_get_try_str(*options, "filename");
-
-    /* Open the file */
-    if (!drv->bdrv_file_open) {
-        ret = bdrv_open(&bs, filename, NULL, *options, flags, drv, &local_err);
-        *options = NULL;
-    } else {
-        ret = bdrv_open_common(bs, NULL, *options, flags, drv, &local_err);
-    }
-    if (ret < 0) {
-        error_propagate(errp, local_err);
-        goto fail;
-    }
-
-    bs->growable = 1;
-    return 0;
-
-fail:
-    return ret;
-}
-
 void bdrv_set_backing_hd(BlockDriverState *bs, BlockDriverState *backing_hd)
 {
 
@@ -1494,9 +1456,18 @@ int bdrv_open(BlockDriverState **pbs, const char *filename,
 
     /* Open the image */
     if (flags & BDRV_O_PROTOCOL) {
-        ret = bdrv_file_open(bs, drv, &options, flags & ~BDRV_O_PROTOCOL,
-                             &local_err);
+        if (!drv->bdrv_file_open) {
+            const char *filename;
+            filename = qdict_get_try_str(options, "filename");
+            ret = bdrv_open(&bs, filename, NULL, options,
+                            flags & ~BDRV_O_PROTOCOL, drv, &local_err);
+            options = NULL;
+        } else {
+            ret = bdrv_open_common(bs, NULL, options,
+                                   flags & ~BDRV_O_PROTOCOL, drv, &local_err);
+        }
         if (!ret) {
+            bs->growable = 1;
             goto done;
         } else if (bs->drv) {
             goto close_and_fail;
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH 6/9] block: Inline bdrv_file_open()

[Benoît Canet]

The Wednesday 11 Jun 2014 à 16:05:00 (+0200), Kevin Wolf wrote :
> It doesn't do much any more, we can move the code to bdrv_open() now.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block.c | 51 +++++++++++----------------------------------------
>  1 file changed, 11 insertions(+), 40 deletions(-)
> 
> diff --git a/block.c b/block.c
> index 2aa0a56..034656f 100644
> --- a/block.c
> +++ b/block.c
> @@ -1126,44 +1126,6 @@ static int bdrv_fill_options(QDict **options, const char **pfilename, int flags,
>      return 0;
>  }
>  
> -/*
> - * Opens a file using a protocol (file, host_device, nbd, ...)
> - *
> - * options is an indirect pointer to a QDict of options to pass to the block
> - * drivers, or pointer to NULL for an empty set of options. If this function
> - * takes ownership of the QDict reference, it will set *options to NULL;
> - * otherwise, it will contain unused/unrecognized options after this function
> - * returns. Then, the caller is responsible for freeing it. If it intends to
> - * reuse the QDict, QINCREF() should be called beforehand.
> - */
> -static int bdrv_file_open(BlockDriverState *bs, BlockDriver *drv,
> -                          QDict **options, int flags, Error **errp)
> -{
> -    const char *filename;
> -    Error *local_err = NULL;
> -    int ret;
> -
> -    filename = qdict_get_try_str(*options, "filename");
> -
> -    /* Open the file */
> -    if (!drv->bdrv_file_open) {
> -        ret = bdrv_open(&bs, filename, NULL, *options, flags, drv, &local_err);
> -        *options = NULL;
> -    } else {
> -        ret = bdrv_open_common(bs, NULL, *options, flags, drv, &local_err);
> -    }
> -    if (ret < 0) {
> -        error_propagate(errp, local_err);
> -        goto fail;
> -    }
> -
> -    bs->growable = 1;
> -    return 0;
> -
> -fail:
> -    return ret;
> -}
> -
>  void bdrv_set_backing_hd(BlockDriverState *bs, BlockDriverState *backing_hd)
>  {
>  
> @@ -1494,9 +1456,18 @@ int bdrv_open(BlockDriverState **pbs, const char *filename,
>  
>      /* Open the image */
>      if (flags & BDRV_O_PROTOCOL) {
> -        ret = bdrv_file_open(bs, drv, &options, flags & ~BDRV_O_PROTOCOL,
> -                             &local_err);
> +        if (!drv->bdrv_file_open) {
> +            const char *filename;
> +            filename = qdict_get_try_str(options, "filename");
> +            ret = bdrv_open(&bs, filename, NULL, options,
> +                            flags & ~BDRV_O_PROTOCOL, drv, &local_err);
> +            options = NULL;
> +        } else {
> +            ret = bdrv_open_common(bs, NULL, options,
> +                                   flags & ~BDRV_O_PROTOCOL, drv, &local_err);
> +        }
>          if (!ret) {
> +            bs->growable = 1;
>              goto done;
>          } else if (bs->drv) {
>              goto close_and_fail;
> -- 
> 1.8.3.1
> 
> 
Reviewed-by: Benoit Canet <benoit@irqsave.net>

[Qemu-devel] [PATCH 7/9] block: Remove second bdrv_open() recursion

[Kevin Wolf]

This recursion was introduced in commit 505d7583 in order to allow
nesting image formats. It only ever takes effect when the user
explicitly specifies a driver name and that driver isn't suitable for
the protocol level.

We can check this earlier in bdrv_open() and if the explicitly
requested driver is a format driver, clear BDRV_O_PROTOCOL so that
another bs->file layer is opened.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block.c | 50 +++++++++++++++++++++++++-------------------------
 1 file changed, 25 insertions(+), 25 deletions(-)

diff --git a/block.c b/block.c
index 034656f..0dbc06c 100644
--- a/block.c
+++ b/block.c
@@ -1408,6 +1408,26 @@ int bdrv_open(BlockDriverState **pbs, const char *filename,
         return ret;
     }
 
+    /* Find the right image format driver */
+    drv = NULL;
+    drvname = qdict_get_try_str(options, "driver");
+    if (drvname) {
+        drv = bdrv_find_format(drvname);
+        qdict_del(options, "driver");
+        if (!drv) {
+            error_setg(errp, "Unknown driver: '%s'", drvname);
+            ret = -EINVAL;
+            goto fail;
+        }
+    }
+
+    assert(drvname || !(flags & BDRV_O_PROTOCOL));
+    if (drv && !drv->bdrv_file_open) {
+        /* If the user explicitly wants a format driver here, we'll need to add
+         * another layer for the protocol in bs->file */
+        flags &= ~BDRV_O_PROTOCOL;
+    }
+
     bs->options = options;
     options = qdict_clone_shallow(options);
 
@@ -1430,25 +1450,13 @@ int bdrv_open(BlockDriverState **pbs, const char *filename,
         }
     }
 
-    /* Find the right image format driver */
-    drv = NULL;
-    drvname = qdict_get_try_str(options, "driver");
-    assert(drvname || !(flags & BDRV_O_PROTOCOL));
-
-    if (drvname) {
-        drv = bdrv_find_format(drvname);
-        qdict_del(options, "driver");
-        if (!drv) {
-            error_setg(errp, "Unknown driver: '%s'", drvname);
-            ret = -EINVAL;
-            goto fail;
-        }
-    } else if (file) {
+    /* Image format probing */
+    if (!drv && file) {
         ret = find_image_format(file, filename, &drv, &local_err);
         if (ret < 0) {
             goto fail;
         }
-    } else {
+    } else if (!drv) {
         error_setg(errp, "Must specify either driver or file");
         ret = -EINVAL;
         goto fail;
@@ -1456,16 +1464,8 @@ int bdrv_open(BlockDriverState **pbs, const char *filename,
 
     /* Open the image */
     if (flags & BDRV_O_PROTOCOL) {
-        if (!drv->bdrv_file_open) {
-            const char *filename;
-            filename = qdict_get_try_str(options, "filename");
-            ret = bdrv_open(&bs, filename, NULL, options,
-                            flags & ~BDRV_O_PROTOCOL, drv, &local_err);
-            options = NULL;
-        } else {
-            ret = bdrv_open_common(bs, NULL, options,
-                                   flags & ~BDRV_O_PROTOCOL, drv, &local_err);
-        }
+        ret = bdrv_open_common(bs, NULL, options,
+                               flags & ~BDRV_O_PROTOCOL, drv, &local_err);
         if (!ret) {
             bs->growable = 1;
             goto done;
-- 
1.8.3.1

[Qemu-devel] [PATCH 8/9] block: Catch backing files assigned to non-COW drivers

[Kevin Wolf]

Since we parse backing.* options to add a backing file from the command
line when the driver didn't assign one, it has been possible to have a
backing file for e.g. raw images (it just was never accessed).

This is obvious nonsense and should be rejected.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block.c                    | 7 +++++++
 block/cow.c                | 1 +
 block/qcow.c               | 1 +
 block/qcow2.c              | 1 +
 block/qed.c                | 1 +
 block/vmdk.c               | 1 +
 include/block/block_int.h  | 3 +++
 tests/qemu-iotests/051     | 5 +++++
 tests/qemu-iotests/051.out | 9 +++++++++
 9 files changed, 29 insertions(+)

diff --git a/block.c b/block.c
index 0dbc06c..f27f61e 100644
--- a/block.c
+++ b/block.c
@@ -1193,6 +1193,13 @@ int bdrv_open_backing_file(BlockDriverState *bs, QDict *options, Error **errp)
         bdrv_get_full_backing_filename(bs, backing_filename, PATH_MAX);
     }
 
+    if (!bs->drv || !bs->drv->supports_backing) {
+        ret = -EINVAL;
+        error_setg(errp, "Driver doesn't support backing files");
+        QDECREF(options);
+        goto free_exit;
+    }
+
     backing_hd = bdrv_new("", errp);
 
     if (bs->backing_format[0] != '\0') {
diff --git a/block/cow.c b/block/cow.c
index 164759f..43d509c 100644
--- a/block/cow.c
+++ b/block/cow.c
@@ -416,6 +416,7 @@ static BlockDriver bdrv_cow = {
     .bdrv_close     = cow_close,
     .bdrv_create    = cow_create,
     .bdrv_has_zero_init     = bdrv_has_zero_init_1,
+    .supports_backing       = true,
 
     .bdrv_read              = cow_co_read,
     .bdrv_write             = cow_co_write,
diff --git a/block/qcow.c b/block/qcow.c
index 7fd57d7..23f5498 100644
--- a/block/qcow.c
+++ b/block/qcow.c
@@ -941,6 +941,7 @@ static BlockDriver bdrv_qcow = {
     .bdrv_reopen_prepare = qcow_reopen_prepare,
     .bdrv_create	= qcow_create,
     .bdrv_has_zero_init     = bdrv_has_zero_init_1,
+    .supports_backing       = true,
 
     .bdrv_co_readv          = qcow_co_readv,
     .bdrv_co_writev         = qcow_co_writev,
diff --git a/block/qcow2.c b/block/qcow2.c
index a54d2ba..10b83c7 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -2401,6 +2401,7 @@ static BlockDriver bdrv_qcow2 = {
     .bdrv_save_vmstate    = qcow2_save_vmstate,
     .bdrv_load_vmstate    = qcow2_load_vmstate,
 
+    .supports_backing           = true,
     .bdrv_change_backing_file   = qcow2_change_backing_file,
 
     .bdrv_refresh_limits        = qcow2_refresh_limits,
diff --git a/block/qed.c b/block/qed.c
index 79f5bd3..b646ee8 100644
--- a/block/qed.c
+++ b/block/qed.c
@@ -1646,6 +1646,7 @@ static BlockDriver bdrv_qed = {
     .format_name              = "qed",
     .instance_size            = sizeof(BDRVQEDState),
     .create_options           = qed_create_options,
+    .supports_backing         = true,
 
     .bdrv_probe               = bdrv_qed_probe,
     .bdrv_rebind              = bdrv_qed_rebind,
diff --git a/block/vmdk.c b/block/vmdk.c
index b8a4762..360b73e 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -2177,6 +2177,7 @@ static BlockDriver bdrv_vmdk = {
     .bdrv_detach_aio_context      = vmdk_detach_aio_context,
     .bdrv_attach_aio_context      = vmdk_attach_aio_context,
 
+    .supports_backing             = true,
     .create_options               = vmdk_create_options,
 };
 
diff --git a/include/block/block_int.h b/include/block/block_int.h
index 8d58334..461ff6a 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -100,6 +100,9 @@ struct BlockDriver {
      */
     bool bdrv_needs_filename;
 
+    /* Set if a driver can support backing files */
+    bool supports_backing;
+
     /* For handling image reopen for split or non-split files */
     int (*bdrv_reopen_prepare)(BDRVReopenState *reopen_state,
                                BlockReopenQueue *queue, Error **errp);
diff --git a/tests/qemu-iotests/051 b/tests/qemu-iotests/051
index 30a712f..a41334e 100755
--- a/tests/qemu-iotests/051
+++ b/tests/qemu-iotests/051
@@ -100,6 +100,11 @@ echo
 
 echo "info block" | run_qemu -drive file="$TEST_IMG",driver=qcow2,backing.file.filename="$TEST_IMG.orig" -nodefaults
 
+# Drivers that don't support backing files
+run_qemu -drive file="$TEST_IMG",driver=raw,backing.file.filename="$TEST_IMG.orig"
+run_qemu -drive file="$TEST_IMG",file.backing.driver=file,file.backing.filename="$TEST_IMG.orig"
+run_qemu -drive file="$TEST_IMG",file.backing.driver=qcow2,file.backing.file.filename="$TEST_IMG.orig"
+
 echo
 echo === Enable and disable lazy refcounting on the command line, plus some invalid values ===
 echo
diff --git a/tests/qemu-iotests/051.out b/tests/qemu-iotests/051.out
index 37cb049..791956f 100644
--- a/tests/qemu-iotests/051.out
+++ b/tests/qemu-iotests/051.out
@@ -54,6 +54,15 @@ ide0-hd0: TEST_DIR/t.qcow2 (qcow2)
     Backing file:     TEST_DIR/t.qcow2.orig (chain depth: 1)
 (qemu) q^[[K^[[Dqu^[[K^[[D^[[Dqui^[[K^[[D^[[D^[[Dquit^[[K
 
+Testing: -drive file=TEST_DIR/t.qcow2,driver=raw,backing.file.filename=TEST_DIR/t.qcow2.orig
+QEMU_PROG: -drive file=TEST_DIR/t.qcow2,driver=raw,backing.file.filename=TEST_DIR/t.qcow2.orig: could not open disk image TEST_DIR/t.qcow2: Driver doesn't support backing files
+
+Testing: -drive file=TEST_DIR/t.qcow2,file.backing.driver=file,file.backing.filename=TEST_DIR/t.qcow2.orig
+QEMU_PROG: -drive file=TEST_DIR/t.qcow2,file.backing.driver=file,file.backing.filename=TEST_DIR/t.qcow2.orig: could not open disk image TEST_DIR/t.qcow2: Driver doesn't support backing files
+
+Testing: -drive file=TEST_DIR/t.qcow2,file.backing.driver=qcow2,file.backing.file.filename=TEST_DIR/t.qcow2.orig
+QEMU_PROG: -drive file=TEST_DIR/t.qcow2,file.backing.driver=qcow2,file.backing.file.filename=TEST_DIR/t.qcow2.orig: could not open disk image TEST_DIR/t.qcow2: Driver doesn't support backing files
+
 
 === Enable and disable lazy refcounting on the command line, plus some invalid values ===
 
-- 
1.8.3.1

[Qemu-devel] [PATCH 9/9] block: Remove a special case for protocols

[Kevin Wolf]

The only semantic change is that bs->open_flags gets BDRV_O_PROTOCOL set
now. This isn't useful, but it doesn't hurt either. The code that was
previously skipped by 'goto done' is automatically disabled because
protocol drivers don't support backing files (and if they did, this
would probably be a fix) and can't have snapshot_flags set.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block.c | 18 ++----------------
 1 file changed, 2 insertions(+), 16 deletions(-)

diff --git a/block.c b/block.c
index f27f61e..cfeca8b 100644
--- a/block.c
+++ b/block.c
@@ -832,7 +832,7 @@ static int bdrv_open_flags(BlockDriverState *bs, int flags)
      * Clear flags that are internal to the block layer before opening the
      * image.
      */
-    open_flags &= ~(BDRV_O_SNAPSHOT | BDRV_O_NO_BACKING);
+    open_flags &= ~(BDRV_O_SNAPSHOT | BDRV_O_NO_BACKING | BDRV_O_PROTOCOL);
 
     /*
      * Snapshots should be writable.
@@ -929,6 +929,7 @@ static int bdrv_open_common(BlockDriverState *bs, BlockDriverState *file,
     bs->zero_beyond_eof = true;
     open_flags = bdrv_open_flags(bs, flags);
     bs->read_only = !(open_flags & BDRV_O_RDWR);
+    bs->growable = !!(flags & BDRV_O_PROTOCOL);
 
     if (use_bdrv_whitelist && !bdrv_is_whitelisted(drv, bs->read_only)) {
         error_setg(errp,
@@ -1470,19 +1471,6 @@ int bdrv_open(BlockDriverState **pbs, const char *filename,
     }
 
     /* Open the image */
-    if (flags & BDRV_O_PROTOCOL) {
-        ret = bdrv_open_common(bs, NULL, options,
-                               flags & ~BDRV_O_PROTOCOL, drv, &local_err);
-        if (!ret) {
-            bs->growable = 1;
-            goto done;
-        } else if (bs->drv) {
-            goto close_and_fail;
-        } else {
-            goto fail;
-        }
-    }
-
     ret = bdrv_open_common(bs, file, options, flags, drv, &local_err);
     if (ret < 0) {
         goto fail;
@@ -1514,8 +1502,6 @@ int bdrv_open(BlockDriverState **pbs, const char *filename,
         }
     }
 
-
-done:
     /* Check if any unknown options were used */
     if (options && (qdict_size(options) != 0)) {
         const QDictEntry *entry = qdict_first(options);
-- 
1.8.3.1