On Sun. 13 Nov. 2022 at 21:18, Marc Kleine-Budde <mkl@xxxxxxxxxxxxxx> wrote:
> On 13.11.2022 17:53:18, Vincent Mailhol wrote:
> > Add the pr_debug() macro so that replace:
> >
> > #ifdef DEBUG
> > printf("foo");
> > #endif
> >
> > by:
> >
> > pr_debug("foo");
> >
> > Apply the pr_debug() macro wherever relevant.
> >
> > Currently, there is no consensus whether debug messages should be
> > printed on stdout or stderr. Most of the modules: canbusload.c,
> > candump.c and canlogserver.c use stdout but
> > mcp251xfd/mcp251xfd-dev-coredump.c uses stderr. Harmonize the behavior
> > by following the major trend and make
> > mcp251xfd/mcp251xfd-dev-coredump.c also output to stdout.
> >
> > CC: Marc Kleine-Budde <mkl@xxxxxxxxxxxxxx>
> > Signed-off-by: Vincent Mailhol <mailhol.vincent@xxxxxxxxxx>
> > ---
> > @Marc, was there any reasons to print debug information to stderr and
> > not stdout in mcp251xfd-dev-coredump.c?
>
> No real reason, just gut feeling. There are at least 2 differences: IIRC
> stdout is line buffered, it will not write to console until a newline.
> stderr will print even if there is no newline.
Something I learned today!
I thought that all the buffering was done by the libc (i.e.
write(stdout, "foo") is not buffered while fprint(stdout, "foo") is.
But after investigation, you are right. Indeed, 'man 3 stdout' says:
The stream stderr is unbuffered. The stream stdout is
line-buffered when it points to a terminal. Partial lines will
not appear until fflush(3) or exit(3) is called, or a newline
is printed. This can produce unexpected results, especially
with debugging output.
> The other one is: If
> use/parse the stdout if the program you're debugging (i.e. in a pipe)
> the debug output on stdout will interfere with the regular output.
I agree. I am fine with putting the debug to stderr or to prefix the
stdout output with "debug: " or something similar so that it can be
easily filtered (usually, I do the latter). The only things which
really bothered me was the #DEBUG everywhere and the inconsistency of
stdout vs. stderr. As long as those things are fixed, anything else is
OK for me.
By default, I just did the most concervative change: everyone prints
to stdout and I did not add a prefix.
> > diff --git a/lib.h b/lib.h
> > index a4d3ce5..1cb1dd4 100644
> > --- a/lib.h
> > +++ b/lib.h
> > @@ -47,6 +47,12 @@
> >
> > #include <stdio.h>
> >
> > +#ifdef DEBUG
> > +#define pr_debug(fmt, args...) printf(fmt, ##args)
> > +#else
> > +#define pr_debug(...)
> > +#endif
>
> With this change if DEBUG is not defined, the print format strings are
> not checked. This is why I have the pr_no macro. Side node: For functions
> you can use __attribute__ ((format (printf, 2, 3))).
I never thought of that. Now I understand why you put a pr_no macro.
I knew about the print attribute and I prefer to use it instead of the
pr_no to make it more explicit that this is only for compiler check.
Now, I am thinking to do:
#ifdef DEBUG
#define pr_debug(fmt, args...) printf(fmt, ##args)
#else
__attribute__((format (printf, 1, 2)))
static inline int pr_debug(const char* fmt, ...) {return 0;}
#endif
> > +
> > /* buffer sizes for CAN frame string representations */
> >
> > #define CL_ID (sizeof("12345678##1"))
> > diff --git a/mcp251xfd/mcp251xfd-dev-coredump.c b/mcp251xfd/mcp251xfd-dev-coredump.c
> > index 5874d24..422900f 100644
> > --- a/mcp251xfd/mcp251xfd-dev-coredump.c
> > +++ b/mcp251xfd/mcp251xfd-dev-coredump.c
> > @@ -17,18 +17,10 @@
> >
> > #include <linux/kernel.h>
> >
> > +#include "../lib.h"
> > #include "mcp251xfd.h"
> > #include "mcp251xfd-dump-userspace.h"
> >
> > -#define pr_err(fmt, args...) fprintf(stderr, fmt, ##args)
> > -#define pr_no(fmt, args...) while (0) { fprintf(stdout, fmt, ##args); }
> > -
> > -#ifdef DEBUG
> > -#define pr_debug(fmt, args...) pr_err(fmt, ##args)
> > -#else
> > -#define pr_debug(fmt, args...) pr_no(fmt, ##args)
> > -#endif
> > -
> >
> > struct mcp251xfd_dump_iter {
> > const void *start;
> > diff --git a/slcanpty.c b/slcanpty.c
> > index 4ac9e8a..3eba07e 100644
> > --- a/slcanpty.c
> > +++ b/slcanpty.c
> > @@ -49,8 +49,6 @@
> > #define SLC_MTU (sizeof("T1111222281122334455667788EA5F\r")+1)
> > #define DEVICE_NAME_PTMX "/dev/ptmx"
> >
> > -#define DEBUG
>
> For completeness mention that you switch off debugging in slcanpty.c.
Ack.
Yours sincerely,
Vincent Mailhol
