Re: memstat-0.9 hurd patch (PATH_MAX)
Hi, thanks for working on this.
A couple of issues I found in your patch:
> 3) Fix...
> a) "piece of cake" (TM), but include <math.h> (for log10() function) to get
> an accurate length for the buffer... which may be an overhead!
It is indeed much trouble for very few bytes saved :-)
The usual trick for "%d" is to use the constant "sizeof (int) * 3 +
1". I included + 1 for the sign, but it's not really necessary if we
exepect sizeof(int) >= 2, which we probably should.
On the other hand, don't forget to include the byte for the
terminating '\0' in your buffer length calculations!
> b) read file lines in a dynamically re-sized buffer
The usual approach is to reallocate the buffer geometrically, for instance:
size = size ? size * 2 : BUFSIZ;
This way the number of reallocations grows logarithmically rather than
linearily with the actual size we need.
Also, I think there are some problems with your pointer arithmetic.
For instance,
if (fgets(buf+last,size,f) == NULL)
will overwrite the last character read from the previous iteration (if
there was one).
In fact,
- at entry, "last" points to the first unused byte in your buffer (0),
- on subsequent iterations, it points the last used byte (strlen(buf) - 1).
I would say the "first unused byte" convention is more common and
straightforward.
Also, in:
if (buf[last+1] != '\n') { return buf; }
buf[last+1] will always be '\0' (the terminating null character of the
string). I also think you mean to return when the last character _is_
a newline (fortunately the two mistakes cancel each other out in the
usual case of no reallocation needed :-).
> c) use lstat as described in readlink man page
In the error case ("Cannot read link information"),
- "filename" is used after it has been freed
- the two if() blocks could be merged as :
if (len < 0 || len > sb.st_size) { ... }
> 5) But...
> - memstat uses /proc/XYZ/exe and /proc/XYZ/maps... and the output of
> memstat on Hurd is different than the one on Linux
I can't test your patch right now but I'd say this is to be expected.
> - implement (copy/paste mostly) a get_line function. Might have been better
> to use the GNU get_line()... or not?!
The getline() function has only recently been added to POSIX, so
portability might be a problem in the short term. What you can use
generally depends on the intended target platforms for the original
code.
Last, but not least, you should be careful with whitespace. Your patch
mixes tabs and spaces for indentation, and gratuitously removes an
empty line (line 181 in the new version). This may seem trivial at
first, but it's very important for source code to be uniformly
indented, and upstream will rightfully insist that the patches you
submit respect the conventions of the original code.
> Feel free to make comments on everything (code, decisions, style), I know
> that I still have a lot to learn!
Don't we all? :-)
Happy hacking,
--
Jérémie Koenig <jk@jk.fr.eu.org>
http://jk.fr.eu.org/
Reply to: