* How to ideally fix the log function
@ 2019-07-25 1:05 John Wang
2019-07-25 11:21 ` vishwa
0 siblings, 1 reply; 6+ messages in thread
From: John Wang @ 2019-07-25 1:05 UTC (permalink / raw)
To: OpenBMC Maillist, Yu Lei, venture, mspinler, vishwa
Hi all
I am just curious about how to ideally fix the log function
Currently,error log code location metadata is not useful, link:
https://github.com/openbmc/openbmc/issues/2297
That issue mentioned several solutions,But I am still confused.
The call stack of phsophor::logging:log<>() is like this:
func() -> log<>() -> helper_log() -> sd_journal_send
We need to pass arguments(CODE_LINE and CODE_FILE) from `func` to
`sd_journal_send`,right?
Ideally:
template <level L, typename Msg, typename... Entry>
void log(Msg msg, Entry... e,
const source_location& location = source_location::current())
but packs must be last.
is there any suggestions ?
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: How to ideally fix the log function
2019-07-25 1:05 How to ideally fix the log function John Wang
@ 2019-07-25 11:21 ` vishwa
2019-07-26 2:08 ` Andrew Jeffery
0 siblings, 1 reply; 6+ messages in thread
From: vishwa @ 2019-07-25 11:21 UTC (permalink / raw)
To: John Wang, OpenBMC Maillist, Yu Lei, venture, mspinler
Hi John,
Just some background; The goal is not to have func() pass in CODE_LINE
and CODE_FUNC since it would be a huge change all over in the code. So
the plan was to see if something can be done under the cover with no
impacts to current usage of the API.
!! Vishwa !!
On 7/25/19 6:35 AM, John Wang wrote:
> Hi all
>
> I am just curious about how to ideally fix the log function
>
> Currently,error log code location metadata is not useful, link:
> https://github.com/openbmc/openbmc/issues/2297
>
> That issue mentioned several solutions,But I am still confused.
>
> The call stack of phsophor::logging:log<>() is like this:
>
> func() -> log<>() -> helper_log() -> sd_journal_send
>
> We need to pass arguments(CODE_LINE and CODE_FILE) from `func` to
> `sd_journal_send`,right?
>
> Ideally:
>
> template <level L, typename Msg, typename... Entry>
> void log(Msg msg, Entry... e,
> const source_location& location = source_location::current())
>
> but packs must be last.
>
> is there any suggestions ?
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: How to ideally fix the log function
2019-07-25 11:21 ` vishwa
@ 2019-07-26 2:08 ` Andrew Jeffery
2019-07-26 3:03 ` John Wang
0 siblings, 1 reply; 6+ messages in thread
From: Andrew Jeffery @ 2019-07-26 2:08 UTC (permalink / raw)
To: vishwa, John Wang, OpenBMC Maillist, Mine, Patrick Venture, Matt Spinler
On Thu, 25 Jul 2019, at 20:52, vishwa wrote:
> Hi John,
>
> Just some background; The goal is not to have func() pass in CODE_LINE
> and CODE_FUNC since it would be a huge change all over in the code.
There's no way this is really going to work. It turns out sd_journal_print()
is actually a macro that adds the CODE_* properties via __LINE__ etc when
SD_JOURNAL_SUPPRESS_LOCATION is not defined[1], so we either get
the misleading information or force the caller to provide it.
A hack approach that almost immediately falls short is that you could try
macro over log() and adjust the actual log function prototype to take in the
location parameters so we can call sd_journal_send_with_location()
directly. However macro-ing over log won't handle namespaces correctly,
and log() seems to parameterise the level with a template so we can't
define the hypothetical log macro as a macro function (which we need to
do in order to adjust the parameter list).
Having said all that I'm not really a C++ person, but it appears the API has
trapped itself in a C++ corner. Maybe people more experienced and creative
than I can come up with something, but I think the best thing we can do
is three-fold:
1. Build libsystemd with `CFLAGS=-DSD_JOURNAL_SUPPRESS_LOCATION`
2. Add a new log macro that allows us to capture the info properly
3. Change callsites to the new macro function over time.
With 1 we remove the misleading information from the journal, and 2 and 3
get us to the right place, eventually at the expense of CPP macros.
Andrew
[1] https://github.com/systemd/systemd/blob/master/src/systemd/sd-journal.h#L53
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: How to ideally fix the log function
2019-07-26 2:08 ` Andrew Jeffery
@ 2019-07-26 3:03 ` John Wang
2019-07-29 0:26 ` Andrew Jeffery
0 siblings, 1 reply; 6+ messages in thread
From: John Wang @ 2019-07-26 3:03 UTC (permalink / raw)
To: Andrew Jeffery
Cc: vishwa, OpenBMC Maillist, Mine, Patrick Venture, Matt Spinler
On Fri, Jul 26, 2019 at 10:08 AM Andrew Jeffery <andrew@aj.id.au> wrote:
>
>
>
> On Thu, 25 Jul 2019, at 20:52, vishwa wrote:
> > Hi John,
> >
> > Just some background; The goal is not to have func() pass in CODE_LINE
> > and CODE_FUNC since it would be a huge change all over in the code.
>
> There's no way this is really going to work. It turns out sd_journal_print()
> is actually a macro that adds the CODE_* properties via __LINE__ etc when
> SD_JOURNAL_SUPPRESS_LOCATION is not defined[1], so we either get
> the misleading information or force the caller to provide it.
>
> A hack approach that almost immediately falls short is that you could try
> macro over log() and adjust the actual log function prototype to take in the
> location parameters so we can call sd_journal_send_with_location()
> directly. However macro-ing over log won't handle namespaces correctly,
> and log() seems to parameterise the level with a template so we can't
> define the hypothetical log macro as a macro function (which we need to
> do in order to adjust the parameter list).
>
> Having said all that I'm not really a C++ person, but it appears the API has
> trapped itself in a C++ corner. Maybe people more experienced and creative
> than I can come up with something, but I think the best thing we can do
> is three-fold:
>
> 1. Build libsystemd with `CFLAGS=-DSD_JOURNAL_SUPPRESS_LOCATION`
> 2. Add a new log macro that allows us to capture the info properly
Maybe something like below:
```
struct traceLog
{
traceLog(const source_location location = source_location::current()) :
location(location)
{
}
template <level L, typename Msg, typename... Entry>
void log(Msg msg, Entry... e)
{
phosphor::logging::log<L>(msg, entry("CODE_LINE=%d", location.line()),
entry("CODE_FILE=%s", location.file_name()),
e...);
}
private:
source_location location;
};
int main()
{
traceLog{}.log<level::INFO>("xxxxx", entry("entry=%s", "xxxx"));
}
```
> 3. Change callsites to the new macro function over time.
>
> With 1 we remove the misleading information from the journal, and 2 and 3
> get us to the right place, eventually at the expense of CPP macros.
>
> Andrew
>
> [1] https://github.com/systemd/systemd/blob/master/src/systemd/sd-journal.h#L53
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: How to ideally fix the log function
2019-07-26 3:03 ` John Wang
@ 2019-07-29 0:26 ` Andrew Jeffery
2019-07-29 7:53 ` John Wang
0 siblings, 1 reply; 6+ messages in thread
From: Andrew Jeffery @ 2019-07-29 0:26 UTC (permalink / raw)
To: John Wang; +Cc: vishwa, OpenBMC Maillist, Mine, Patrick Venture, Matt Spinler
On Fri, 26 Jul 2019, at 12:33, John Wang wrote:
> On Fri, Jul 26, 2019 at 10:08 AM Andrew Jeffery <andrew@aj.id.au> wrote:
> >
> > Having said all that I'm not really a C++ person, but it appears the API has
> > trapped itself in a C++ corner. Maybe people more experienced and creative
> > than I can come up with something, but I think the best thing we can do
> > is three-fold:
> >
> > 1. Build libsystemd with `CFLAGS=-DSD_JOURNAL_SUPPRESS_LOCATION`
> > 2. Add a new log macro that allows us to capture the info properly
> Maybe something like below:
> ```
> struct traceLog
> {
> traceLog(const source_location location = source_location::current()) :
> location(location)
> {
> }
> template <level L, typename Msg, typename... Entry>
> void log(Msg msg, Entry... e)
> {
> phosphor::logging::log<L>(msg, entry("CODE_LINE=%d", location.line()),
> entry("CODE_FILE=%s", location.file_name()),
> e...);
> }
>
> private:
> source_location location;
> };
>
> int main()
> {
> traceLog{}.log<level::INFO>("xxxxx", entry("entry=%s", "xxxx"));
> }
> ```
My immediate thought is "pretty ugly" :D
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: How to ideally fix the log function
2019-07-29 0:26 ` Andrew Jeffery
@ 2019-07-29 7:53 ` John Wang
0 siblings, 0 replies; 6+ messages in thread
From: John Wang @ 2019-07-29 7:53 UTC (permalink / raw)
To: Andrew Jeffery
Cc: vishwa, OpenBMC Maillist, Mine, Patrick Venture, Matt Spinler
On Mon, Jul 29, 2019 at 8:26 AM Andrew Jeffery <andrew@aj.id.au> wrote:
>
>
>
> On Fri, 26 Jul 2019, at 12:33, John Wang wrote:
> > On Fri, Jul 26, 2019 at 10:08 AM Andrew Jeffery <andrew@aj.id.au> wrote:
> > >
> > > Having said all that I'm not really a C++ person, but it appears the API has
> > > trapped itself in a C++ corner. Maybe people more experienced and creative
> > > than I can come up with something, but I think the best thing we can do
> > > is three-fold:
> > >
> > > 1. Build libsystemd with `CFLAGS=-DSD_JOURNAL_SUPPRESS_LOCATION`
> > > 2. Add a new log macro that allows us to capture the info properly
> > Maybe something like below:
> > ```
> > struct traceLog
> > {
> > traceLog(const source_location location = source_location::current()) :
> > location(location)
> > {
> > }
> > template <level L, typename Msg, typename... Entry>
> > void log(Msg msg, Entry... e)
> > {
> > phosphor::logging::log<L>(msg, entry("CODE_LINE=%d", location.line()),
> > entry("CODE_FILE=%s", location.file_name()),
> > e...);
> > }
> >
> > private:
> > source_location location;
> > };
> >
> > int main()
> > {
> > traceLog{}.log<level::INFO>("xxxxx", entry("entry=%s", "xxxx"));
> > }
> > ```
>
> My immediate thought is "pretty ugly" :D
I tried my best to make those codes "pretty" ... sad.gif..
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2019-07-29 7:54 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-07-25 1:05 How to ideally fix the log function John Wang
2019-07-25 11:21 ` vishwa
2019-07-26 2:08 ` Andrew Jeffery
2019-07-26 3:03 ` John Wang
2019-07-29 0:26 ` Andrew Jeffery
2019-07-29 7:53 ` John Wang
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.