Re: [PATCH 2/3] getcwd-lgpl: new module

[Bruno Haible]

Hi Eric,

Good move. I also like to see a glibc compatible getcwd() that does not
require the complex 'openat' and 'fdopendir' machinery.

A remark regarding copyright. Through your now module, lib/getcwd.c changes
from GPLv3+ to LGPLv2+. (It is too hard to explain to lawyers that part of a
file could be under GPL and another part under LGPL. It is also too hard for
users to check that a series of #ifs happen to pick only the LGPL code.)
So, either the authors of this file (Paul, Jim, Petr Salinger, and perhaps me)
need to give their approval to this copyright change. Or you move your new
code to lib/getcwd-lgpl.c, like we have two files canonicalize.c and
canonicalize-lgpl.c with code from different origins.

About the code:

> +char *
> +rpl_getcwd (char *buf, size_t size)
> +{
> +  char *tmp = NULL;

Useless initialization, since 'tmp = buf;' in the first loop round.

> +  bool first = true;
> +
> +  if (buf)
> +    return getcwd (buf, size);
> +
> +  do
> +    {
> +      tmp = buf;
> +      if (first)
> +        {
> +          size = size ? size : 4096;

This code is hard to understand, because you are playing ping-pong
with two variables 'buf' and 'tmp', and
  - buf is used at a point where it is NULL,
  - tmp appears to be the longer-lived variable and buf the short-term
    pointer - just the opposite what the variable names suggest,
  - At the end of the loop, you use tmp in one case and buf in the other case.

Can't you use just 1 'char *' variable outside the loop, and 1 or 2
extra - temporary - 'char *' variables with a scope limited to the loop?
That would be definitely simpler to understand.

> +      if ((buf = realloc (tmp, size)) == NULL)

It is good style to not put assignments or other side effects into 'if'
conditions:

         buf = realloc (tmp, size);
         if (buf == NULL)

> +      /* Trim to fit, if possible.  */
> +      tmp = realloc (buf, strlen (buf) + 1);
> +      if (!tmp)
> +        tmp = buf;

If I understand the glibc documentation right, this trimming should only
occur when size == 0. Quoting the glibc manual:

     The GNU library version of this function also permits you to
     specify a null pointer for the BUFFER argument.  Then `getcwd'
     allocates a buffer automatically, as with `malloc' (*note
     Unconstrained Allocation::).  If the SIZE is greater than zero,
     then the buffer is that large; otherwise, the buffer is as large
     as necessary to hold the result.

This is what glibc/sysdeps/unix/sysv/linux/getcwd.c does. Yes, this calling
convention leads to lots of allocated and unused memory.

Bruno
-- 
In memoriam Heinrich Conradi <http://de.wikipedia.org/wiki/Heinrich_Conradi>