[PATCH] Introduce vtracef

[PATCH] Introduce vtracef

[Maxime Roussin-Belanger]

vtracef accepts a va_list argument to simplify tracing
functions which use a va_list

Here's an example from wpa_supplicant that I wanted to
trace:

void wpa_debug(int level, const char* fmt, ...) {

    va_list ap;
    va_start(ap, fmt);

    ...
    // The call I want to easily trace with vtracef
    vprintf(fmt, ap);

    ...
    va_end(ap);
}

wpa_debug is used a fair amount and it would be annoying to
replace all the wpa_debug calls with tracef.

With vtracef, it simplifies the find and replace effort by
only changing it at one place.

Signed-off-by: Maxime Roussin-Belanger <maxime.roussinbelanger@gmail.com>
---
 include/lttng/tracef.h | 10 ++++++++++
 liblttng-ust/tracef.c  | 18 +++++++++++-------
 2 files changed, 21 insertions(+), 7 deletions(-)

diff --git a/include/lttng/tracef.h b/include/lttng/tracef.h
index 0c59c9ae..e3a7587d 100644
--- a/include/lttng/tracef.h
+++ b/include/lttng/tracef.h
@@ -32,13 +32,23 @@ extern "C" {
 extern
 void _lttng_ust_tracef(const char *fmt, ...);
 
+extern
+void _lttng_ust_vtracef(const char *fmt, va_list ap);
+
 #define tracef(fmt, ...)						\
 	do {								\
 		LTTNG_STAP_PROBEV(tracepoint_lttng_ust_tracef, event, ## __VA_ARGS__); \
 		if (caa_unlikely(__tracepoint_lttng_ust_tracef___event.state)) \
 			_lttng_ust_tracef(fmt, ## __VA_ARGS__);		\
 	} while (0)
 
+#ifndef LTTNG_UST_HAVE_SDT_INTEGRATION
+#define vtracef(fmt, ap)						\
+	do {								\
+		if (caa_unlikely(__tracepoint_lttng_ust_tracef___event.state)) \
+			_lttng_ust_vtracef(fmt, ap);		\
+	} while (0)
+#endif
 #ifdef __cplusplus
 }
 #endif
diff --git a/liblttng-ust/tracef.c b/liblttng-ust/tracef.c
index ea98e43e..2a809eed 100644
--- a/liblttng-ust/tracef.c
+++ b/liblttng-ust/tracef.c
@@ -29,20 +29,24 @@
 #define TRACEPOINT_DEFINE
 #include "lttng-ust-tracef-provider.h"
 
-void _lttng_ust_tracef(const char *fmt, ...)
+void _lttng_ust_vtracef(const char *fmt, va_list ap)
 {
-	va_list ap;
 	char *msg;
-	int len;
-
-	va_start(ap, fmt);
-	len = vasprintf(&msg, fmt, ap);
+	const int len = vasprintf(&msg, fmt, ap);
 	/* len does not include the final \0 */
 	if (len < 0)
-		goto end;
+		return;
 	__tracepoint_cb_lttng_ust_tracef___event(msg, len,
 		LTTNG_UST_CALLER_IP());
 	free(msg);
+}
+
+void _lttng_ust_tracef(const char *fmt, ...)
+{
+	va_list ap;
+
+	va_start(ap, fmt);
+	_lttng_ust_vtracef(fmt, ap);
 end:
 	va_end(ap);
 }
-- 
2.20.1

Re: [PATCH] Introduce vtracef

[Simon Marchi]

Hi Maxime,

Tip for next time: this mailing is used for many projects, so it helps
to include the project name in the patch prefix.  You can do so by using

  --subject-prefix="PATCH lttng-ust"

on you git-send-email command line.

On 2020-01-31 4:55 p.m., Maxime Roussin-Belanger wrote:
> vtracef accepts a va_list argument to simplify tracing
> functions which use a va_list
> 
> Here's an example from wpa_supplicant that I wanted to
> trace:
> 
> void wpa_debug(int level, const char* fmt, ...) {
> 
>     va_list ap;
>     va_start(ap, fmt);
> 
>     ...
>     // The call I want to easily trace with vtracef
>     vprintf(fmt, ap);
> 
>     ...
>     va_end(ap);
> }
> 
> wpa_debug is used a fair amount and it would be annoying to
> replace all the wpa_debug calls with tracef.
> 
> With vtracef, it simplifies the find and replace effort by
> only changing it at one place.

The shortcoming, as you mentiond on IRC, is that it doesn't support SDT integration.  The tracef
macro call has a `STAP_PROBEV` call, but it doesn't seem possible to use that here.  The number
of arguments in a va_list is known at runtime (in fact it's not really known by the va_list itself),
whereas STAP_PROBEV wants to know the number of arguments at compile time.

I think it would still be useful to have vtracef, for the use case explained above, but have it
documented that it is not integrated with the stap probes (unless somebody has a magic idea that
would make it work).  But that will be for the maintainer to decide.

Note that if the feature is accepted, it will require at least a man page update. An update in
doc/examples/demo-tracef/demo-tracef.c would also be nice, to have an example of how it can be used.

Simon

> 
> Signed-off-by: Maxime Roussin-Belanger <maxime.roussinbelanger@gmail.com>
> ---
>  include/lttng/tracef.h | 10 ++++++++++
>  liblttng-ust/tracef.c  | 18 +++++++++++-------
>  2 files changed, 21 insertions(+), 7 deletions(-)
> 
> diff --git a/include/lttng/tracef.h b/include/lttng/tracef.h
> index 0c59c9ae..e3a7587d 100644
> --- a/include/lttng/tracef.h
> +++ b/include/lttng/tracef.h
> @@ -32,13 +32,23 @@ extern "C" {
>  extern
>  void _lttng_ust_tracef(const char *fmt, ...);
>  
> +extern
> +void _lttng_ust_vtracef(const char *fmt, va_list ap);
> +
>  #define tracef(fmt, ...)						\
>  	do {								\
>  		LTTNG_STAP_PROBEV(tracepoint_lttng_ust_tracef, event, ## __VA_ARGS__); \
>  		if (caa_unlikely(__tracepoint_lttng_ust_tracef___event.state)) \
>  			_lttng_ust_tracef(fmt, ## __VA_ARGS__);		\
>  	} while (0)
>  
> +#ifndef LTTNG_UST_HAVE_SDT_INTEGRATION
> +#define vtracef(fmt, ap)						\
> +	do {								\
> +		if (caa_unlikely(__tracepoint_lttng_ust_tracef___event.state)) \
> +			_lttng_ust_vtracef(fmt, ap);		\
> +	} while (0)
> +#endif

If we end up with what I suggested above (vtracef documented not to integrate an
stap probe), this #ifndef would go away,

>  #ifdef __cplusplus
>  }
>  #endif
> diff --git a/liblttng-ust/tracef.c b/liblttng-ust/tracef.c
> index ea98e43e..2a809eed 100644
> --- a/liblttng-ust/tracef.c
> +++ b/liblttng-ust/tracef.c
> @@ -29,20 +29,24 @@
>  #define TRACEPOINT_DEFINE
>  #include "lttng-ust-tracef-provider.h"
>  
> -void _lttng_ust_tracef(const char *fmt, ...)
> +void _lttng_ust_vtracef(const char *fmt, va_list ap)
>  {
> -	va_list ap;
>  	char *msg;
> -	int len;
> -
> -	va_start(ap, fmt);
> -	len = vasprintf(&msg, fmt, ap);
> +	const int len = vasprintf(&msg, fmt, ap);
>  	/* len does not include the final \0 */
>  	if (len < 0)
> -		goto end;
> +		return;

We try to have a single exit point in each function, to facilitate
resource management in the case of errors, so please keep the goto
end pattern:

  ...

  if (len < 0)
    goto end;

  ...

end:
  return
}

>  	__tracepoint_cb_lttng_ust_tracef___event(msg, len,
>  		LTTNG_UST_CALLER_IP());
>  	free(msg);
> +}
> +
> +void _lttng_ust_tracef(const char *fmt, ...)
> +{
> +	va_list ap;
> +
> +	va_start(ap, fmt);
> +	_lttng_ust_vtracef(fmt, ap);
>  end:

You can remove the end label here.

Simon

Re: [PATCH] Introduce vtracef

[Maxime Roussin-Bélanger]

[-- Attachment #1.1: Type: text/plain, Size: 5019 bytes --]

Hi Simon,

On Fri, Jan 31, 2020 at 6:03 PM Simon Marchi <simark@simark.ca> wrote:
>
> Hi Maxime,
>
> Tip for next time: this mailing is used for many projects, so it helps
> to include the project name in the patch prefix.  You can do so by using
>
>   --subject-prefix="PATCH lttng-ust"
>
> on you git-send-email command line.

Will do!

>
> On 2020-01-31 4:55 p.m., Maxime Roussin-Belanger wrote:
> > vtracef accepts a va_list argument to simplify tracing
> > functions which use a va_list
> >
> > Here's an example from wpa_supplicant that I wanted to
> > trace:
> >
> > void wpa_debug(int level, const char* fmt, ...) {
> >
> >     va_list ap;
> >     va_start(ap, fmt);
> >
> >     ...
> >     // The call I want to easily trace with vtracef
> >     vprintf(fmt, ap);
> >
> >     ...
> >     va_end(ap);
> > }
> >
> > wpa_debug is used a fair amount and it would be annoying to
> > replace all the wpa_debug calls with tracef.
> >
> > With vtracef, it simplifies the find and replace effort by
> > only changing it at one place.
>
> The shortcoming, as you mentiond on IRC, is that it doesn't support SDT
integration.  The tracef
> macro call has a `STAP_PROBEV` call, but it doesn't seem possible to use
that here.  The number
> of arguments in a va_list is known at runtime (in fact it's not really
known by the va_list itself),
> whereas STAP_PROBEV wants to know the number of arguments at compile time.
>
> I think it would still be useful to have vtracef, for the use case
explained above, but have it
> documented that it is not integrated with the stap probes (unless
somebody has a magic idea that
> would make it work).  But that will be for the maintainer to decide.

Where would that be? In the man page? If so it would be the first time
editing/creating a man page.
It seems fairly simple, but I probably need some guidance on where to start.

>
> Note that if the feature is accepted, it will require at least a man page
update. An update in
> doc/examples/demo-tracef/demo-tracef.c would also be nice, to have an
example of how it can be used.

Looking at the example, maybe I should create another file demo-vtracef.c?

Max.

>
> Simon
>
> >
> > Signed-off-by: Maxime Roussin-Belanger <maxime.roussinbelanger@gmail.com
>
> > ---
> >  include/lttng/tracef.h | 10 ++++++++++
> >  liblttng-ust/tracef.c  | 18 +++++++++++-------
> >  2 files changed, 21 insertions(+), 7 deletions(-)
> >
> > diff --git a/include/lttng/tracef.h b/include/lttng/tracef.h
> > index 0c59c9ae..e3a7587d 100644
> > --- a/include/lttng/tracef.h
> > +++ b/include/lttng/tracef.h
> > @@ -32,13 +32,23 @@ extern "C" {
> >  extern
> >  void _lttng_ust_tracef(const char *fmt, ...);
> >
> > +extern
> > +void _lttng_ust_vtracef(const char *fmt, va_list ap);
> > +
> >  #define tracef(fmt, ...)                                             \
> >       do {                                                            \
> >               LTTNG_STAP_PROBEV(tracepoint_lttng_ust_tracef, event, ##
__VA_ARGS__); \
> >               if
(caa_unlikely(__tracepoint_lttng_ust_tracef___event.state)) \
> >                       _lttng_ust_tracef(fmt, ## __VA_ARGS__);         \
> >       } while (0)
> >
> > +#ifndef LTTNG_UST_HAVE_SDT_INTEGRATION
> > +#define vtracef(fmt, ap)                                             \
> > +     do {                                                            \
> > +             if
(caa_unlikely(__tracepoint_lttng_ust_tracef___event.state)) \
> > +                     _lttng_ust_vtracef(fmt, ap);            \
> > +     } while (0)
> > +#endif
>
> If we end up with what I suggested above (vtracef documented not to
integrate an
> stap probe), this #ifndef would go away,
>
> >  #ifdef __cplusplus
> >  }
> >  #endif
> > diff --git a/liblttng-ust/tracef.c b/liblttng-ust/tracef.c
> > index ea98e43e..2a809eed 100644
> > --- a/liblttng-ust/tracef.c
> > +++ b/liblttng-ust/tracef.c
> > @@ -29,20 +29,24 @@
> >  #define TRACEPOINT_DEFINE
> >  #include "lttng-ust-tracef-provider.h"
> >
> > -void _lttng_ust_tracef(const char *fmt, ...)
> > +void _lttng_ust_vtracef(const char *fmt, va_list ap)
> >  {
> > -     va_list ap;
> >       char *msg;
> > -     int len;
> > -
> > -     va_start(ap, fmt);
> > -     len = vasprintf(&msg, fmt, ap);
> > +     const int len = vasprintf(&msg, fmt, ap);
> >       /* len does not include the final \0 */
> >       if (len < 0)
> > -             goto end;
> > +             return;
>
> We try to have a single exit point in each function, to facilitate
> resource management in the case of errors, so please keep the goto
> end pattern:
>
>   ...
>
>   if (len < 0)
>     goto end;
>
>   ...
>
> end:
>   return
> }
>
> >       __tracepoint_cb_lttng_ust_tracef___event(msg, len,
> >               LTTNG_UST_CALLER_IP());
> >       free(msg);
> > +}
> > +
> > +void _lttng_ust_tracef(const char *fmt, ...)
> > +{
> > +     va_list ap;
> > +
> > +     va_start(ap, fmt);
> > +     _lttng_ust_vtracef(fmt, ap);
> >  end:
>
> You can remove the end label here.
>
> Simon

[-- Attachment #1.2: Type: text/html, Size: 6740 bytes --]

[-- Attachment #2: Type: text/plain, Size: 156 bytes --]

_______________________________________________
lttng-dev mailing list
lttng-dev@lists.lttng.org
https://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev

Re: [PATCH] Introduce vtracef

[Simon Marchi]

On 2020-01-31 6:17 p.m., Maxime Roussin-Bélanger wrote:
>> On 2020-01-31 4:55 p.m., Maxime Roussin-Belanger wrote:
>> > vtracef accepts a va_list argument to simplify tracing
>> > functions which use a va_list
>> >
>> > Here's an example from wpa_supplicant that I wanted to
>> > trace:
>> >
>> > void wpa_debug(int level, const char* fmt, ...) {
>> >
>> >     va_list ap;
>> >     va_start(ap, fmt);
>> >
>> >     ...
>> >     // The call I want to easily trace with vtracef
>> >     vprintf(fmt, ap);
>> >
>> >     ...
>> >     va_end(ap);
>> > }
>> >
>> > wpa_debug is used a fair amount and it would be annoying to
>> > replace all the wpa_debug calls with tracef.
>> >
>> > With vtracef, it simplifies the find and replace effort by
>> > only changing it at one place.
>>
>> The shortcoming, as you mentiond on IRC, is that it doesn't support SDT integration.  The tracef
>> macro call has a `STAP_PROBEV` call, but it doesn't seem possible to use that here.  The number
>> of arguments in a va_list is known at runtime (in fact it's not really known by the va_list itself),
>> whereas STAP_PROBEV wants to know the number of arguments at compile time.
>>
>> I think it would still be useful to have vtracef, for the use case explained above, but have it
>> documented that it is not integrated with the stap probes (unless somebody has a magic idea that
>> would make it work).  But that will be for the maintainer to decide.
> 
> Where would that be? In the man page? If so it would be the first time editing/creating a man page.
> It seems fairly simple, but I probably need some guidance on where to start.

I think it would make sense for tracef and vtracef to be documented in the same man page, a bit like
printf and vprintf are documented in the same man page.  Looking at how it's done for printf/vprintf,
the vprintf man page is a symlink to printf's man page:

$ ls -l /usr/share/man/man3/printf.3.gz
-rw-r--r-- 1 root root 9.3K Feb  2  2018 /usr/share/man/man3/printf.3.gz
$ ls -l /usr/share/man/man3/vfprintf.3.gz
lrwxrwxrwx 1 root root 11 Feb  2  2018 /usr/share/man/man3/vfprintf.3.gz -> printf.3.gz

So I suppose we could do the same.  It will juste require a bit of automake/Makefile magic.

The tracef man page doesn't mention its stap probe integration, but it probably should.  And then
it would be the place (maybe in the LIMITATIONS section) to say that vtracef doesn't integrate a
stap probe.

>>
>> Note that if the feature is accepted, it will require at least a man page update. An update in
>> doc/examples/demo-tracef/demo-tracef.c would also be nice, to have an example of how it can be used.
> 
> Looking at the example, maybe I should create another file demo-vtracef.c?

As you wish, I think either would be fine.

Simon
_______________________________________________
lttng-dev mailing list
lttng-dev@lists.lttng.org
https://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev