Re: [Qemu-devel] [PATCH for-2.9 3/5] rbd: Rewrite the code to extract list-valued options

[Eric Blake]

On 03/23/2017 05:55 AM, Markus Armbruster wrote:
> We have two list-values options:
> 
> * "server" is a list of InetSocketAddress.  We use members "host" and
>   "port", and silently ignore the rest.
> 
> * "auth-supported" is a list of RbdAuthMethod.  We use its only member
>   "auth".
> 
> Since qemu_rbd_open() takes options as a flattened QDict, options has
> keys of the form server.%d.host, server.%d.port and
> auth-supported.%d.auth, where %d counts up from zero.
> 
> qemu_rbd_array_opts() extracts these values as follows.  First, it
> calls qdict_array_entries() to find the list's length.  For each list
> element, it first formats the list's key prefix (e.g. "server.0."),
> then creates a new QDict holding the options with that key prefix,
> then converts that to a QemuOpts, so it can finally get the member
> values from there.
> 
> If there's one surefire way to make code using QDict more awkward,
> it's creating more of them and mixing in QemuOpts for good measure.

No kidding!

> 
> The conversion to QemuOpts abuses runtime_opts, as described in the
> commit before previous.
> 
> Rewrite to simply get the values straight from the options QDict.
> This removes the abuse of runtime_opts, so clean it up.
> 
> Signed-off-by: Markus Armbruster <address@hidden>
> ---
>  block/rbd.c | 151 
> +++++++++++++++++-------------------------------------------
>  1 file changed, 42 insertions(+), 109 deletions(-)
> 
> diff --git a/block/rbd.c b/block/rbd.c
> index 59c822a..8ba0f79 100644
> --- a/block/rbd.c
> +++ b/block/rbd.c
> @@ -13,6 +13,7 @@
>  
>  #include "qemu/osdep.h"
>  
> +#include <rbd/librbd.h>
>  #include "qapi/error.h"
>  #include "qemu/error-report.h"
>  #include "block/block_int.h"
> @@ -20,8 +21,6 @@
>  #include "qemu/cutils.h"
>  #include "qapi/qmp/qstring.h"
>  
> -#include <rbd/librbd.h>
> -

Not mentioned in the commit message, but also a useful cleanup for
hoisting <includes> prior to "includes".

> +static char *rbd_auth(QDict *options)
>  {
> -    int num_entries;
> -    QemuOpts *opts = NULL;
> -    QDict *sub_options;
> -    const char *host;
> -    const char *port;
> -    char *str;
> -    char *rados_str = NULL;
> -    Error *local_err = NULL;
> +    const char **vals = g_new(const char *, qdict_size(options));
> +    char keybuf[32];
> +    QObject *val;
> +    char *rados_str;
>      int i;
>  
> -    assert(type == RBD_MON_HOST || type == RBD_AUTH_SUPPORTED);
> -
> -    num_entries = qdict_array_entries(options, prefix);
> +    for (i = 0;; i++) {
> +        sprintf(keybuf, "auth-supported.%d.auth", i);

By my count, and including a trailing NUL, this is 21 bytes + the
maximum size of a formatted int to fit in keybuf[32]; 32-bit INT_MIN is
indeed 11 bytes.  Cutting it close there, but I don't see an overflow
(if gcc 7's new -Wformat-truncation spots something, then gcc is too
strict.)

> +static char *rbd_mon_host(QDict *options)
> +{
> +    const char **vals = g_new(const char *, qdict_size(options));
> +    char keybuf[32];
> +    QObject *val;
> +    const char *host, *port;
> +    char *rados_str;
> +    int i;
>  
> -            value = host;
> -            if (port) {
> -                /* check for ipv6 */
> -                if (strchr(host, ':')) {
> -                    strbuf = g_strdup_printf("[%s]:%s", host, port);
> -                } else {
> -                    strbuf = g_strdup_printf("%s:%s", host, port);

The old code only prints port information if it is present...

> -                }
> -                value = strbuf;
> -            } else if (strchr(host, ':')) {
> -                strbuf = g_strdup_printf("[%s]", host);
> -                value = strbuf;
> -            }
> -        } else {
> -            value = qemu_opt_get(opts, "auth");
> +    for (i = 0;; i++) {
> +        sprintf(keybuf, "server.%d.host", i);

Here, you've got more breathing room.

> +        val = qdict_get(options, keybuf);
> +        if (!val) {
> +            break;
>          }
> +        host = qstring_get_str(qobject_to_qstring(val));
> +        sprintf(keybuf, "server.%d.port", i);
> +        port = qdict_get_str(options, keybuf);
>  
> -
> -        /* each iteration in the for loop will build upon the string, and if
> -         * rados_str is NULL then it is our first pass */
> -        if (rados_str) {
> -            /* separate options with ';', as that  is what rados_conf_set()
> -             * requires */
> -            rados_str_tmp = rados_str;
> -            rados_str = g_strdup_printf("%s;%s", rados_str_tmp, value);
> -            g_free(rados_str_tmp);
> +        if (strchr(host, ':')) {
> +            vals[i] = g_strdup_printf("[%s]:%s", host, port);
>          } else {
> -            rados_str = g_strdup(value);
> +            vals[i] = g_strdup_printf("%s:%s", host, port);

...but the new code unconditionally prints port information, even when
port == NULL.  Oops.


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

signature.asc
Description: OpenPGP digital signature