On Wed, 14 Sep 2016 13:03:58 +0200 Giuseppe Scrivano <[email protected]> wrote:
> Hi Matthew, > > Matthew White <[email protected]> writes: > > > On Sun, 11 Sep 2016 23:39:09 +0200 > > Giuseppe Scrivano <[email protected]> wrote: > > > >> Hi Matthew, > >> > >> Matthew White <[email protected]> writes: > >> > >> > +void > >> > +replace_metalink_basename (char **name, char *ref) > >> > +{ > >> > + size_t dir_len = 0; > >> > + char *p, *dir, *file, *new; > >> > + > >> > + if (!name) > >> > + return; > >> > + > >> > + /* New basename from file name reference. */ > >> > + file = ref; > >> > + if (file) > >> > + { > >> > + p = strrchr (file, '/'); > >> > + if (p) > >> > + file = p + 1; > >> > + } > >> > + > >> > + /* Old directory. */ > >> > + dir = NULL; > >> > + if (*name) > >> > + { > >> > + p = strrchr (*name, '/'); > >> > + if (p) > >> > + dir_len = (p - *name) + 1; > >> > + } > >> > + dir = xstrndup (*name, dir_len); > >> > >> I'll review this patch more in details, but one small thing here, could > >> we modify name in place and avoid a memory allocation for dir? > >> > >> name[dir_len] = '\0'; > > > > Function amended to modify *name in place. > > > > Followed Tim's suggestions for Patch 09/25 about different environments > > compatibility, the function now uses last_component() to detect the > > basename: > > http://lists.gnu.org/archive/html/bug-wget/2016-09/msg00083.html > > > > NOTES: if *name is NULL and ref is like 'dir/C:D:file', the result will be > > 'C:D:file'; is it advisable to remove the drive letters 'C:D:' and return > > only 'file'? > > > > /* > > Replace/remove the basename of a file name. > > > > The file name is permanently modified. > > > > Always set NAME to a string, even an empty one. > > > > Use REF's basename as replacement. If REF is NULL or if it doesn't > > provide a valid basename candidate, then remove NAME's basename. > > */ > > void > > replace_metalink_basename (char **name, char *ref) > > { > > is it something we could do using only dirname and basename? What you > need here is "dirname(name) + basename(ref)"? You asked to avoid superfluous memory allocations, right? > >> I'll review this patch more in details, but one small thing here, could > >> we modify name in place and avoid a memory allocation for dir? > >> > >> name[dir_len] = '\0'; dir_name() allocates new memory and it may add a '.' (append_dot), there's a WARNING disclaimer in this patch: + /* Insert the current "Directory Options". */ + if (metalink->origin) + { + /* WARNING: Do not use lib/dirname.c (dir_name) to + get the directory name, it may append a dot '.' + character to the directory name. */ + metadir = xstrdup (planname); + replace_metalink_basename (&metadir, NULL); + } base_name() allocates new memory, it was also discussed in Patch 09/25 http://lists.gnu.org/archive/html/bug-wget/2016-09/msg00070.html . > > Regards, > Giuseppe Regards, Matthew -- Matthew White <[email protected]>
pgpS4DwtFlqeP.pgp
Description: PGP signature
