[ovs-dev] [PATCH 2/4] ovs-vsctl: Prepare for more flexible database argument parsing.

Jesse Gross jesse at nicira.com
Mon Jun 28 22:39:46 UTC 2010


On Thu, Jun 24, 2010 at 11:41 AM, Ben Pfaff <blp at nicira.com> wrote:

> + *      - If 'valuep' is nonnull, optionally an operator followed by a
> value
> + *        string.  The allowed operators are the 'n_allowed' string in
> + *        'allowed_operators', or just "=" if 'n_allowed' is 0.  If
> + *        'operatorp' is nonnull, then the operator is stored into
> + *        '*operatorp'.  The value is stored as a malloc()'d string into
> + *        '*valuep', or NULl if no value is present in 'arg'.
>

The last letter of NULL isn't capitalized.


> -parse_column_key_value(const char *arg, const struct vsctl_table_class
> *table,
> -                       const struct ovsdb_idl_column **columnp,
> -                       char **keyp, char **valuep)
> +parse_column_key_value(const char *arg,
> +                       const struct vsctl_table_class *table,
> +                       const struct ovsdb_idl_column **columnp, char
> **keyp,
> +                       const char **operatorp,
> +                       const char **allowed_operators, size_t n_allowed,
> +                       char **valuep)
>

This function is getting a little unwieldy (8 arguments).


>     /* Parse value string. */
> -    if (*p == '=') {
> -        if (!valuep) {
> -            error = xasprintf("%s: value not accepted here", arg);
>

We no longer warn if there is an operator but no value?


> +        if (*p == '\0') {
> +            error = xasprintf("%s: missing value in argument", arg);
> +            goto error;
> +        }
>

The comment above the function says that if valuep is non-null, then there
is an optional operator and value but this errors out if they aren't
present.

Separately, I think the error message is not very descriptive.


> +        if (!best) {
> +            error = xasprintf("%s: missing operator", arg);


 Same comment here about the optional operator.  This may or may not be the
desired behavior but it doesn't seem very clear or consistent between the
different fields that are being parsed.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openvswitch.org/pipermail/ovs-dev/attachments/20100628/f5922094/attachment-0003.html>


More information about the dev mailing list