Message ID | 20200131215501.19536-1-maxime.roussinbelanger@gmail.com |
---|---|
State | Superseded, archived |
Headers |
From: maxime.roussinbelanger at gmail.com (Maxime Roussin-Belanger) Date: Fri, 31 Jan 2020 16:55:01 -0500 Subject: [lttng-dev] [PATCH] Introduce vtracef Message-ID: <20200131215501.19536-1-maxime.roussinbelanger@gmail.com> |
Series |
Introduce vtracef
|
|
Commit Message
Maxime Roussin-Bélanger
Jan. 31, 2020, 9:55 p.m. UTC
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 at gmail.com>
---
include/lttng/tracef.h | 10 ++++++++++
liblttng-ust/tracef.c | 18 +++++++++++-------
2 files changed, 21 insertions(+), 7 deletions(-)
Comments
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 at 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
Hi Simon, On Fri, Jan 31, 2020 at 6:03 PM Simon Marchi <simark at 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 at 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 -------------- next part -------------- An HTML attachment was scrubbed... URL: <https://lists.lttng.org/pipermail/lttng-dev/attachments/20200131/ae6ac615/attachment.html>
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
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); }