| Message ID | 20210722132430.1945153-1-siddhesh@sourceware.org |
|---|---|
| State | New |
| Headers | show |
| Series | mtrace: Use a static buffer for printing [BZ #25947] | expand |
Ping! On 7/22/21 6:54 PM, Siddhesh Poyarekar via Libc-alpha wrote: > Use a static buffer for mtrace printing now that it no longer adds to > default libc footprint. > --- > malloc/mtrace-impl.c | 14 +++----------- > 1 file changed, 3 insertions(+), 11 deletions(-) > > diff --git a/malloc/mtrace-impl.c b/malloc/mtrace-impl.c > index 0e10ab7f60..83008ca18f 100644 > --- a/malloc/mtrace-impl.c > +++ b/malloc/mtrace-impl.c > @@ -34,11 +34,8 @@ > > #include <kernel-features.h> > > -#define TRACE_BUFFER_SIZE 512 > - > static FILE *mallstream; > static const char mallenv[] = "MALLOC_TRACE"; > -static char *malloc_trace_buffer; > > static void > tr_where (const void *caller, Dl_info *info) > @@ -184,16 +181,13 @@ do_mtrace (void) > mallfile = secure_getenv (mallenv); > if (mallfile != NULL) > { > - char *mtb = malloc (TRACE_BUFFER_SIZE); > - if (mtb == NULL) > - return; > - > mallstream = fopen (mallfile != NULL ? mallfile : "/dev/null", "wce"); > if (mallstream != NULL) > { > /* Be sure it doesn't malloc its buffer! */ > - malloc_trace_buffer = mtb; > - setvbuf (mallstream, malloc_trace_buffer, _IOFBF, TRACE_BUFFER_SIZE); > + static char tracebuf [512]; > + > + setvbuf (mallstream, tracebuf, _IOFBF, sizeof (tracebuf)); > fprintf (mallstream, "= Start\n"); > if (!added_atexit_handler) > { > @@ -203,8 +197,6 @@ do_mtrace (void) > } > __malloc_debug_enable (MALLOC_MTRACE_HOOK); > } > - else > - free (mtb); > } > } > >
Siddhesh Poyarekar via Libc-alpha <libc-alpha@sourceware.org> writes: > -#define TRACE_BUFFER_SIZE 512 I think it would be prudent to keep this, instead of hard-coding the 512 in the code below. I don't feel strongly about it, since the 512 only appears once. > -static char *malloc_trace_buffer; We could have kept this here, too, but I have no real preference about it. > - char *mtb = malloc (TRACE_BUFFER_SIZE); > - if (mtb == NULL) > - return; Ok. > /* Be sure it doesn't malloc its buffer! */ > - malloc_trace_buffer = mtb; > - setvbuf (mallstream, malloc_trace_buffer, _IOFBF, TRACE_BUFFER_SIZE); > + static char tracebuf [512]; > + > + setvbuf (mallstream, tracebuf, _IOFBF, sizeof (tracebuf)); _IOFBF is same as before, so OK, but I wonder if _IOLBF (line buffered) would be more user-friendly? I can't think of a use case for piping the output through "tail -f" or something, and actually needing to wait for it, but it's text output. > - else > - free (mtb); Ok. LGTM as-is, my comments are for pondering only ;-) Reviewed-by: DJ Delorie <dj@redhat.com>
diff --git a/malloc/mtrace-impl.c b/malloc/mtrace-impl.c index 0e10ab7f60..83008ca18f 100644 --- a/malloc/mtrace-impl.c +++ b/malloc/mtrace-impl.c @@ -34,11 +34,8 @@ #include <kernel-features.h> -#define TRACE_BUFFER_SIZE 512 - static FILE *mallstream; static const char mallenv[] = "MALLOC_TRACE"; -static char *malloc_trace_buffer; static void tr_where (const void *caller, Dl_info *info) @@ -184,16 +181,13 @@ do_mtrace (void) mallfile = secure_getenv (mallenv); if (mallfile != NULL) { - char *mtb = malloc (TRACE_BUFFER_SIZE); - if (mtb == NULL) - return; - mallstream = fopen (mallfile != NULL ? mallfile : "/dev/null", "wce"); if (mallstream != NULL) { /* Be sure it doesn't malloc its buffer! */ - malloc_trace_buffer = mtb; - setvbuf (mallstream, malloc_trace_buffer, _IOFBF, TRACE_BUFFER_SIZE); + static char tracebuf [512]; + + setvbuf (mallstream, tracebuf, _IOFBF, sizeof (tracebuf)); fprintf (mallstream, "= Start\n"); if (!added_atexit_handler) { @@ -203,8 +197,6 @@ do_mtrace (void) } __malloc_debug_enable (MALLOC_MTRACE_HOOK); } - else - free (mtb); } }