Re: [Qemu-devel] [PATCH RFC v2 3/9] rbd: Don't limit length of parameter values

[Eric Blake]

On 03/24/2017 12:44 PM, Markus Armbruster wrote:
> We laboriously enforce parameter values are between one and some
> arbitrary limit in length.  Only RBD_MAX_IMAGE_NAME_SIZE comes from
> librbd.h, and I'm not sure it applies.  Where the other limits come
> from is unclear.
> 
> Drop the length checking.  The limits librbd actually imposes must be
> checked by librbd anyway.

And if librbd is NOT doing proper length checks, the bug lies there, not
in qemu.

> 
> There's one minor complication: BDRVRBDState member name is a
> fixed-size array.  Depends on the length limit.  Make it a pointer to
> a dynamically allocated string.
> 
> Signed-off-by: Markus Armbruster <address@hidden>
> ---
>  block/rbd.c | 91 
> ++++++++++---------------------------------------------------
>  1 file changed, 14 insertions(+), 77 deletions(-)

Wow - you weren't kidding about the checks being laborious.

> -static char *qemu_rbd_next_tok(int max_len,
> -                               char *src, char delim,
> -                               const char *name,
> -                               char **p, Error **errp)
> +static char *qemu_rbd_next_tok(char *src, char delim, char **p)

Getting rid of the forced length means we can't fail - nice.


> @@ -184,27 +161,15 @@ static void qemu_rbd_parse_filename(const char 
> *filename, QDict *options,
>      qdict_put(options, "pool", qstring_from_str(found_str));
>  
>      if (strchr(p, '@')) {
> -        found_str = qemu_rbd_next_tok(RBD_MAX_IMAGE_NAME_SIZE, p,
> -                                      '@', "object name", &p, &local_err);
> -        if (local_err) {
> -            goto done;
> -        }
> +        found_str = qemu_rbd_next_tok(p, '@', &p);
>          qemu_rbd_unescape(found_str);
>          qdict_put(options, "image", qstring_from_str(found_str));

Unrelated to your patch, and doesn't hold you up, but I'd love to
eventually get in my patch that shortens this pattern to:

qdict_put_str(options, "image", found_str);

(I still need to find time to write up a Coccinelle script to automate
the task; it's 2.10 material now)

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