Re: [PATCH 03/19] libctf: lowest-level memory allocation and debug-dumping wrappers

[Nick Clifton <nickc@redhat.com>]

Hi Nick,

> I could easily be convinced that all of these wrappers serve no purpose
> and should be globally removed, but the debugging tracer is frequently
> useful, and the malloc/free/mmap/munmap wrappers have proved mildly
> useful in conjunction with symbol interposition for allocation debugging
> in the (relatively distant) past.

I see no problem with retaining these wrappers.  They are not going to inflict
a large overhead on the library, and if they have proved helpful in debugging
then they have proved their worth.  Besides having functions like these allows
them to be intercepted by third parties, if for some reason that becomes
necessary.

> (I am amenable to replacing the environment-variable triggering of
> ctf_dprintf() with something else in the binutils commit,

I am not a fan of using environment variables, although in this particular
case it is not so bad.  There is of course the problem of documentation -
you must tell the users about the variable - and also the fact that users
of the library might wish to enable/disable debugging themselves.  Perhaps
the library could provide a function to set the value of _libctf_debug even
after _libctf_init() has been called ?

> +/* Miscellary.
> +   Copyright (C) 2003-2019 Free Software Foundation, Inc.
> +

Ooo - just noticed - this header, and others too, do not mention who
contributed the code.  It would be nice to see your efforts acknowledged
don't you think ?


> +void *
> +ctf_data_alloc (size_t size)
> +{
> +  return (mmap (NULL, size, PROT_READ | PROT_WRITE,
> +		MAP_PRIVATE | MAP_ANON, -1, 0));
> +}

I am not a memory allocation expert - but is it efficient to
call mmap for every memory allocation ?  Or do higher levels
of libctf manage the allocated memory so that this function is
only used when needed to grab large chunks of memory ?


> +void
> +ctf_data_free (void *buf, size_t size)
> +{
> +  (void) munmap (buf, size);
> +}

Why do you ignore and discard the return value from munmap() ?
If there is an error, surely you would want to examine it or
return it to the caller ?


> +void
> +ctf_free (void *buf, size_t size _libctf_unused_)
> +{
> +  free (buf);
> +}

Why does your free function have a size parameter at all ?


> +_libctf_printflike_ (1, 2)
> +void ctf_dprintf (const char *format, ...)
> +{
> +  if (_libctf_debug)
> +    {
> +      va_list alist;
> +
> +      va_start (alist, format);
> +      (void) fputs ("libctf DEBUG: ", stderr);
> +      (void) vfprintf (stderr, format, alist);

I would recommend calling "fflush (stdout)" before starting to
print to stderr, just in order to make sure that the debug output
does not appear halfway through a line of ordinary output.

Cheers
  Nick