Re: [Bug-wget] [PATCH 20/25] New option --metalink-index to process Metalink application/metalink4+xml

[Matthew White]

On Wed, 14 Sep 2016 13:03:58 +0200
Giuseppe Scrivano <address@hidden> wrote:

> Hi Matthew,
> 
> Matthew White <address@hidden> writes:
> 
> > On Sun, 11 Sep 2016 23:39:09 +0200
> > Giuseppe Scrivano <address@hidden> wrote:
> >
> >> Hi Matthew,
> >> 
> >> Matthew White <address@hidden> 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 <address@hidden>

pgp1eeYuJhzYS.pgp
Description: PGP signature