search

linux-application-whitelisting / fapolicyd

File Access Policy Daemon
GNU General Public License v3.0
192 stars 55 forks source link

Improve logging: make stderr output more colourful; add timestamps #275

Closed Kangie closed 8 months ago

Kangie commented 9 months ago

stderr logging got colourful and timestamped:

image

stevegrubb commented 8 months ago

Thanks for the patch. There are a lot of formatting changes in this patch which complicates seeing what actually changed. Please separate formatting from logical changes in future PR's to be a separate PR.

About the time format...that should be locale specific. "%x %T" might be better. Fputc is faster than fprintf since it doesn't parse a format string.

Still reviewing...

Kangie commented 8 months ago

Thanks for the patch. There are a lot of formatting changes in this patch which complicates seeing what actually changed. Please separate formatting from logical changes in future PR's to be a separate PR.

Ack.

About the time format...that should be locale specific. "%x %T" might be better. Fputc is faster than fprintf since it doesn't parse a format string.

Updated to (I think) address this as desired.

I'm not sure if setlocale should be called here or just done once when fapolicyd is initialised - it seems to work if I set it within int main(...){, which I have staged but not yet committed. Would you prefer that we set LC_ALL, instead?

stevegrubb commented 8 months ago

Would you prefer that we set LC_ALL, instead?

Yes. We only use the time locale items but this is easier.

stevegrubb commented 8 months ago

Actually, it's not necessary. Looks good to me. Thanks!

Kangie commented 8 months ago

I think it might be better to set it once instead of every time we call msg in the current PR, but my understanding of locale stuff is... developing, so I'll take your advice.

Move this to main instead?

https://github.com/linux-application-whitelisting/fapolicyd/blob/ac13faac8b1dd6db40727b00f15f36e86d50b584/src/library/message.c#L62

stevegrubb commented 8 months ago

Yes, put in main() somewhere near the beginning. Btw, we are close to doing a new release sometime before the holidays. Mentioning that in case you have anything else pending.

Kangie commented 8 months ago

I have a WIP portage/ebuild backend that's pretty much ready to go pending some tests - I'll ping you in that PR and see about splitting it up into a few PRs to make the changes easier to review :)

This should be good to go provided that it all passes CI!

image

stevegrubb commented 8 months ago

config.h always goes first. It can have defines that are needed to swing in and out other includes or even functions within an include. This is a generality of how configure.ac works.

Kangie commented 8 months ago

Fixed!

Kangie commented 8 months ago

or not, let me fix that vscode shrapnel, silly editor.

stevegrubb commented 8 months ago

OK. Looks good to me. Thanks!