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 <j...@jk.fr.eu.org> http://jk.fr.eu.org/ -- To UNSUBSCRIBE, email to debian-hurd-requ...@lists.debian.org with a subject of "unsubscribe". Trouble? Contact listmas...@lists.debian.org Archive: http://lists.debian.org/ca+kcsaz5vaynjcpvky6pwajub1ngmuwjpodj+0mfh2pn-+3...@mail.gmail.com