[PATCH] xfree86: Add option parsing for percent options.

[Peter Hutterer]

On Thu, May 13, 2010 at 03:19:12PM -0700, Dan Nicholson wrote:
> On Wed, May 12, 2010 at 11:07 PM, Peter Hutterer
> <peter.hutterer at who-t.net> wrote:
> > In some cases, an option of "50%" would be preferable over fixed value
> > configuration - especially if the actual values are autoprobed.
> > Add a new set of functions to parse percent values from configurations.
> >
> > The percent value parsing differs slightly - if the option is not to marked
> > as used (e.g. xf86CheckPercentOption()), no warning is emitted to the log
> > file if the value is not a percent value. This allows double-options (either
> > as % or as absolute number) without warnings.
> >
> > Signed-off-by: Peter Hutterer <peter.hutterer at who-t.net>
> > ---
> > This would be quite useful in a few input drivers, especially now that we
> > have xorg.conf snippets. Best example here are the synaptics Area options,
> > where it'd be much easier to just specify the bottom area as 80% instead of
> > guessing what the touchpad may have.
> >
> > One question is of course if we should allow the values to be floats instead
> > of ints, not sure if we need something like 0.2% but I guess it might not
> > hurt, either.
> >
> >  hw/xfree86/common/xf86Opt.h    |    4 +++
> >  hw/xfree86/common/xf86Option.c |   48 ++++++++++++++++++++++++++++++++++++++++
> >  2 files changed, 52 insertions(+), 0 deletions(-)
> >
> > diff --git a/hw/xfree86/common/xf86Opt.h b/hw/xfree86/common/xf86Opt.h
> > index ce3d767..479c513 100644
> > --- a/hw/xfree86/common/xf86Opt.h
> > +++ b/hw/xfree86/common/xf86Opt.h
> > @@ -51,6 +51,7 @@ typedef enum {
> >     OPTV_ANYSTR,                /* Any string, including an empty one */
> >     OPTV_REAL,
> >     OPTV_BOOLEAN,
> > +    OPTV_PERCENT,
> >     OPTV_FREQ
> >  } OptionValueType;
> >
> > @@ -72,10 +73,12 @@ extern _X_EXPORT int xf86SetIntOption(pointer optlist, const char *name, int def
> >  extern _X_EXPORT double xf86SetRealOption(pointer optlist, const char *name, double deflt);
> >  extern _X_EXPORT char *xf86SetStrOption(pointer optlist, const char *name, char *deflt);
> >  extern _X_EXPORT int xf86SetBoolOption(pointer list, const char *name, int deflt );
> > +extern _X_EXPORT int xf86SetPercentOption(pointer list, const char *name, int deflt );
> >  extern _X_EXPORT int xf86CheckIntOption(pointer optlist, const char *name, int deflt);
> >  extern _X_EXPORT double xf86CheckRealOption(pointer optlist, const char *name, double deflt);
> >  extern _X_EXPORT char *xf86CheckStrOption(pointer optlist, const char *name, char *deflt);
> >  extern _X_EXPORT int xf86CheckBoolOption(pointer list, const char *name, int deflt );
> > +extern _X_EXPORT int xf86CheckPercentOption(pointer list, const char *name, int deflt );
> >  extern _X_EXPORT pointer xf86AddNewOption(pointer head, const char *name, const char *val );
> >  extern _X_EXPORT pointer xf86NewOption(char *name, char *value );
> >  extern _X_EXPORT pointer xf86NextOption(pointer list );
> > @@ -109,5 +112,6 @@ extern _X_EXPORT char *xf86NormalizeName(const char *s);
> >  extern _X_EXPORT pointer xf86ReplaceIntOption(pointer optlist,  const char *name, const int val);
> >  extern _X_EXPORT pointer xf86ReplaceRealOption(pointer optlist,  const char *name, const double val);
> >  extern _X_EXPORT pointer xf86ReplaceBoolOption(pointer optlist, const char *name, const Bool val);
> > +extern _X_EXPORT pointer xf86ReplacePercentOption(pointer optlist, const char *name, const int val);
> >  extern _X_EXPORT pointer xf86ReplaceStrOption(pointer optlist,  const char *name, const char* val);
> >  #endif
> > diff --git a/hw/xfree86/common/xf86Option.c b/hw/xfree86/common/xf86Option.c
> > index a2868bf..ab86ace 100644
> > --- a/hw/xfree86/common/xf86Option.c
> > +++ b/hw/xfree86/common/xf86Option.c
> > @@ -223,6 +223,18 @@ LookupBoolOption(pointer optlist, const char *name, int deflt, Bool markUsed)
> >     return deflt;
> >  }
> >
> > +static int
> > +LookupPercentOption(pointer optlist, const char *name, int deflt, Bool markUsed)
> > +{
> > +    OptionInfoRec o;
> > +
> > +    o.name = name;
> > +    o.type = OPTV_PERCENT;
> > +    if (ParseOptionValue(-1, optlist, &o, markUsed))
> > +       deflt = o.value.num;
> > +    return deflt;
> > +}
> > +
> >  /* These xf86Set* functions are intended for use by non-screen specific code */
> >
> >  int
> > @@ -252,6 +264,12 @@ xf86SetBoolOption(pointer optlist, const char *name, int deflt)
> >     return LookupBoolOption(optlist, name, deflt, TRUE);
> >  }
> >
> > +int
> > +xf86SetPercentOption(pointer optlist, const char *name, int deflt)
> > +{
> > +    return LookupPercentOption(optlist, name, deflt, TRUE);
> > +}
> > +
> >  /*
> >  * These are like the Set*Option functions, but they don't mark the options
> >  * as used.
> > @@ -283,6 +301,12 @@ xf86CheckBoolOption(pointer optlist, const char *name, int deflt)
> >     return LookupBoolOption(optlist, name, deflt, FALSE);
> >  }
> >
> > +
> > +int
> > +xf86CheckPercentOption(pointer optlist, const char *name, int deflt)
> > +{
> > +    return LookupPercentOption(optlist, name, deflt, FALSE);
> > +}
> >  /*
> 
> I think you wanted a newline before the comment instead of two after
> the previous function. :)

will do.

> 
> >  * addNewOption() has the required property of replacing the option value
> >  * if the option is already present.
> > @@ -310,6 +334,14 @@ xf86ReplaceBoolOption(pointer optlist, const char *name, const Bool val)
> >  }
> >
> >  pointer
> > +xf86ReplacePercentOption(pointer optlist, const char *name, const int val)
> > +{
> > +    char tmp[16];
> > +    sprintf(tmp, "%d%%", val);
> > +    return xf86AddNewOption(optlist,name,tmp);
> > +}
> 
> Why is the buffer length 16? Seems you could limit to 5 by sanity
> checking 0 <= val <= 100. Oh, I guess when the new option is added it
> could be checked there.

copy/paste, I just copied from ReplaceIntOption which has the 16. See below
though for more.

> > +
> > +pointer
> >  xf86ReplaceStrOption(pointer optlist, const char *name, const char* val)
> >  {
> >       return xf86AddNewOption(optlist,name,val);
> > @@ -533,6 +565,22 @@ ParseOptionValue(int scrnIndex, pointer options, OptionInfoPtr p,
> >                p->found = FALSE;
> >            }
> >            break;
> > +       case OPTV_PERCENT:
> > +           {
> > +               char tmp[2];
> > +               /* awkward match, but %% doesn't increase the match counter,
> > +                * hence 100 looks the same as 100% to the caller of sccanf
> > +                */
> > +               if (sscanf(s, "%ld%1[%]", &p->value.num, tmp) != 2) {
> > +                   if (markUsed)
> > +                       xf86DrvMsg(scrnIndex, X_WARNING,
> > +                                  "Option \"%s\" requires a percent value\n", p->name);
> > +                   p->found = FALSE;
> 
> Why is the warning being suppressed with markUsed? Because we can't
> trust the formatting of the value or so that CheckPercent doesn't see
> a warning or something else?

yes, exactly. I added this to the commit message, but I guess it should be
in the code as well:

""
The percent value parsing differs slightly - if the option is not to marked
as used (e.g. xf86CheckPercentOption()), no warning is emitted to the log
file if the value is not a percent value. This allows double-options (either
as % or as absolute number) without warnings.
""

> 
> > +               } else {
> > +                   p->found = TRUE;
> 
> Would be nice to sanity check 0 <= val <= 100 here so that users didn't have to.

I think this should be done in the driver, because there are plenty of cases
where an option of 200% may be appropriate. Hence limiting this to a 0-100
range seems unnecessarily restrictive. Same with negative values, if we have
a generic mechanism like this in the server, -10% should be possible too.

that's also one argument for the tmp[16], 10000% may just be a valid value
somewhere? (well, even then 16 is too much, but..)

> Looks nice, though. Assuming the warning suppression is explained sufficiently,
> 
> Reviewed-by: Dan Nicholson <dbn.lists at gmail.com>

thanks, I'll amend and add Keith's comments.
 
Cheers,
  Peter