[PATCH] xfree86: Add option parsing for percent options.
Dan Nicholson
dbn.lists at gmail.com
Thu May 13 16:12:34 PDT 2010
On Thu, May 13, 2010 at 3:39 PM, Peter Hutterer
<peter.hutterer at who-t.net> wrote:
> 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.
> ""
I guess I meant I need a little more explanation. Why wouldn't we want
a properly formatted percentage for the value? Not to mention that
FALSE will be returned in that case and CheckPercent will end up using
the default passed in. None of the other option types have this
special handling. OPTV_FREQ seems like a good example to follow.
>
>>
>> > + } 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.
Duh. I'm thinking of grades or something. :) Obviously we want to make
arbitrary percentages possible.
> 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..)
Yeah, at first I thought this was a definite overflow happening, but
if you assume a max 32 bit integer, then there are max 10 decimal
digits. So, that part looks fine.
--
Dan
More information about the xorg-devel
mailing list