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

[Ben Pfaff]

On Mon, Jun 28, 2010 at 03:39:46PM -0700, Jesse Gross wrote:
> 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.

Thanks, fixed.

> 
> > -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).

Yes.  It's a bit of a screwball function.  Any idea of a good way to
break it up?

> >     /* 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?

Oops.  I put back that check now.

> > +        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.

Oops.  The comment was wrong.  If valuep is nonnull, then the operator
and value are required.

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

You're right.  I've improved it now.

> 
> > +        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.

I changed it so that all of these errors are reported the same way with
a message that clearly specifies what operators are allowed.