From mboxrd@z Thu Jan 1 00:00:00 1970 X-Msuck: nntp://news.gmane.org/gmane.linux.lib.musl.general/133 Path: news.gmane.org!not-for-mail From: Solar Designer Newsgroups: gmane.linux.lib.musl.general Subject: Re: cluts review Date: Wed, 13 Jul 2011 17:54:23 +0400 Message-ID: <20110713135423.GA23183@openwall.com> References: <20110713110723.GA22153@openwall.com> <4E1D8964.3020502@gmail.com> <20110713122128.GA22658@openwall.com> <4E1D9631.3070203@gmail.com> Reply-To: musl@lists.openwall.com NNTP-Posting-Host: lo.gmane.org Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii X-Trace: dough.gmane.org 1310565272 32758 80.91.229.12 (13 Jul 2011 13:54:32 GMT) X-Complaints-To: usenet@dough.gmane.org NNTP-Posting-Date: Wed, 13 Jul 2011 13:54:32 +0000 (UTC) To: musl@lists.openwall.com Original-X-From: musl-return-217-gllmg-musl=m.gmane.org@lists.openwall.com Wed Jul 13 15:54:28 2011 Return-path: Envelope-to: gllmg-musl@lo.gmane.org Original-Received: from mother.openwall.net ([195.42.179.200]) by lo.gmane.org with smtp (Exim 4.69) (envelope-from ) id 1QgztS-0001TC-GZ for gllmg-musl@lo.gmane.org; Wed, 13 Jul 2011 15:54:26 +0200 Original-Received: (qmail 7941 invoked by uid 550); 13 Jul 2011 13:54:25 -0000 Mailing-List: contact musl-help@lists.openwall.com; run by ezmlm Precedence: bulk List-Post: List-Help: List-Unsubscribe: List-Subscribe: Original-Received: (qmail 7932 invoked from network); 13 Jul 2011 13:54:25 -0000 Content-Disposition: inline In-Reply-To: <4E1D9631.3070203@gmail.com> User-Agent: Mutt/1.4.2.3i Xref: news.gmane.org gmane.linux.lib.musl.general:133 Archived-At: 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