[PATCH] xfree86: Allow a config directory for multiple config files

[Peter Hutterer]

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 :)

> 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)


> +{
> +	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.

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 :)

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.

> +				}
> +			}
> +		}
> +		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.

>  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.

>      }
>    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 :)

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.

Cheers,
  Peter