* [PATCH] Introduce vtracef
@ 2020-01-31 21:55 Maxime Roussin-Belanger
2020-01-31 23:03 ` Simon Marchi
0 siblings, 1 reply; 4+ messages in thread
From: Maxime Roussin-Belanger @ 2020-01-31 21:55 UTC (permalink / raw)
To: lttng-dev; +Cc: champagne.guillaume.c
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
^ permalink raw reply related [flat|nested] 4+ messages in thread* Re: [PATCH] Introduce vtracef 2020-01-31 21:55 [PATCH] Introduce vtracef Maxime Roussin-Belanger @ 2020-01-31 23:03 ` Simon Marchi 2020-01-31 23:17 ` Maxime Roussin-Bélanger 0 siblings, 1 reply; 4+ messages in thread From: Simon Marchi @ 2020-01-31 23:03 UTC (permalink / raw) To: Maxime Roussin-Belanger, lttng-dev; +Cc: champagne.guillaume.c 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 ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] Introduce vtracef 2020-01-31 23:03 ` Simon Marchi @ 2020-01-31 23:17 ` Maxime Roussin-Bélanger 2020-01-31 23:30 ` Simon Marchi 0 siblings, 1 reply; 4+ messages in thread From: Maxime Roussin-Bélanger @ 2020-01-31 23:17 UTC (permalink / raw) To: Simon Marchi; +Cc: lttng-dev, champagne.guillaume.c [-- 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 ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] Introduce vtracef 2020-01-31 23:17 ` Maxime Roussin-Bélanger @ 2020-01-31 23:30 ` Simon Marchi 0 siblings, 0 replies; 4+ messages in thread From: Simon Marchi @ 2020-01-31 23:30 UTC (permalink / raw) To: Maxime Roussin-Bélanger; +Cc: lttng-dev, champagne.guillaume.c 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 ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2020-01-31 23:30 UTC | newest] Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-01-31 21:55 [PATCH] Introduce vtracef Maxime Roussin-Belanger 2020-01-31 23:03 ` Simon Marchi 2020-01-31 23:17 ` Maxime Roussin-Bélanger 2020-01-31 23:30 ` Simon Marchi
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).