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

[Eric Blake]

On 04/26/2011 04:34 AM, Bruno Haible wrote:
> 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.

I'm going with the latter (getcwd-lgpl.c will be a new file when I post v2).

> 
> 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.

My v2 rewrite uses more variables, with better names, to hopefully make
the logic easier to follow.

> 
>> +      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)

Sure.  After all, it's more lines, but fewer (), to separate the two
phases, as well as making it slightly easier to follow in gdb.

> 
>> +      /* 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.

Oops, good point.  I've altered v2 to break the work into 4 phases:

if (buf) return getcwd()
if (size) single malloc, getcwd, free buf on error, and return
using stack, try getcwd, strdup on success
iterate over larger sizes until we don't have ERANGE failure

-- 
Eric Blake   address@hidden    +1-801-349-2682
Libvirt virtualization library http://libvirt.org

signature.asc
Description: OpenPGP digital signature