| Message ID | 20210809062740.322823-1-siddhesh@sourceware.org |
|---|---|
| State | New |
| Headers | show |
| Series | iconvconfig: Fix behaviour with --prefix [BZ #28199] | expand |
On Mon, 9 Aug 2021 at 18:28, Siddhesh Poyarekar <siddhesh@sourceware.org> wrote: > The consolidation of configuration parsing broke behaviour with > --prefix, where the prefix bled into the modules cache. Accept a > prefix which, when non-NULL, is prepended to the path when looking for > configuration files but only the original directory is added to the > modules cache. > > This has no effect on the codegen of gconv_conf since it passes NULL. > I've lightly tested this patch and it fixes the problem I've been experiencing and is certainly much nicer than the patch I attached to the bug report. Cheers, mwh
On 2021-08-09 at 11:57:40 +0530, Siddhesh Poyarekar wrote: > The consolidation of configuration parsing broke behaviour with > --prefix, where the prefix bled into the modules cache. Accept a > prefix which, when non-NULL, is prepended to the path when looking for > configuration files but only the original directory is added to the > modules cache. > > This has no effect on the codegen of gconv_conf since it passes NULL. > > Reported-by: Patrick McCarty <patrick.mccarty@intel.com> > Reported-by: Michael Hudson-Doyle <michael.hudson@canonical.com> > Signed-off-by: Siddhesh Poyarekar <siddhesh@sourceware.org> > --- > iconv/gconv_conf.c | 2 +- > iconv/gconv_parseconfdir.h | 21 +++++++++++++++------ > iconv/iconvconfig.c | 16 ++++++++++++---- > 3 files changed, 28 insertions(+), 11 deletions(-) Testing the patch locally, I see that the prefix is now absent from the modules cache, so this change looks good on my end. Regards, -Patrick
On 8/11/21 12:22 PM, Patrick McCarty via Libc-alpha wrote: > On 2021-08-09 at 11:57:40 +0530, Siddhesh Poyarekar wrote: >> The consolidation of configuration parsing broke behaviour with >> --prefix, where the prefix bled into the modules cache. Accept a >> prefix which, when non-NULL, is prepended to the path when looking for >> configuration files but only the original directory is added to the >> modules cache. >> >> This has no effect on the codegen of gconv_conf since it passes NULL. >> >> Reported-by: Patrick McCarty <patrick.mccarty@intel.com> >> Reported-by: Michael Hudson-Doyle <michael.hudson@canonical.com> >> Signed-off-by: Siddhesh Poyarekar <siddhesh@sourceware.org> >> --- >> iconv/gconv_conf.c | 2 +- >> iconv/gconv_parseconfdir.h | 21 +++++++++++++++------ >> iconv/iconvconfig.c | 16 ++++++++++++---- >> 3 files changed, 28 insertions(+), 11 deletions(-) > > Testing the patch locally, I see that the prefix is now absent from the > modules cache, so this change looks good on my end. Thank you for testing Patrick and Michael. I'll push this to master and the 2.34 branch once I get a code review. Siddhesh
Ping! On 8/9/21 11:57 AM, Siddhesh Poyarekar via Libc-alpha wrote: > The consolidation of configuration parsing broke behaviour with > --prefix, where the prefix bled into the modules cache. Accept a > prefix which, when non-NULL, is prepended to the path when looking for > configuration files but only the original directory is added to the > modules cache. > > This has no effect on the codegen of gconv_conf since it passes NULL. > > Reported-by: Patrick McCarty <patrick.mccarty@intel.com> > Reported-by: Michael Hudson-Doyle <michael.hudson@canonical.com> > Signed-off-by: Siddhesh Poyarekar <siddhesh@sourceware.org> > --- > iconv/gconv_conf.c | 2 +- > iconv/gconv_parseconfdir.h | 21 +++++++++++++++------ > iconv/iconvconfig.c | 16 ++++++++++++---- > 3 files changed, 28 insertions(+), 11 deletions(-) > > diff --git a/iconv/gconv_conf.c b/iconv/gconv_conf.c > index 62bee28769..cc391d8f93 100644 > --- a/iconv/gconv_conf.c > +++ b/iconv/gconv_conf.c > @@ -478,7 +478,7 @@ __gconv_read_conf (void) > __gconv_get_path (); > > for (cnt = 0; __gconv_path_elem[cnt].name != NULL; ++cnt) > - gconv_parseconfdir (__gconv_path_elem[cnt].name, > + gconv_parseconfdir (NULL, __gconv_path_elem[cnt].name, > __gconv_path_elem[cnt].len); > #endif > > diff --git a/iconv/gconv_parseconfdir.h b/iconv/gconv_parseconfdir.h > index 2f062689ec..38ffa9353a 100644 > --- a/iconv/gconv_parseconfdir.h > +++ b/iconv/gconv_parseconfdir.h > @@ -38,8 +38,9 @@ > > /* Name of the file containing the module information in the directories > along the path. */ > -static const char gconv_conf_filename[] = "gconv-modules"; > -static const char gconv_conf_dirname[] = "gconv-modules.d"; > +#define GCONV_CONF_FILENAME "gconv-modules" > +static const char gconv_conf_filename[] = GCONV_CONF_FILENAME; > +static const char gconv_conf_dirname[] = GCONV_CONF_FILENAME ".d"; > > static void add_alias (char *); > static void add_module (char *, const char *, size_t, int); > @@ -110,19 +111,27 @@ read_conf_file (const char *filename, const char *directory, size_t dir_len) > return true; > } > > +/* Prefix DIR (with length DIR_LEN) with PREFIX if the latter is non-NULL and > + parse configuration in it. */ > + > static __always_inline bool > -gconv_parseconfdir (const char *dir, size_t dir_len) > +gconv_parseconfdir (const char *prefix, const char *dir, size_t dir_len) > { > /* No slash needs to be inserted between dir and gconv_conf_filename; > dir already ends in a slash. */ > - char *buf = malloc (dir_len + sizeof (gconv_conf_dirname)); > + size_t buflen = dir_len + sizeof (gconv_conf_dirname); > + char *buf = malloc (buflen + (prefix != NULL ? strlen (prefix) : 0)); > + char *cp = buf; > bool found = false; > > if (buf == NULL) > return false; > > - char *cp = mempcpy (mempcpy (buf, dir, dir_len), gconv_conf_filename, > - sizeof (gconv_conf_filename)); > + if (prefix != NULL) > + cp = stpcpy (cp, prefix); > + > + cp = mempcpy (mempcpy (cp, dir, dir_len), gconv_conf_filename, > + sizeof (gconv_conf_filename)); > > /* Read the gconv-modules configuration file first. */ > found = read_conf_file (buf, dir, dir_len); > diff --git a/iconv/iconvconfig.c b/iconv/iconvconfig.c > index 783b2bbdbb..273a71f673 100644 > --- a/iconv/iconvconfig.c > +++ b/iconv/iconvconfig.c > @@ -653,13 +653,21 @@ add_module (char *rp, const char *directory, > static int > handle_dir (const char *dir) > { > + char *newp = NULL; > size_t dirlen = strlen (dir); > bool found = false; > > - char *fulldir = xasprintf ("%s%s%s", dir[0] == '/' ? prefix : "", > - dir, dir[dirlen - 1] != '/' ? "/" : ""); > + /* End directory path with a '/' if it doesn't already. */ > + if (dir[dirlen - 1] != '/') > + { > + newp = xmalloc (dirlen + 2); > + memcpy (newp, dir, dirlen); > + newp[dirlen++] = '/'; > + newp[dirlen] = '\0'; > + dir = newp; > + } > > - found = gconv_parseconfdir (fulldir, strlen (fulldir)); > + found = gconv_parseconfdir (dir[0] == '/' ? prefix : NULL, dir, dirlen); > > if (!found) > { > @@ -671,7 +679,7 @@ handle_dir (const char *dir) > "configuration files with names ending in .conf."); > } > > - free (fulldir); > + free (newp); > > return found ? 0 : 1; > } >
Ping! On 8/17/21 8:28 AM, Siddhesh Poyarekar wrote: > Ping! > > On 8/9/21 11:57 AM, Siddhesh Poyarekar via Libc-alpha wrote: >> The consolidation of configuration parsing broke behaviour with >> --prefix, where the prefix bled into the modules cache. Accept a >> prefix which, when non-NULL, is prepended to the path when looking for >> configuration files but only the original directory is added to the >> modules cache. >> >> This has no effect on the codegen of gconv_conf since it passes NULL. >> >> Reported-by: Patrick McCarty <patrick.mccarty@intel.com> >> Reported-by: Michael Hudson-Doyle <michael.hudson@canonical.com> >> Signed-off-by: Siddhesh Poyarekar <siddhesh@sourceware.org> >> --- >> iconv/gconv_conf.c | 2 +- >> iconv/gconv_parseconfdir.h | 21 +++++++++++++++------ >> iconv/iconvconfig.c | 16 ++++++++++++---- >> 3 files changed, 28 insertions(+), 11 deletions(-) >> >> diff --git a/iconv/gconv_conf.c b/iconv/gconv_conf.c >> index 62bee28769..cc391d8f93 100644 >> --- a/iconv/gconv_conf.c >> +++ b/iconv/gconv_conf.c >> @@ -478,7 +478,7 @@ __gconv_read_conf (void) >> __gconv_get_path (); >> for (cnt = 0; __gconv_path_elem[cnt].name != NULL; ++cnt) >> - gconv_parseconfdir (__gconv_path_elem[cnt].name, >> + gconv_parseconfdir (NULL, __gconv_path_elem[cnt].name, >> __gconv_path_elem[cnt].len); >> #endif >> diff --git a/iconv/gconv_parseconfdir.h b/iconv/gconv_parseconfdir.h >> index 2f062689ec..38ffa9353a 100644 >> --- a/iconv/gconv_parseconfdir.h >> +++ b/iconv/gconv_parseconfdir.h >> @@ -38,8 +38,9 @@ >> /* Name of the file containing the module information in the >> directories >> along the path. */ >> -static const char gconv_conf_filename[] = "gconv-modules"; >> -static const char gconv_conf_dirname[] = "gconv-modules.d"; >> +#define GCONV_CONF_FILENAME "gconv-modules" >> +static const char gconv_conf_filename[] = GCONV_CONF_FILENAME; >> +static const char gconv_conf_dirname[] = GCONV_CONF_FILENAME ".d"; >> static void add_alias (char *); >> static void add_module (char *, const char *, size_t, int); >> @@ -110,19 +111,27 @@ read_conf_file (const char *filename, const char >> *directory, size_t dir_len) >> return true; >> } >> +/* Prefix DIR (with length DIR_LEN) with PREFIX if the latter is >> non-NULL and >> + parse configuration in it. */ >> + >> static __always_inline bool >> -gconv_parseconfdir (const char *dir, size_t dir_len) >> +gconv_parseconfdir (const char *prefix, const char *dir, size_t dir_len) >> { >> /* No slash needs to be inserted between dir and gconv_conf_filename; >> dir already ends in a slash. */ >> - char *buf = malloc (dir_len + sizeof (gconv_conf_dirname)); >> + size_t buflen = dir_len + sizeof (gconv_conf_dirname); >> + char *buf = malloc (buflen + (prefix != NULL ? strlen (prefix) : 0)); >> + char *cp = buf; >> bool found = false; >> if (buf == NULL) >> return false; >> - char *cp = mempcpy (mempcpy (buf, dir, dir_len), gconv_conf_filename, >> - sizeof (gconv_conf_filename)); >> + if (prefix != NULL) >> + cp = stpcpy (cp, prefix); >> + >> + cp = mempcpy (mempcpy (cp, dir, dir_len), gconv_conf_filename, >> + sizeof (gconv_conf_filename)); >> /* Read the gconv-modules configuration file first. */ >> found = read_conf_file (buf, dir, dir_len); >> diff --git a/iconv/iconvconfig.c b/iconv/iconvconfig.c >> index 783b2bbdbb..273a71f673 100644 >> --- a/iconv/iconvconfig.c >> +++ b/iconv/iconvconfig.c >> @@ -653,13 +653,21 @@ add_module (char *rp, const char *directory, >> static int >> handle_dir (const char *dir) >> { >> + char *newp = NULL; >> size_t dirlen = strlen (dir); >> bool found = false; >> - char *fulldir = xasprintf ("%s%s%s", dir[0] == '/' ? prefix : "", >> - dir, dir[dirlen - 1] != '/' ? "/" : ""); >> + /* End directory path with a '/' if it doesn't already. */ >> + if (dir[dirlen - 1] != '/') >> + { >> + newp = xmalloc (dirlen + 2); >> + memcpy (newp, dir, dirlen); >> + newp[dirlen++] = '/'; >> + newp[dirlen] = '\0'; >> + dir = newp; >> + } >> - found = gconv_parseconfdir (fulldir, strlen (fulldir)); >> + found = gconv_parseconfdir (dir[0] == '/' ? prefix : NULL, dir, >> dirlen); >> if (!found) >> { >> @@ -671,7 +679,7 @@ handle_dir (const char *dir) >> "configuration files with names ending in .conf."); >> } >> - free (fulldir); >> + free (newp); >> return found ? 0 : 1; >> } >> >
diff --git a/iconv/gconv_conf.c b/iconv/gconv_conf.c index 62bee28769..cc391d8f93 100644 --- a/iconv/gconv_conf.c +++ b/iconv/gconv_conf.c @@ -478,7 +478,7 @@ __gconv_read_conf (void) __gconv_get_path (); for (cnt = 0; __gconv_path_elem[cnt].name != NULL; ++cnt) - gconv_parseconfdir (__gconv_path_elem[cnt].name, + gconv_parseconfdir (NULL, __gconv_path_elem[cnt].name, __gconv_path_elem[cnt].len); #endif diff --git a/iconv/gconv_parseconfdir.h b/iconv/gconv_parseconfdir.h index 2f062689ec..38ffa9353a 100644 --- a/iconv/gconv_parseconfdir.h +++ b/iconv/gconv_parseconfdir.h @@ -38,8 +38,9 @@ /* Name of the file containing the module information in the directories along the path. */ -static const char gconv_conf_filename[] = "gconv-modules"; -static const char gconv_conf_dirname[] = "gconv-modules.d"; +#define GCONV_CONF_FILENAME "gconv-modules" +static const char gconv_conf_filename[] = GCONV_CONF_FILENAME; +static const char gconv_conf_dirname[] = GCONV_CONF_FILENAME ".d"; static void add_alias (char *); static void add_module (char *, const char *, size_t, int); @@ -110,19 +111,27 @@ read_conf_file (const char *filename, const char *directory, size_t dir_len) return true; } +/* Prefix DIR (with length DIR_LEN) with PREFIX if the latter is non-NULL and + parse configuration in it. */ + static __always_inline bool -gconv_parseconfdir (const char *dir, size_t dir_len) +gconv_parseconfdir (const char *prefix, const char *dir, size_t dir_len) { /* No slash needs to be inserted between dir and gconv_conf_filename; dir already ends in a slash. */ - char *buf = malloc (dir_len + sizeof (gconv_conf_dirname)); + size_t buflen = dir_len + sizeof (gconv_conf_dirname); + char *buf = malloc (buflen + (prefix != NULL ? strlen (prefix) : 0)); + char *cp = buf; bool found = false; if (buf == NULL) return false; - char *cp = mempcpy (mempcpy (buf, dir, dir_len), gconv_conf_filename, - sizeof (gconv_conf_filename)); + if (prefix != NULL) + cp = stpcpy (cp, prefix); + + cp = mempcpy (mempcpy (cp, dir, dir_len), gconv_conf_filename, + sizeof (gconv_conf_filename)); /* Read the gconv-modules configuration file first. */ found = read_conf_file (buf, dir, dir_len); diff --git a/iconv/iconvconfig.c b/iconv/iconvconfig.c index 783b2bbdbb..273a71f673 100644 --- a/iconv/iconvconfig.c +++ b/iconv/iconvconfig.c @@ -653,13 +653,21 @@ add_module (char *rp, const char *directory, static int handle_dir (const char *dir) { + char *newp = NULL; size_t dirlen = strlen (dir); bool found = false; - char *fulldir = xasprintf ("%s%s%s", dir[0] == '/' ? prefix : "", - dir, dir[dirlen - 1] != '/' ? "/" : ""); + /* End directory path with a '/' if it doesn't already. */ + if (dir[dirlen - 1] != '/') + { + newp = xmalloc (dirlen + 2); + memcpy (newp, dir, dirlen); + newp[dirlen++] = '/'; + newp[dirlen] = '\0'; + dir = newp; + } - found = gconv_parseconfdir (fulldir, strlen (fulldir)); + found = gconv_parseconfdir (dir[0] == '/' ? prefix : NULL, dir, dirlen); if (!found) { @@ -671,7 +679,7 @@ handle_dir (const char *dir) "configuration files with names ending in .conf."); } - free (fulldir); + free (newp); return found ? 0 : 1; }
The consolidation of configuration parsing broke behaviour with --prefix, where the prefix bled into the modules cache. Accept a prefix which, when non-NULL, is prepended to the path when looking for configuration files but only the original directory is added to the modules cache. This has no effect on the codegen of gconv_conf since it passes NULL. Reported-by: Patrick McCarty <patrick.mccarty@intel.com> Reported-by: Michael Hudson-Doyle <michael.hudson@canonical.com> Signed-off-by: Siddhesh Poyarekar <siddhesh@sourceware.org> --- iconv/gconv_conf.c | 2 +- iconv/gconv_parseconfdir.h | 21 +++++++++++++++------ iconv/iconvconfig.c | 16 ++++++++++++---- 3 files changed, 28 insertions(+), 11 deletions(-)