| Message ID | 1524497125.7418.5.camel@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Justin, Sorry for taking so long to get to this... Most of my comments is about the style you are using. I would like to keep the coding style somewhat the same and in a few places that is not just not happening... On 04/23/2018 11:25 AM, Justin Mitchell wrote: > When including additional config files with the include directive > make relative paths be relative to the current config file instead > of to the process's cwd. > > Signed-off-by: Justin Mitchell <jumitche@redhat.com> > --- > support/nfs/conffile.c | 50 +++++++++++++++++++++++++++++++++++++++++--------- > 1 file changed, 41 insertions(+), 9 deletions(-) > > diff --git a/support/nfs/conffile.c b/support/nfs/conffile.c > index a4cc236..42c0913 100644 > --- a/support/nfs/conffile.c > +++ b/support/nfs/conffile.c > @@ -49,6 +49,7 @@ > #include <errno.h> > #include <err.h> > #include <syslog.h> > +#include <libgen.h> > > #include "conffile.h" > #include "xlog.h" > @@ -60,7 +61,7 @@ static char * conf_readfile(const char *path); > static int conf_set(int , const char *, const char *, const char *, > const char *, int , int ); > static void conf_parse(int trans, char *buf, > - char **section, char **subsection); > + char **section, char **subsection, const char *filename); > > struct conf_trans { > TAILQ_ENTRY (conf_trans) link; > @@ -206,12 +207,40 @@ conf_set_now(const char *section, const char *arg, const char *tag, > return 0; > } > > +/* Attempt to construct a relative path to the new file */ > +static char * > +relative_path(const char *oldfile, const char *newfile) > +{ > + char * tmpcopy = NULL; > + char * dir = NULL; > + char * relpath = NULL; The space between the '*' and variable is not need. > + > + if (!newfile) return strdup(oldfile); > + if (newfile[0] == '/') return strdup(newfile); Go a head a put a newline and tab after the if statement. > + > + tmpcopy = strdup(oldfile); > + if (!tmpcopy) goto mem_err; > + > + dir = dirname(tmpcopy); > + size_t pathlen = strlen(dir)+strlen(newfile)+2; > + relpath = calloc(1, pathlen); > + if (!relpath) goto mem_err; > + snprintf(relpath, pathlen, "%s/%s", dir, newfile); Same here... That is just hard to read IMHO... Also put all variable at the top of the routine. > + > + free(tmpcopy); > + return relpath; > +mem_err: > + if (tmpcopy) free(tmpcopy); > + return NULL; > +} > + > + > /* > * Parse the line LINE of SZ bytes. Skip Comments, recognize section > * headers and feed tag-value pairs into our configuration database. > */ > static void > -conf_parse_line(int trans, char *line, int lineno, char **section, char **subsection) > +conf_parse_line(int trans, char *line, const char *filename, int lineno, char **section, char **subsection) > { > char *val, *ptr; > > @@ -366,10 +395,12 @@ conf_parse_line(int trans, char *line, int lineno, char **section, char **subsec > > if (strcasecmp(line, "include")==0) { > /* load and parse subordinate config files */ > - char * subconf = conf_readfile(val); > + char * relpath = relative_path(filename, val); > + char * subconf = conf_readfile(relpath); Same here no space also isn't subconf being over written? > if (subconf == NULL) { > - xlog_warn("config file error: line %d: " > - "error loading included config", lineno); > + xlog_warn("%s:%d: error loading included config %s", > + filename, lineno, relpath); > + if (relpath) free(relpath); > return; > } > > @@ -383,10 +414,11 @@ conf_parse_line(int trans, char *line, int lineno, char **section, char **subsec > inc_subsection = strdup(*subsection); > } > > - conf_parse(trans, subconf, &inc_section, &inc_subsection); > + conf_parse(trans, subconf, &inc_section, &inc_subsection, relpath); > > if (inc_section) free(inc_section); > if (inc_subsection) free(inc_subsection); > + if (relpath) free(relpath); Are these if even needed? Won't these either be NULL or allocated memory? Again... I'm trying to is keep the coding similar which hopefully will make the code easier to read... > free(subconf); > } else { > /* XXX Perhaps should we not ignore errors? */ > @@ -396,7 +428,7 @@ conf_parse_line(int trans, char *line, int lineno, char **section, char **subsec > > /* Parse the mapped configuration file. */ > static void > -conf_parse(int trans, char *buf, char **section, char **subsection) > +conf_parse(int trans, char *buf, char **section, char **subsection, const char *filename) > { > char *cp = buf; > char *bufend = NULL; > @@ -413,7 +445,7 @@ conf_parse(int trans, char *buf, char **section, char **subsection) > else { > *cp = '\0'; > lineno++; > - conf_parse_line(trans, line, lineno, section, subsection); > + conf_parse_line(trans, line, filename, lineno, section, subsection); > line = cp + 1; > } > } > @@ -508,7 +540,7 @@ conf_load_file(const char *conf_file) > /* Parse config contents into the transaction queue */ > char *section = NULL; > char *subsection = NULL; > - conf_parse(trans, conf_data, §ion, &subsection); > + conf_parse(trans, conf_data, §ion, &subsection, conf_file); > if (section) free(section); > if (subsection) free(subsection); Same here... steved. > free(conf_data); > -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, 2018-05-02 at 14:21 -0400, Steve Dickson wrote: > Hi Justin, > > Sorry for taking so long to get to this... > > Most of my comments is about the style you are using. > > I would like to keep the coding style somewhat the same > and in a few places that is not just not happening... Yeah sorry I should have paid more attention to maintaining style, will fix it all up and repost. > > /* > > * Parse the line LINE of SZ bytes. Skip Comments, recognize section > > * headers and feed tag-value pairs into our configuration database. > > */ > > static void > > -conf_parse_line(int trans, char *line, int lineno, char **section, char **subsection) > > +conf_parse_line(int trans, char *line, const char *filename, int lineno, char **section, char **subsection) > > { > > char *val, *ptr; > > > > @@ -366,10 +395,12 @@ conf_parse_line(int trans, char *line, int lineno, char **section, char **subsec > > > > if (strcasecmp(line, "include")==0) { > > /* load and parse subordinate config files */ > > - char * subconf = conf_readfile(val); > > + char * relpath = relative_path(filename, val); > > + char * subconf = conf_readfile(relpath); > Same here no space also isn't subconf being over written? nope, conf_readfile() returns a malloced chunk that gets freed at the end of the include handler > > @@ -383,10 +414,11 @@ conf_parse_line(int trans, char *line, int lineno, char **section, char **subsec > > inc_subsection = strdup(*subsection); > > } > > > > - conf_parse(trans, subconf, &inc_section, &inc_subsection); > > + conf_parse(trans, subconf, &inc_section, &inc_subsection, relpath); > > > > if (inc_section) free(inc_section); > > if (inc_subsection) free(inc_subsection); > > + if (relpath) free(relpath); > Are these if even needed? Won't these either be NULL or allocated memory? yes, inc_section and inc_subsection are strdup()ed copies of the current section titles because conf_parse() will alter them if any new section headers are encountered, yet we needed to know what section we were in when we entered this included conf file. the way it is written will correctly handle recursive includes. in general all my code changes are avoiding any use of globals and statics in the hope of eventually making the code base thread-safe and suitable for use as a public library, so that tends to mean more freeing of short lived malloced chunks. -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/support/nfs/conffile.c b/support/nfs/conffile.c index a4cc236..42c0913 100644 --- a/support/nfs/conffile.c +++ b/support/nfs/conffile.c @@ -49,6 +49,7 @@ #include <errno.h> #include <err.h> #include <syslog.h> +#include <libgen.h> #include "conffile.h" #include "xlog.h" @@ -60,7 +61,7 @@ static char * conf_readfile(const char *path); static int conf_set(int , const char *, const char *, const char *, const char *, int , int ); static void conf_parse(int trans, char *buf, - char **section, char **subsection); + char **section, char **subsection, const char *filename); struct conf_trans { TAILQ_ENTRY (conf_trans) link; @@ -206,12 +207,40 @@ conf_set_now(const char *section, const char *arg, const char *tag, return 0; } +/* Attempt to construct a relative path to the new file */ +static char * +relative_path(const char *oldfile, const char *newfile) +{ + char * tmpcopy = NULL; + char * dir = NULL; + char * relpath = NULL; + + if (!newfile) return strdup(oldfile); + if (newfile[0] == '/') return strdup(newfile); + + tmpcopy = strdup(oldfile); + if (!tmpcopy) goto mem_err; + + dir = dirname(tmpcopy); + size_t pathlen = strlen(dir)+strlen(newfile)+2; + relpath = calloc(1, pathlen); + if (!relpath) goto mem_err; + snprintf(relpath, pathlen, "%s/%s", dir, newfile); + + free(tmpcopy); + return relpath; +mem_err: + if (tmpcopy) free(tmpcopy); + return NULL; +} + + /* * Parse the line LINE of SZ bytes. Skip Comments, recognize section * headers and feed tag-value pairs into our configuration database. */ static void -conf_parse_line(int trans, char *line, int lineno, char **section, char **subsection) +conf_parse_line(int trans, char *line, const char *filename, int lineno, char **section, char **subsection) { char *val, *ptr; @@ -366,10 +395,12 @@ conf_parse_line(int trans, char *line, int lineno, char **section, char **subsec if (strcasecmp(line, "include")==0) { /* load and parse subordinate config files */ - char * subconf = conf_readfile(val); + char * relpath = relative_path(filename, val); + char * subconf = conf_readfile(relpath); if (subconf == NULL) { - xlog_warn("config file error: line %d: " - "error loading included config", lineno); + xlog_warn("%s:%d: error loading included config %s", + filename, lineno, relpath); + if (relpath) free(relpath); return; } @@ -383,10 +414,11 @@ conf_parse_line(int trans, char *line, int lineno, char **section, char **subsec inc_subsection = strdup(*subsection); } - conf_parse(trans, subconf, &inc_section, &inc_subsection); + conf_parse(trans, subconf, &inc_section, &inc_subsection, relpath); if (inc_section) free(inc_section); if (inc_subsection) free(inc_subsection); + if (relpath) free(relpath); free(subconf); } else { /* XXX Perhaps should we not ignore errors? */ @@ -396,7 +428,7 @@ conf_parse_line(int trans, char *line, int lineno, char **section, char **subsec /* Parse the mapped configuration file. */ static void -conf_parse(int trans, char *buf, char **section, char **subsection) +conf_parse(int trans, char *buf, char **section, char **subsection, const char *filename) { char *cp = buf; char *bufend = NULL; @@ -413,7 +445,7 @@ conf_parse(int trans, char *buf, char **section, char **subsection) else { *cp = '\0'; lineno++; - conf_parse_line(trans, line, lineno, section, subsection); + conf_parse_line(trans, line, filename, lineno, section, subsection); line = cp + 1; } } @@ -508,7 +540,7 @@ conf_load_file(const char *conf_file) /* Parse config contents into the transaction queue */ char *section = NULL; char *subsection = NULL; - conf_parse(trans, conf_data, §ion, &subsection); + conf_parse(trans, conf_data, §ion, &subsection, conf_file); if (section) free(section); if (subsection) free(subsection); free(conf_data);
When including additional config files with the include directive make relative paths be relative to the current config file instead of to the process's cwd. Signed-off-by: Justin Mitchell <jumitche@redhat.com> --- support/nfs/conffile.c | 50 +++++++++++++++++++++++++++++++++++++++++--------- 1 file changed, 41 insertions(+), 9 deletions(-)