Re: [Qemu-devel] [PATCH v3 1/6] qemu-option: has_help_option() and is_valid_option_list()

[Eric Blake <eblake@redhat.com>]

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

On 02/21/2014 08:24 AM, Kevin Wolf wrote:
> has_help_option() checks if any help option ('help' or '?') occurs
> anywhere in an option string, so that things like 'cluster_size=4k,help'
> are recognised.
> 
> is_valid_option_list() ensures that the option list doesn't have options
> with leading commas or trailing unescaped commas.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---

> +
> +    while (*p) {
> +        p = get_opt_value(buf, buflen, p);
> +        if (*p) {
> +            p++;
> +        }
> +
> +        if (is_help_option(buf)) {
> +            result = true;
> +            goto out;

If this were 'break;',

> +        }
> +    }
> +
> +out:

then you wouldn't need this label.  But that's cosmetic.

> +    free(buf);
> +    return result;
> +}
> +
> +bool is_valid_option_list(const char *param)
> +{
> +    size_t buflen = strlen(param) + 1;
> +    char *buf = g_malloc0(buflen);
> +    const char *p = param;
> +    bool result = true;
> +
> +    while (*p) {
> +        p = get_opt_value(buf, buflen, p);
> +        if (*p && !*++p) {
> +            result = false;
> +            goto out;
> +        }

Rejects trailing commas.

> +
> +        if (!*buf || *buf == ',') {

Rejects empty options, but also rejects values beginning with a comma.
But we have legacy users that accept implicitly named first options (see
opts_do_parse()).  For example, this is a valid command line (albeit one
that prints a list of valid machines):

qemu-kvm -machine ,,blah

as shorthand for

qemu-kvm -machine type=,,blah

and where *buf would indeed be validly ','.

> +            result = false;
> +            goto out;
> +        }
> +    }
> +
> +out:

Same observation about the possibility of using break to avoid goto.

> +    free(buf);
> +    return result;
> +}

That said, none of our users of -o options have an implicitly-named
first option, and therefore THIS patch is safe, even if we have a
ticking time-bomb if any other option with implicit first name starts
using this function.  And break vs. goto is cosmetic.  So I'm okay
taking this as-is.

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 --]