Date: Wed, 13 Jul 2011 17:54:23 +0400 From: Solar Designer <solar@...nwall.com> To: musl@...ts.openwall.com Subject: Re: cluts review On Wed, Jul 13, 2011 at 02:57:21PM +0200, Luka Mar??eti?? wrote: > I guess I like doing things manually. Sure. I also don't blindly apply patches that people send me for my code. But I don't think there's a better way to represent the changes being proposed. > But thanks for the patch. Anyway, here: > https://github.com/lmarcetic/cluts/commit/7c836ff779c1f9ffecdae9f7d469772e88d3bc68 OK. Why the "#define _POSIX_C_SOURCE 200809L //sigaction" vs. "#define _XOPEN_SOURCE //sigaction" inconsistency, though? I think _XOPEN_SOURCE is a safer bet here. And I generally avoid C++ comments in C source files, even though this became legal in C99 (and many C compilers recognized C++ comments before then). If we enable/require C99 anyway, this is a non-issue, and perhaps I am too old-fashioned. ;-) Testing cluts on some older systems could make sense, though - not so much to test those systems' libc's, but rather to better test cluts itself. > (note also that cluts.c should now return correct nr. of failed tests, > that would've been a valid critique) Yes, I noticed weird output there. Thanks for fixing it. > >What you could actually want to do is get rid of the dependency on > >PATH_MAX and FILENAME_MAX. The system does not guarantee that actual > >pathnames fit in PATH_MAX anyway. So you may want to replace those > >strcat()'s with dynamic memory allocation or add a check for potential > >buffer overflow there (then report the error and skip the test). > > Ah, wherever I do this, I think the path isn't big anyway (cluts.c and > buf.c). I'll keep it in mind if there's ever a chance they might be. Agreed? I don't strictly object to this, but I am trying to help you improve your code quality overall. It may be preferable to consistently apply best practices to cluts source code rather than to take shortcuts in places where this is currently deemed acceptable. That said, I agree that you have other priorities right now than dealing with those existing pathname length issues. > I already have my sreturnf function for such purposes (right now it uses > vsnprintf). Oh, I didn't notice this one yet. Thanks for pointing it out. > Why do you think *snprintf and *asprintf aren't portable? They just are less portable than older functions such as sprintf(). In practice, I think *snprintf() are portable enough these days and for this specific application, so your use of vsnprintf() is fine (although calling it with "NULL, 0" is a stress-test). asprintf() may or may not be portable enough depending on what systems cluts is meant to run on. Alexander
Powered by blists - more mailing lists
Confused about mailing lists and their use? Read about mailing lists on Wikipedia and check out these guidelines on proper formatting of your messages.