[PATCH v2] xfree86: Use xorg.conf.d directory for multiple config files

[Peter Hutterer]

On Wed, Dec 16, 2009 at 06:42:56AM -0800, Dan Nicholson wrote:
> >> @@ -1732,6 +1733,7 @@ if test "x$XORG" = xyes; then
> >>       AC_DEFINE(__XSERVERNAME__, "Xorg", [Name of X server])
> >>       AC_DEFINE_DIR(__XCONFIGFILE__, XF86CONFIGFILE, [Name of configuration file])
> >>       AC_DEFINE_DIR(XF86CONFIGFILE, XF86CONFIGFILE, [Name of configuration file])
> >> +     AC_DEFINE_DIR(__XCONFIGDIR__, XF86CONFIGDIR, [Name of configuration directory])
> >
> > nitpick: moduledir and libdir are both directories as well and and referred
> > to with "path", not with "name". Maybe this should be used here to? (e.g.
> > "Configuration directory path")
> 
> I originally had path, but that's not correct. This define (and
> __XCONFIGFILE__) only tell you what the basename is. I.e., xorg.conf
> or xorg.conf.d. moduledir is a path (/usr/lib/xorg/modules). I can
> change it if it's a big deal, but I think this is right.

tbh, I don't really care enough, so whatever you prefer is fine with me here.
 
> >> +                     continue;
> >> +             }
> >> +
> >> +             path = malloc(PATH_MAX + 1);
> >
> > unrelated, but - malloc? xalloc? we have both of them in use already though
> > xalloc hold the majority for now. eventually we should just pick one. same
> > for free/xfree.
> 
> Ha. I think I used malloc because the original code did. I can just
> make it use xalloc/xfree until some time that it's decided that the
> wrappers suck.

then use whatever the code around it does. This was an unrelated comment,
and a decision either way should affect the whole server in one go, it's not
crucial to this patch.

> >>  /*
> >>   * prototypes for public functions
> >>   */
> >> -extern _X_EXPORT const char *xf86openConfigFile (const char *, const char *,
> >> -                                     const char *);
> >> +extern _X_EXPORT const XF86ConfPathsPtr
> >> +xf86openConfigFile(const char *filepath, const char *dirpath,
> >> +                const char *cmdline, const char *projroot);
> >
> >
> > s/_X_EXPORT// should work here.
> 
> Yeah, I forgot about that, but all the ones below this should be, too.
> I think it would be better to unexport all these functions in a
> separate patch.

yes, but meanwhile we might as well avoid adding more symbols to the ABI.
unless you've got any plans to writing and committing that patch in the near
future (before Dec 31), I'd say leave it without _X_EXPORT.
still, your call, this is not a dealbreaker.

> > tested, works. I'd be happy to see this patch with these last few changes
> > merged.
> 
> Strange to say this since I just submitted this patch, but I think it
> might be better to make xf86openConfigFile two functions in light
> Daniel and Alan's discussion about having separate command lines for
> config file and config dir. xf86openConfigFile is a little overloaded
> now. I can refactor that now, or we can wait until that discussion
> gets more concrete. I didn't put a lot of thought into this until
> after I sent this.

at this point I only care about the warnOnce bug and the typo, once they're
done I'm happy to give my Reviewed-by tag and see it merged. the
xf86openConfigFile refacturing could go on top of this to avoid making
the patch too hard to review, it probably makes the patch easier to write
for you as well.
 
Cheers,
  Peter