[PATCH] xfree86: Add option parsing for percent options.
Peter Hutterer
peter.hutterer at who-t.net
Thu May 13 16:34:36 PDT 2010
On Thu, May 13, 2010 at 04:12:34PM -0700, Dan Nicholson wrote:
> 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.
oh, right. the main reason is that I expect some options that take percent
also to take absolute values. markUsed is set when you use the
xf86SetPercentOption(), which is the preferred one.
If you use xf86CheckPercentOption() just to check if the value is there,
then the following construct is possible (from an upcoming synaptics patch):
percent = xf86CheckPercentOption(opts, "AreaTopEdge", -1);
if (percent != -1) {
percent = xf86SetPercentOption(opts, "AreaTopEdge", -1);
pars->area_top_edge = percent/100.0 * (priv->maxy - priv->miny) + priv->miny;
} else
pars->area_top_edge = xf86SetIntOption(opts, "AreaTopEdge", 0);
this checks for a percent option and takes if if present, otherwise treats
it as an integer number. Without the special handling, this would always pop
a warning into the logs, even if the value is valid.
(the xf86SetPercentOption() is there to make the option appear as selected
in the log)
the other option is to somehow coerce the xf86Set{Int|Float} options into
supporting % values as well and returning some magic flag if it is set.
Cheers,
Peter
More information about the xorg-devel
mailing list