[PATCH] xfree86: Add option parsing for percent options.
Peter Hutterer
peter.hutterer at who-t.net
Thu May 13 15:39:20 PDT 2010
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
More information about the xorg-devel
mailing list