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 <hoegsb...@gmail.com>wrote: > > > On Mon, May 13, 2013 at 11:41 PM, Othman, Ossama > > <ossama.oth...@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@lists.freedesktop.org > > http://lists.freedesktop.org/mailman/listinfo/wayland-devel > > _______________________________________________ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel