[PATCH v4 3/3] xfree86: remove board and vendor identifier strings from the configuration path

[Dan Nicholson]

On Wed, Jun 30, 2010 at 6:07 AM, Vignatti Tiago (Nokia-D/Helsinki)
<tiago.vignatti at nokia.com> wrote:
> On Wed, Jun 30, 2010 at 01:37:10AM +0200, ext Keith Packard wrote:
>> On Tue, 29 Jun 2010 16:08:03 +0300, Tiago Vignatti <tiago.vignatti at nokia.com> wrote:
>>
>> > diff --git a/hw/xfree86/doc/man/xorg.conf.man.pre b/hw/xfree86/doc/man/xorg.conf.man.pre
>> > index 6b3636f..3443e9b 100644
>> > --- a/hw/xfree86/doc/man/xorg.conf.man.pre
>> > +++ b/hw/xfree86/doc/man/xorg.conf.man.pre
>> > @@ -1455,9 +1455,6 @@ The entries that may be used in
>> >  .B Monitor
>> >  sections are described below.
>> >  .TP 7
>> > -.BI "VendorName  \*q" vendor \*q
>> > -This optional entry specifies the monitor's manufacturer.
>> > -.TP 7
>> >  .BI "ModelName  \*q" model \*q
>> >  This optional entry specifies the monitor's model.
>> >  .TP 7
>>
>> You appear to be patching the monitor section instead of the device
>> section here.
>
> Arghh, that's the problem to code when we have hangover, without thinking much
> (You know, World Cup is running, Brazil is doing well, summer time in Finland
> - yes exists! - , etc).
>
> Anyway, I gonna fix this and send another version of the patch.
>
>
>> > diff --git a/hw/xfree86/parser/Device.c b/hw/xfree86/parser/Device.c
>> > index d71abc6..2675686 100644
>> > --- a/hw/xfree86/parser/Device.c
>> > +++ b/hw/xfree86/parser/Device.c
>> > @@ -123,14 +123,9 @@ xf86parseDeviceSection (void)
>> >                     has_ident = TRUE;
>> >                     break;
>> >             case VENDOR:
>> > -                   if (xf86getSubToken (&(ptr->dev_comment)) != STRING)
>> > -                           Error (QUOTE_MSG, "Vendor");
>> > -                   ptr->dev_vendor = val.str;
>> > -                   break;
>> >             case BOARD:
>> > -                   if (xf86getSubToken (&(ptr->dev_comment)) != STRING)
>> > -                           Error (QUOTE_MSG, "Board");
>> > -                   ptr->dev_board = val.str;
>> > +                   xf86parseError (OBSOLETE_MSG, xf86tokenString());
>> > +                   xf86getSubToken (&(ptr->dev_comment));
>> >                     break;
>>
>> Just FYI -- I think there is a memory leak here as you're parsing and
>> then not freeing the string. Not a big deal as it's in the config file
>> stuff.
>
> I'll take a look on it. I also seeing other minor defects inside the parser
> code (bad free and some deadcode), and I'll be probably take a look on those
> as well.

There are lots of parser leaks, so I wouldn't lose sleep over this. A
relatively easy project would be for someone to just go through
hw/xfree86/parser and clean up crappy code.

>> Question to the crowd -- any opinion on whether this change has become
>> too large for inclusion in 1.9? It isn't adding any code, and only
>> changes the ABI in removing some useless struct fields.
>
> I'd say to push the first and second of this set to 1.9, given their
> importance. This third one concerns more clean up, so I'll work a bit more,
> squash to my tree and eventually we can push later when the merge window is
> opened.

I don't see any reason to introduce an ABI break for 1.9. We can still
get the benefit of not calling the expensive pciaccess functions
without causing any superfluous rebuilds. If you really wanted to, you
could split the parser/Device.c warnings into a separate commit so at
least 1.9 users know that those fields do nothing anymore.

--
Dan