Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Better logging required #77

Closed
ThomasAdam opened this issue May 12, 2020 · 7 comments · Fixed by #79
Closed

Better logging required #77

ThomasAdam opened this issue May 12, 2020 · 7 comments · Fixed by #79
Assignees
Milestone

Comments

@ThomasAdam
Copy link
Member

ThomasAdam commented May 12, 2020

At the moment, FVWM3's logging is OK if you know where to look, but ultimately overly verbose.

We should standardise on using something like log_debug() to do our logging for us, and allowing the user to set a path/filename to the log file.

This should be toggleable via USR2, for example.

Auditing of existing fvwm_msg() callers should be done to determine what's still relevant.

A start on this should be made for milestone 1.0 if possible. See: ta/logging

@ThomasAdam ThomasAdam added this to the 1.0 milestone May 12, 2020
@ThomasAdam ThomasAdam self-assigned this May 12, 2020
@ThomasAdam ThomasAdam linked a pull request May 27, 2020 that will close this issue
@ThomasAdam ThomasAdam reopened this May 27, 2020
@ThomasAdam ThomasAdam modified the milestones: 1.0, post-1.0 May 27, 2020
@ThomasAdam
Copy link
Member Author

A start on this has been made, and is now merged to master ready for 1.0

This now means:

  • Callers using fprintf(stderr, ...); have been converted to use fvwm_debug();
  • Logging now happens to a file: fvwm3-<PID>.log, typically in $HOME -- this is to reduce the confusion around where some display managers log STDERR to.
  • Logging can be toggled at run-time by sending SIGUSR2 to fvwm3, a la: pkill -USR2 fvwm3

Post 1.0:

  • Rationalise logging (less verbose/more accurate);
  • Add additional log lines in the code;
  • Check module support for libs/log.c and check they can use it -- some modules (FvwmIconMan? replicates fvwm_msg() itself because it's not in libfvwm.

@afhp-2020
Copy link

Is there any way to enable logging at startup, instead of explicity with SIGUSR2 ?

When something goes wrong as Fvwm is setting itself up, the old log in xsession-errors caught everything. Now if one has to wait until it's up (when it does come up) manually activating logging is much too late in the process.

@ThomasAdam
Copy link
Member Author

@afhp-2020,

You can pass -v to fvwm3, hence: fvwm3 -v

Pull from master first...

@afhp-2020
Copy link

Pulling from master is what got me in today's rabbit hole (other report incoming).

Some issues I've noticed:

  • fvwm -v on a "fresh" setup works fine: the log is created and filled.
  • fvwm -v if there already is a ~/fvwm-<some pid>.log will not open a new log and will not update the existing log (the latter makes sense, since the pid has changed; but there is no fvwm-<new pid>.log either). This is observed when completely exiting fvwm and relaunching it.
  • Restarting fvwm will stop updating the current log (the pid is unchanged), while the toggle is still on ; in other words, after a restart SIGUSR2 must be sent twice otherwise nothing new appears in the log.

ThomasAdam added a commit that referenced this issue Jun 2, 2020
Previously, the log file written to by fvwm3 used to contain the pid in
the file name.  This isn't useful, and since the log file is opened in
append mode anyway, it makes more sense to remove this.

Also added a description to the man page.

Helps with #77
ThomasAdam added a commit that referenced this issue Jun 2, 2020
Previously, the log file written to by fvwm3 used to contain the pid in
the file name.  This isn't useful, and since the log file is opened in
append mode anyway, it makes more sense to remove this.

Also added a description to the man page.

Helps with #77
@ThomasAdam
Copy link
Member Author

Hi @afhp-2020,

Yeah. Removing the PID from the filename makes sense here. Since the log file is opened in append mode, this should keep writing to it. I'm about to merge to master a fix for this, such that the log file is: ~/fvwm3-output.log.

Restarting fvwm3 keeps appending to this file, and I don't need to keep toggling fvwm3 via SIGUSR2. If I do, and fvwm3 -v is already present, it toggles as expected.

ThomasAdam added a commit that referenced this issue Jun 2, 2020
Previously, the log file written to by fvwm3 used to contain the pid in
the file name.  This isn't useful, and since the log file is opened in
append mode anyway, it makes more sense to remove this.

Also added a description to the man page.

Helps with #77
@NsCDE
Copy link
Contributor

NsCDE commented Jun 6, 2020

Hi Thomas,

Very little time in the last weeks, but before my wife spots me in the front of the monitor on a weekend day (!), please tell me what you think of this:


--- a/libs/log.c
+++ b/libs/log.c
@@ -46,7 +46,16 @@ log_set_level(int ll)
 void
 log_open(void)
 {
-       char    *path = "fvwm3-output.log";
+       char *path;
+
+       if (getenv("FVWM_LOGFILE"))
+       {
+               path = fxstrdup(getenv("FVWM_LOGFILE"));
+       }
+       else
+       {
+               path = "fvwm3-output.log";
+       }
 
        if (log_level == 0)
                return;

Some people will probably have often or permanent logging turned on, and creating file with fixed file name in $HOME is maybe not flexible enough.

Should I create pull request?

@ThomasAdam
Copy link
Member Author

ThomasAdam commented Jun 6, 2020

Hi @NsCDE,

Can do -- although this is all rather micro-arbitrary stuff.

Please submit a PR for this, but note that:

  • It should be FVWM3_LOGFILE;
  • The default if FVWM3_LOGFILE isn't used should now be stored in a #define instead;
  • We'd need to handle ~/ substitution. For that, see getpwuid, etc.

Thomas

@ThomasAdam ThomasAdam modified the milestones: post-1.0, 1.0 Jul 29, 2020
mikeandmore pushed a commit to mikeandmore/fvwm3 that referenced this issue Nov 28, 2020
Previously, the log file written to by fvwm3 used to contain the pid in
the file name.  This isn't useful, and since the log file is opened in
append mode anyway, it makes more sense to remove this.

Also added a description to the man page.

Helps with fvwmorg#77
@ThomasAdam ThomasAdam moved this to Done in FVWM3 Sep 18, 2022
@ThomasAdam ThomasAdam added this to FVWM3 Sep 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

3 participants