[PATCH] config-parser: Honor the XDG_CONFIG_DIRS environment variable

[Kristian Høgsberg]

On Tue, May 14, 2013 at 09:55:19AM -0700, Othman, Ossama wrote:
> Hi Kristian,
> 
> I've attached the patch since the patch I sent through git send-mail was
> sent through an e-mail account that isn't subscribed to this list (waiting
> for moderator approval), and to make sure the spacing isn't munged again.
>  This patch was generated against weston master as of this morning.

Cool, looks good now, committed.  I did catch a corner case in the
spec that we could handle better.  As far as I can see,
XDG_CONFIG_HOME, if set, overrides ~/.config.  That means if it's set
and we don't find a config file there, we shouldn't fall back and try
~/.config/weston.ini.  Similarly, only if XDG_CONFIG_DIRS is not set
do we fall back to /etc/xdg, but we do handle correctly with your
patch.  Also the spec says "not set or empty", so we should be
checking !config_dirs || *config_dirs == '\0'.

Finally, I didn't see a reason for adding weston/ to the path when
iterating the XDG_CONFIG_DIRS?  I guess many applications put their
config file in a subdirectory of the config dir, but at least for
~/.config/weston.ini we don't do that.

I committed a small patch to fix up these issues - let me know if you
see a problem.

> Sorry for the churn.

No that's fine, we usually go through a couple of revisions of review
before committing the patch.  

Thanks,
Kristian

> 
> 
> 
> On Tue, May 14, 2013 at 7:05 AM, Kristian Høgsberg <hoegsberg at gmail.com>wrote:
> 
> > On Mon, May 13, 2013 at 11:41 PM, Othman, Ossama
> > <ossama.othman at intel.com> wrote:
> > > Hi Kristian,
> > >
> > >> I think it's good to go - however, inlining the patch messed up the
> > >> whitespace.  Ideally, send patches using git send-email, which can be
> > >> configured to use smtp servers, and you can pass --annotate if you
> > >> want to add a message or comment to the patch (put it after the ---
> > >> that indicates the end of the commit message).  Or as a last resort,
> > >> attach the patch.
> > >
> > >
> > > I forgot to attach the patch this last time around.  Sorry about that.
> >  I'll
> > > just resend the patch with git send-email, as you suggest.
> > >
> > >>
> > >> As for the next thing, good catch.  I'd do something like
> > >>
> > >>     for (p = config_dirs; p != NULL; p = next) {
> > >>         next = strchrnul(p, ':');
> > >>         if (*next == ':')
> > >>             next++;
> > >>
> > >>         ...
> > >>
> > >> to keep the for (...) more readable.
> > >
> > >
> > > Changing the loop termination to "p != NULL" causes an infinite loop
> > since
> > > strchnul() returns a pointer to the null character '\0', not the NULL
> >
> > Yeah, sorry for the confusion, for a second while writing the email I
> > was thinking that we could just use strchr instead and test for p !=
> > NULL, but that doesn't work for getting the end of the last path
> > element.
> >
> > > pointer.  Also, the moving the next pointer past the colon must be done
> > > after path construction, other the colon ends up at the end of the path.
> > > How's this instead:
> > >
> > > for (p = config_dirs; *p != '\0'; p = next) {
> > > next = strchrnul(p, ':');
> > > snprintf(path, sizeof path,
> > > "%.*s/weston/%s", (int)(next - p), p, name);
> > > fd = open(path, O_RDONLY | O_CLOEXEC);
> > > if (fd >= 0)
> > > return fd;
> > >
> > > if (*next == ':')
> > > next++;
> > > }
> > >
> > > Note that I also had to cast "next - p" in the snprintf() call to int
> > since
> > > the format specifier "%*.s" wants an int but the pointer difference on
> > my 64
> > > bit Ubuntu box is a long int, at least according to the format specifier
> > > warning I get:
> > >
> > > foo.c: In function ‘open_config_file’:
> > > foo.c:53:5: warning: field precision specifier ‘.*’ expects argument of
> > type
> > > ‘int’, but argument 4 has type ‘long int’ [-Wformat]
> > >
> > > foo.c in this case is a simple test program I wrote.  I assume the cast
> > is
> > > safe since "next - p" should always fall within the range of type int.
> > >
> > > Anyway, I'll resend the patch with the above changes with git send-email.
> >
> > Yup, that all sounds good.
> >
> > thanks,
> > Kristian
> > _______________________________________________
> > wayland-devel mailing list
> > wayland-devel at lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/wayland-devel
> >