Re: [Qemu-devel] [PATCH v2 3/5] block/rbd: parse all options via bdrv_parse_filename

[Eric Blake]

On 02/27/2017 12:58 PM, Jeff Cody wrote:
> Get rid of qemu_rbd_parsename in favor of bdrv_parse_filename.
> This simplifies a lot of the parsing as well, as we can treat everything
> a bit simpler since nonexistent options are simply NULL pointers instead
> of empy strings.

s/empy/empty/

> 
> An important item to note:
> 
> Ceph has many extra option values that can be specified as key/value
> pairs.  This was handled previously in the driver by extracting the
> values that the QEMU driver cared about, and then blindly passing all
> extra options to rbd after splitting them into key/value pairs, and
> cleaning up any special character escaping.
> 
> The practice is continued in this patch; there is an option
> "keyvalue-pairs" that is populated with all the key/value pairs that the
> QEMU driver does not care about.  These key/value pairs will override
> any settings in the 'conf' configuration file, just as they did before.
> 
> Signed-off-by: Jeff Cody <address@hidden>
> ---
>  block/rbd.c | 298 
> ++++++++++++++++++++++++++++++------------------------------
>  1 file changed, 148 insertions(+), 150 deletions(-)
> 

> +
> +    /* The following are essentially all key/value pairs, and we treat
> +     * 'id' and 'conf' a bit special.  Key/value pairs may be in any order. 
> */
> +    while (p) {

> +        if (!strcmp(name, "conf")) {
> +            qdict_put(options, "conf", qstring_from_str(value));
> +        } else if (!strcmp(name, "id")) {
> +            qdict_put(options, "user" , qstring_from_str(value));
> +        } else {
> +            char *tmp = g_malloc0(max_keypair_size);
> +            /* only use a delimiter if it is not the first keypair found */
> +            /* These are sets of unknown key/value pairs we'll pass along
> +             * to ceph */
> +            if (keypairs[0]) {
> +                snprintf(tmp, max_keypair_size, ":%s=%s", name, value);
> +                pstrcat(keypairs, max_keypair_size, tmp);
> +            } else {
> +                snprintf(keypairs, max_keypair_size, "%s=%s", name, value);
> +            }
> +            g_free(tmp);
> +        }
> +    }
> +
> +    if (keypairs[0]) {
> +        qdict_put(options, "keyvalue-pairs", qstring_from_str(keypairs));

Uggh.  Why are we compressing this into a single string, instead of
using a GList?  True, we aren't exposing it through QAPI, but I still
wonder if a smarter representation than a flat string is warranted.

> @@ -434,35 +421,55 @@ static int qemu_rbd_create(const char *filename, 
> QemuOpts *opts, Error **errp)
>      if (objsize) {
>          if ((objsize - 1) & objsize) {    /* not a power of 2? */

Drive-by comment (if you fix it, do it as a separate followup patch): we
have is_power_of_2() to make code like this more legible.

>  
>  static BlockDriver bdrv_rbd = {
> -    .format_name        = "rbd",
> -    .instance_size      = sizeof(BDRVRBDState),
> -    .bdrv_needs_filename = true,
> -    .bdrv_file_open     = qemu_rbd_open,
> -    .bdrv_close         = qemu_rbd_close,
> -    .bdrv_create        = qemu_rbd_create,
> -    .bdrv_has_zero_init = bdrv_has_zero_init_1,
> -    .bdrv_get_info      = qemu_rbd_getinfo,
> -    .create_opts        = &qemu_rbd_create_opts,
> -    .bdrv_getlength     = qemu_rbd_getlength,
> -    .bdrv_truncate      = qemu_rbd_truncate,
> -    .protocol_name      = "rbd",
> +    .format_name            = "rbd",
> +    .instance_size          = sizeof(BDRVRBDState),
> +    .bdrv_parse_filename    = qemu_rbd_parse_filename,
> +    .bdrv_file_open         = qemu_rbd_open,
> +    .bdrv_close             = qemu_rbd_close,
> +    .bdrv_create            = qemu_rbd_create,
> +    .bdrv_has_zero_init     = bdrv_has_zero_init_1,
> +    .bdrv_get_info          = qemu_rbd_getinfo,
> +    .create_opts            = &qemu_rbd_create_opts,

Pointless &; might as well remove it for consistency while touching it.

> +    .bdrv_getlength         = qemu_rbd_getlength,
> +    .bdrv_truncate          = qemu_rbd_truncate,
> +    .protocol_name          = "rbd",
>  

I don't know if it is worth respinning to change keyvalue-pairs into a
more appropriate data type; given our desire to make blockdev-add stable
for 2.9 and the fact that keyvalue-pairs is not exposed to QAPI, I can
live with passing around a flat string.  You may want to add FIXME
comments to call attention to the fact that we know it is gross but why
we do it anyways.

But since adding comments, and fixing minor things like &, doesn't
change the real meat of this patch, I can live with:

Reviewed-by: Eric Blake <address@hidden>

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

signature.asc
Description: OpenPGP digital signature