* 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.