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

[Dan Nicholson]

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

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

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

> +               } else {
> +                   p->found = TRUE;

Would be nice to sanity check 0 <= val <= 100 here so that users didn't have to.

Looks nice, though. Assuming the warning suppression is explained sufficiently,

Reviewed-by: Dan Nicholson <dbn.lists at gmail.com>

--
Dan