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 ebl...@redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature
