From: Denys V. <dvl...@re...> - 2012-01-24 09:10:22
|
The commit was: "strace.c (verror_msg): Rewrite without use of heap memory allocation" it replaced "malloc + one fprintf + free" with "three fprintf's". I guess it's my fault that I did not add a comment which explains that one fprintf was used on purpose: we want to (attempt to) send entire message as one write() call. Since stderr is unbuffered, separate fprintf's to it always result in separate writes, they are not coalesced. If we aren't the only program which writes to this particular stderr, this may result in interleaved messages. Since this function is not performance critical, I guess it's ok to make it less efficient. I propose the following version: static void verror_msg(int err_no, const char *fmt, va_list p) { char *msg; fflush(NULL); /* We want to print entire message with single fprintf to ensure * message integrity if stderr is shared with other programs. * Thus we use vasprintf + single fprintf. */ msg = NULL; vasprintf(&msg, fmt, p); if (msg) { if (err_no) fprintf(stderr, "%s: %s: %s\n", progname, msg, strerror(err_no)); else fprintf(stderr, "%s: %s\n", progname, msg); free(msg); } else { /* malloc in vasprintf failed, try it without malloc */ fprintf(stderr, "%s: ", progname); vfprintf(stderr, fmt, p); if (err_no) fprintf(stderr, ": %s\n", strerror(err_no)); else putc('\n', stderr); } /* We don't switch stderr to buffered, thus fprintf(stderr) * always flushes its output and this is not necessary: */ /* fflush(stderr); */ } -- vda |
From: Dmitry V. L. <ld...@al...> - 2012-01-24 10:24:41
|
On Tue, Jan 24, 2012 at 10:10:05AM +0100, Denys Vlasenko wrote: > The commit was: > > "strace.c (verror_msg): Rewrite without use of heap memory allocation" > > it replaced "malloc + one fprintf + free" with "three fprintf's". > > I guess it's my fault that I did not add a comment which explains > that one fprintf was used on purpose: we want to (attempt to) > send entire message as one write() call. Since stderr is unbuffered, > separate fprintf's to it always result in separate writes, > they are not coalesced. If we aren't the only program which > writes to this particular stderr, this may result in interleaved > messages. > > Since this function is not performance critical, I guess > it's ok to make it less efficient. > > I propose the following version: > > > static void verror_msg(int err_no, const char *fmt, va_list p) > { > char *msg; > > fflush(NULL); > > /* We want to print entire message with single fprintf to ensure > * message integrity if stderr is shared with other programs. > * Thus we use vasprintf + single fprintf. > */ OK with me, but > msg = NULL; > vasprintf(&msg, fmt, p); > if (msg) { I'd rather replace these three lines with if (vasprintf(&msg, fmt, p) >= 0) { because the value of "msg" is undefined in case of an error, and various implementations behave quite differently in that case, including a chance to leave an invalid pointer there. -- ldv |
From: Denys V. <dvl...@re...> - 2012-01-24 10:43:05
|
On 01/24/2012 11:24 AM, Dmitry V. Levin wrote: > On Tue, Jan 24, 2012 at 10:10:05AM +0100, Denys Vlasenko wrote: >> msg = NULL; >> vasprintf(&msg, fmt, p); >> if (msg) { > > I'd rather replace these three lines with > > if (vasprintf(&msg, fmt, p)>= 0) { We can combine both just to be 200% paranoid-grade sure: if (vasprintf(&msg, fmt, p) >= 0 && msg) ... -- vda |
From: Dmitry V. L. <ld...@al...> - 2012-01-24 10:53:48
|
On Tue, Jan 24, 2012 at 11:42:49AM +0100, Denys Vlasenko wrote: > On 01/24/2012 11:24 AM, Dmitry V. Levin wrote: > >On Tue, Jan 24, 2012 at 10:10:05AM +0100, Denys Vlasenko wrote: > >> msg = NULL; > >> vasprintf(&msg, fmt, p); > >> if (msg) { > > > >I'd rather replace these three lines with > > > > if (vasprintf(&msg, fmt, p)>= 0) { > > We can combine both just to be 200% paranoid-grade sure: > > if (vasprintf(&msg, fmt, p) >= 0 && msg) ... No need, the return code check is enough. -- ldv |
From: Denys V. <dvl...@re...> - 2012-01-24 11:49:33
|
On 01/24/2012 11:53 AM, Dmitry V. Levin wrote: > On Tue, Jan 24, 2012 at 11:42:49AM +0100, Denys Vlasenko wrote: >> On 01/24/2012 11:24 AM, Dmitry V. Levin wrote: >>> On Tue, Jan 24, 2012 at 10:10:05AM +0100, Denys Vlasenko wrote: >>>> msg = NULL; >>>> vasprintf(&msg, fmt, p); >>>> if (msg) { >>> >>> I'd rather replace these three lines with >>> >>> if (vasprintf(&msg, fmt, p)>= 0) { >> >> We can combine both just to be 200% paranoid-grade sure: >> >> if (vasprintf(&msg, fmt, p)>= 0&& msg) ... > > No need, the return code check is enough. Ok. -- vda |