[PATCH] xfree86: Allow a config directory for multiple config files
Dan Nicholson
dbn.lists at gmail.com
Tue Dec 1 06:10:58 PST 2009
On Mon, Nov 30, 2009 at 6:14 PM, Peter Hutterer
<peter.hutterer at who-t.net> wrote:
> On Fri, Nov 27, 2009 at 09:34:00AM -0800, Dan Nicholson wrote:
>> Currently there is a single file, xorg.conf, for configuring the server.
>> This works fine most of the time, but it becomes a problem when packages
>> or system services need to adjust the configuration. Instead, allow
>> multiple configuration files to live in a directory. Typically this will
>> be /etc/X11/xorg.conf.d.
>>
>> Right now this uses the same matching templates as the config file but
>> with a different default name. The matching code was refactored a bit to
>> make this more coherent. It also won't fall back to using the auto
>> configuration unless both the config file and config directory don't
>> exist.
>
> Thanks for this patch, much appreciated! I think you should send the next
> version of the patch with 'xorg.conf.d' in the subject, I'm not the only one
> who missed this one :)
Another one of those thoughts that comes to you after you've sent the
patch away. :)
>
>> Signed-off-by: Dan Nicholson <dbn.lists at gmail.com>
>> ---
>> hw/xfree86/common/xf86Config.c | 29 +++--
>> hw/xfree86/parser/scan.c | 229 +++++++++++++++++++++++++++-------------
>> hw/xfree86/parser/xf86Parser.h | 12 ++-
>> hw/xwin/winconfig.c | 34 ++++--
>> 4 files changed, 208 insertions(+), 96 deletions(-)
>>
>> +
>> +static char *
>> +DoConfigDir(const char *path, const char *projroot, const char *confname)
>
> rename to 'OpenConfigDir' or 'ScanConfigDir' maybe? something slightly less
> generic than "do", anyway. (same with the DoConfigFile)
OK.
>> +{
>> + char *dirpath, *pathcopy;
>> + const char *template;
>> + DIR *dir = NULL;
>> +
>> + pathcopy = strdup(path);
>> + template = strtok(pathcopy, ",");
>> + while (template && !dir) {
>> + if ((dirpath = DoSubstitution(template, NULL, projroot,
>> + NULL, NULL, confname))) {
>> + if ((dir = opendir(dirpath))) {
>> + struct dirent *de;
>> + char *name, *path;
>> + size_t len;
>> +
>> + /* match files named *.conf */
>> + while ((de = readdir(dir))) {
>> + name = de->d_name;
>> + len = strlen(name);
>> + if (name[0] == '.')
>> + continue;
>> + if (len <= 5)
>> + continue;
>> + if (strcmp(&name[len-5], ".conf") != 0)
>> + continue;
>> + path = malloc(PATH_MAX + 1);
>> + snprintf(path, PATH_MAX + 1, "%s/%s",
>> + dirpath, name);
>> + configFiles[numFiles].file =
>> + fopen(path, "r");
>> + if (configFiles[numFiles].file)
>> + configFiles[numFiles++].path =
>> + path;
>> + else
>> + free(path);
>> + if (numFiles >= CONFIG_MAX_FILES) {
>> + ErrorF("Maximum number of "
>> + "configuration files "
>> + "opened\n");
>> + break;
>> + }
>
> can this be split out into another fuction? the forced line breaks make the
> code rather hard to read.
Yeah, that loop is nasty. I reworked it as a for loop and it's cleaner now.
> AFAICT, readdir doesn't guarantee the order of entries returned which would
> make the usual 10-something.conf, 20-somethingelse.conf setup unpredictable.
> using scandir may be the better way to go, it also offloads the filtering
> code making the lot a bit easier to read.
>
> the HAL code may be a good source for this since it does exactly the same.
> http://cgit.freedesktop.org/hal/tree/hald/create_cache.c
> (copying that would also add recursive subdirectory support :)
I'd forgotten about that. Reworked with scandir now.
> likewise, HAL has support for a sysconfig and a user config dir - on Fedora
> that's /usr/share/hal and /etc/hal. default configurations shipped with
> drivers are dumped into the former, user-configurations into the latter. I
> think this is a good approach, having something similar would be nice.
This one I think will be harder to wedge into the current code. Might
conflict with the xorg.conf searching algorithm, but maybe you can
just throw another path at it. Let's think about that later.
>
>> + }
>> + }
>> + }
>> + if (dirpath && !dir) {
>> + free(dirpath);
>> + dirpath = NULL;
>> + }
>> + template = strtok(NULL, ",");
>> + }
>> +
>> + if (dir)
>> + closedir(dir);
>> + return dirpath;
>> +}
>> +
>> /*
>> * xf86openConfigFile --
>> *
>> diff --git a/hw/xfree86/parser/xf86Parser.h b/hw/xfree86/parser/xf86Parser.h
>> index b4837d5..16b841a 100644
>> --- a/hw/xfree86/parser/xf86Parser.h
>> +++ b/hw/xfree86/parser/xf86Parser.h
>> @@ -476,11 +476,19 @@ typedef struct
>> }
>> xf86ConfigSymTabRec, *xf86ConfigSymTabPtr;
>>
>> +typedef struct
>> +{
>> + char *file; /* config file */
>> + char *dir; /* multiconf directory */
>> +}
>> +XF86ConfPathsRec, *XF86ConfPathsPtr;
>> +
>> /*
>> * prototypes for public functions
>> */
>> -extern _X_EXPORT const char *xf86openConfigFile (const char *, const char *,
>> - const char *);
>> +extern _X_EXPORT const XF86ConfPathsPtr xf86openConfigFile(const char *path,
>> + const char *cmdline,
>> + const char *projroot);
>
> is this really supposed to be public? I think this was one of the many made
> public in 49f77fff1495c0a2050fb18f9b1fc627839bbfc2.
I don't think so, but since xf86openConfigFile was... I also don't
think I entirely understand the _X_EXPORT/_X_INTERNAL/_X_HIDDEN rules,
but this should only be used by the server.
>> extern _X_EXPORT void xf86setBuiltinConfig(const char *config[]);
>> extern _X_EXPORT XF86ConfigPtr xf86readConfigFile (void);
>> extern _X_EXPORT void xf86closeConfigFile (void);
>> diff --git a/hw/xwin/winconfig.c b/hw/xwin/winconfig.c
>> index 3e1908c..63ead08 100644
>> --- a/hw/xwin/winconfig.c
>> +++ b/hw/xwin/winconfig.c
>> @@ -109,7 +109,9 @@ Bool
>> winReadConfigfile ()
>> {
>> Bool retval = TRUE;
>> + XF86ConfPathsPtr paths;
>> const char *filename;
>> + const char *dirname;
>> MessageType from = X_DEFAULT;
>> char *xf86ConfigFile = NULL;
>>
>> @@ -121,24 +123,36 @@ winReadConfigfile ()
>>
>> /* Parse config file into data structure */
>>
>> - filename = xf86openConfigFile (CONFIGPATH, xf86ConfigFile, PROJECTROOT);
>> -
>> + paths = xf86openConfigFile (CONFIGPATH, xf86ConfigFile, PROJECTROOT);
>> +
>> /* Hack for backward compatibility */
>> - if (!filename && from == X_DEFAULT)
>> - filename = xf86openConfigFile (CONFIGPATH, "XF86Config", PROJECTROOT);
>> + if (!(paths && paths->file) && from == X_DEFAULT)
>> + paths = xf86openConfigFile (CONFIGPATH, "XF86Config", PROJECTROOT);
>>
>> - if (filename)
>> + if (paths && (paths->file || paths->dir))
>> {
>> - winMsg (from, "Using config file: \"%s\"\n", filename);
>> + if (paths && paths->file)
>> + {
>> + winMsg (from, "Using config file: \"%s\"\n", paths->file);
>> + }
>
> no { } for single-line blocks. the context seems to use spaces, not tabs.
>
>> + else
>> + {
>> + winMsg (X_ERROR, "Unable to locate/open config file");
>> + if (xf86ConfigFile)
>> + ErrorF (": \"%s\"", xf86ConfigFile);
>> + ErrorF ("\n");
>> + }
>> +
>> + if (paths && paths->dir)
>> + {
>> + winMsg (X_DEFAULT, "Using config directory \"%s\"\n", paths->dir);
>> + }
>
> no { } for single-line blocks. the context seems to use spaces, not tabs.
For both these I tried to stay consistent with the surrounding style
(which does use spaces, but also brackets around single lines). I'll
make it right.
>
>> }
>> else
>> {
>> - winMsg (X_ERROR, "Unable to locate/open config file");
>> - if (xf86ConfigFile)
>> - ErrorF (": \"%s\"", xf86ConfigFile);
>> - ErrorF ("\n");
>> return FALSE;
>> }
>> +
>> if ((g_xf86configptr = xf86readConfigFile ()) == NULL)
>> {
>> winMsg (X_ERROR, "Problem parsing the config file\n");
>> --
>> 1.6.2.5
>
> The server segfaults if there's no xorg.conf and an empty xorg.conf.d
> directory. From a quick glance it looks like it's returning a valid
> configuration but then passes out when trying to read the first token from
> it. That case should probably be handled better :)
Oops. I didn't go to the effort of actually making sure the directory
had valid .conf files in it before declaring success. The case with no
xorg.conf and no/empty xorg.conf.d needs to be handled a little
specially. I'll test that again, but it should be fixed now.
> Other than that, quick testing shows it works.
>
> Can you explain the interaction between xorg.conf and xorg.conf.d though?
> if I read the code correctly, the xorg.conf would always be the first
> section in the config file, thus takes precedence where the config file
> grammar requires this. can you confirm that?
> that should definitely go into the man page.
Yes, that's the intention. 2 things drove that decision.
1. With AllowEmptyInput, the first InputDevice is used for the core
devices (right?). I wanted to make sure that would remain as the first
one in xorg.conf.
2. For InputClass, earlier sections (or backend supplied options) take
precedence, so I again wanted to have xorg.conf trump xorg.conf.d.
Basically, I see it where packages dump config files in xorg.conf.d
and then the admin edits xorg.conf if necessary. We can certainly
discuss that more.
I'll resend the patch with your suggestions (and a new subject :))
after I give it a whirl tonight. Thanks for the feedback!
--
Dan
More information about the xorg-devel
mailing list