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

update documentation for PrintInfo : it now writes to ~/fvwm3-output instead of stderr #161

Closed
wants to merge 6 commits into from

Conversation

d-e-e-p
Copy link
Contributor

@d-e-e-p d-e-e-p commented Jul 5, 2020

PrintInfo uses fvwm_debug instead of fprintf(stderr, ...

Copy link
Member

@ThomasAdam ThomasAdam left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @d-e-e-p,

Thanks for this. I've noticed that we don't mention anywhere in man fvwm3 that Fvwm3 listens for:

  • SIGUSR1 -- used as a signal to restart Fvwm3;
  • SIGUSR2 -- used as a signal to toggle opening/closing its log file.

I think it's easier to add a small section to Fvwm3's man page detailing this, and then refer back to it from sections such as PrintInfo. That way we don't end up duplicating a lot of these instances.

In terms of your original change though, @d-e-e-p, note that it's not just -v to enable debugging -- SIGUSR2 can be used to affect a running instance of Fvwm3 so that one doesn't need to restart Fvwm3, thus potentially missing vital problems as Fvwm3's running at that specific time. Furthermore, -l can be used to change the default log file name. so always assuming ~/.fvwm3-output.log isn't quite correct.

So can this change morph into documenting the signals Fvwm3 listens for, and perhaps also mention that Fvwm3 can do its own logging to a file, for debug purposes?

Cheers,
Thomas

@d-e-e-p
Copy link
Contributor Author

d-e-e-p commented Jul 6, 2020

I think for most mortals the SIGUSR2 way is going to be confusing--especially for PrintInfo. Ideally PrintInfo internally turns on logging if it wasn't on before, and then restores state after running. eg:

                                                                                                                              
[-l log-file] changes the location of fvwm default log file from ~/fvwm3-output.log to log-file                                   
[-v ]         Enables debug logging.  Writes in append mode to fvwm log file, which is ~/fvwm3-output.log by default              
            Logging can also be dynamically turned on and off by sending SIGUSR2 signal to fvwm with "kill -USR2 pid"           
                                                                                                                                
PrintInfo subject [verbose]                                                                                                       
                                                                                                                                
  Print information on subject to fvwm log file, which is ~/fvwm3-output.log by default.                                        
  ...                                                                                                                           
               

I have "PrintInfo ImageCache 1" in my startup config file somewhere...

@ThomasAdam
Copy link
Member

Hi @d-e-e-p,

Well, we're not talking about most mere mortals, this is a means of independently capturing useful information.

SIGUSR2 is tremendously useful for diagnostics.

I'm not going to make PrintInfo a special case for weird toggling either.

Let me know if you plan on working on this further. If not, I'll add a note to do it.

Cheers,
Thomas

@d-e-e-p
Copy link
Contributor Author

d-e-e-p commented Jul 6, 2020

Is there an updated doc/fvwm/options.xml somewhere? in the current version it seems -l is aliased to --color-limit .
it seems your plan is:

                                                                                                                                
[-l log-file] changes the location of fvwm default log file from ~/fvwm3-output.log to log-file                                   
[-v ]         Enables debug logging.  Writes in append mode to fvwm log file, which is ~/fvwm3-output.log by default              
              Logging can also be dynamically toggled on and off by sending SIGUSR2 signal to fvwm with "kill -USR2 pid"          
                                                                                                                                  
PrintInfo subject [verbose]                                                                                                       
                                                                                                                                  
  Print information on subject to fvwm log file, which is ~/fvwm3-output.log by default.                                          
  PrintInfo writes this only when debugging is enabled, either by invoking fvwm3 with -v option or sending fvwm3 SIGUSR2 signal.                              
  ...   

I could help redo the doc if you need the help.

@ThomasAdam
Copy link
Member

Hi @d-e-e-p,

Ah -- thanks. You've spotted a bug there, since -l only works due to the order the options are parsed in, so we should remove -l as an alias to --color-limit since it's not even used. Care to do that, too?

@d-e-e-p
Copy link
Contributor Author

d-e-e-p commented Jul 7, 2020

i'll take a stab. things to do in options.xml :

  • remove -l and -L options, but leave intact all that dated discussion of XFree-4.2 color allocation and netscape. ..
  • add -l option that changes default debug logfile
  • add info about SIGUSR1/SIGUSR2

@d-e-e-p
Copy link
Contributor Author

d-e-e-p commented Jul 7, 2020

if both -l option and FVWM3_LOGFILE env var is specified, I presume -l wins? or should we remove one of these...
other random ttd:

@ThomasAdam
Copy link
Member

Hi @d-e-e-p,

Don't worry about ColorLimit and IconPath for now, as the code paths still shim to the newer implementations.

As for DesktopConfiguration, there's already a section in man fvwm3 about that.

@ThomasAdam
Copy link
Member

Hi @d-e-e-p,

I'd misread something you mentioned earlier. I thought you were saying that I had previously added -l to change the log path/filename, and that this was in conflict with --color-limit. But that's not what you meant. Apologies for the confusion.

I don't necessarily think we need -l, when we already have FVWM3_LOGFILE as an env var check... At this point, it's all a variation on the same theme.

@somiaj
Copy link
Collaborator

somiaj commented Jul 7, 2020

Hi @d-e-e-p

Seeing this pull request caused me to notice that the log file was put in $HOME instead of $FVWM_USERDIR by default. I think it would probably be best to use $FVWM_USERDIR to keep all fvwm files in a central location. Here is a patch that prepends $FVWM_USERDIR to the default log file location and to $FVWM3_LOGFILE if it doesn't contain a '/'. Since you are working on this already, here is the patch that you can choose to include with your changes or not.

diff --git a/fvwm/fvwm3.c b/fvwm/fvwm3.c
index ff4f9a7a..a03b55ae 100644
--- a/fvwm/fvwm3.c
+++ b/fvwm/fvwm3.c
@@ -295,7 +295,7 @@ Restart(int sig)
 static RETSIGTYPE
 ToggleLogging(int sig)
 {
-       log_toggle();
+       log_toggle(fvwm_userdir);

        SIGNAL_RETURN;
 }
@@ -2069,7 +2069,7 @@ int main(int argc, char **argv)
                else if (strcmp(argv[i], "-v") == 0)
                {
                        log_set_level(1);
-                       log_open();
+                       log_open(fvwm_userdir);
                }
                else
                {
diff --git a/libs/log.c b/libs/log.c
index 58fcdccd..81da555e 100644
--- a/libs/log.c
+++ b/libs/log.c
@@ -45,9 +45,10 @@ log_set_level(int ll)

 /* Open logging to file. */
 void
-log_open(void)
+log_open(const char *fvwm_userdir)
 {
-       char            *path = fxstrdup(FVWM3_LOGFILE_DEFAULT);
+       char *path;
+       asprintf(&path, "%s/%s", fvwm_userdir, FVWM3_LOGFILE_DEFAULT);

        if (getenv("FVWM3_LOGFILE")) {
                const char      *expanded_path;
@@ -56,7 +57,12 @@ log_open(void)
                path = getenv("FVWM3_LOGFILE");
                expanded_path = expand_path(path);

-               path = fxstrdup(expanded_path);
+               if (strchr(expanded_path, '/') != NULL)
+               {
+                       path = fxstrdup(expanded_path);
+               } else {
+                       asprintf(&path, "%s/%s", fvwm_userdir, expanded_path);
+               }
                free((char *)expanded_path);
        }

@@ -77,11 +83,11 @@ log_open(void)

 /* Toggle logging. */
 void
-log_toggle(void)
+log_toggle(const char *fvwm_userdir)
 {
        if (log_level == 0) {
                log_level = 1;
-               log_open();
+               log_open(fvwm_userdir);
                fvwm_debug(NULL, "log opened (because of SIGUSR2)");
        } else {
                fvwm_debug(NULL, "log closed (because of SIGUSR2)");
diff --git a/libs/log.h b/libs/log.h
index c43347c8..7933e9e5 100644
--- a/libs/log.h
+++ b/libs/log.h
@@ -6,8 +6,8 @@

 void   log_set_level(int);
 int    log_get_level(void);
-void   log_open(void);
-void   log_toggle(void);
+void   log_open(const char *);
+void   log_toggle(const char *);
 void   log_close(void);
 void printflike(2, 3) fvwm_debug(const char *, const char *, ...);

- added documentation on using SIGUSR1/SIGUSR2 to manpage
- cleanup of intro to fvwm in manpage
- add pointers to override default config/log files using env vars
- update PrintInfo doc to explain how to see output from command
- move default debug log file from $HOME to $FVWM_USERDIR (patch by
ThomasAdam)
@@ -23,7 +23,7 @@

<para>Print information on
<replaceable>subject</replaceable>
on stderr. An optional integer argument
to ~/fvwm3-output.log. fvwm3 has to be run with -v option for this log file to be written. An optional integer argument
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
to ~/fvwm3-output.log. fvwm3 has to be run with -v option for this log file to be written. An optional integer argument
to ~/fvwm3-output.log. fvwm3 has to be run with -v option for this log file to be written, or sent SIGUSR2. An optional integer argument

Copy link
Member

@ThomasAdam ThomasAdam left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @d-e-e-p,

Perfect -- I think this change is looking good! Just a few things to consider; comments in-line.

Also, the code change to logging -- has anyone tested this?

Comment on lines 26 to 34
to debug log file, which is <filename>$HOME/.fvwm/fvwm3-output.log</filename>
by default. <envar>$FVWM_USERDIR</envar> overrides
<filename>$HOME/.fvwm<filename> directory, while
<envar>$FVWM3_LOGFILE</envar> overrides debug log path and filename. For this
logfile to be written, either fvwm3 has to be run with <option>-v</option>
option or <option>SIGUSR2</option> signal can be used to toggle
opening/closing debug log file.</para>

<para> An optional integer argument
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Be careful with the implication here -- we're not suggesting that FVWM_USERDIR should be changed while Fvwm3 is running -- that would be bad. I think what you mean to say is that it default to using the value of FVWM_USERDIR, but both the path and the filename can be changed via FVWM3_LOGFILE.

"fvwm" in their name. Fvwm3 is the successor to fvwm2, which
preceded the 1.x versions of fvwm. This version is simply called
fvwm throughout this document, while the main executable is named
fvwm3.</para>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm OK with this, but we should standardise on Fvwm3 in the future.

-c '<fvwmref cmd="Read"/>
as its initialization file. <envar>$FVWM_USERDIR</envar> can also be used to
change location of default user directory <filename>~/.fvwm</filename>. Another
option is to use -c '<fvwmref cmd="Read"/>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not in favour of advocating -c -- it's one of those options which will go...

which is ~/.fvwm/fvwm3-output.log by default. See ENVIRONMENT section on
how to override this location using <envar>$FVWM_USERDIR</envar> or
<envar>$FVWM3_LOGFILE</envar>.</para>

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again, FVWM_USERDIR is set once, and really shouldn't be updated once Fvwm is running.

- Improve explanation of interaction between env vars FVWM_USERDIR and FVWM3_LOGFILE
- How to dump output with PrintInfo
- Explain absolute vs relative FVWM_USERDIR settings
- Add blurb on FVWM_DATADIR
Copy link
Member

@ThomasAdam ThomasAdam left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

HI @d-e-e-p,

Excellent. This looks good.

Can you please start to squahs a few of these commits into one? I'm also wanting the patch about checking FVWM3_LOGFILE in its own commit, as part of this series, please.

@d-e-e-p
Copy link
Contributor Author

d-e-e-p commented Jul 8, 2020

Tested absolute and relative path settings of FVWM_LOGFILE, and also using the FVWM_USERDIR to override logfile location.
These three options seem to work....

d-e-e-p added 2 commits July 8, 2020 22:19
 - Move default debug log file from $HOME to $FVWM_USERDIR (patch by ThomasAdam)

Documentation updates:
 - How to override default config/log files using env vars FVWM_USERDIR and FVWM3_LOGFILE
 - Using SIGUSR1/SIGUSR2 to toggle logfile writer
 - Update PrintInfo doc to explain how to get output from command
 - Explain absolute vs relative FVWM_USERDIR settings
 - Add blurb on FVWM_DATADIR in ENVIRONMENT section
 - Fixed link to manpage in index.html
 - Cleanup of intro to fvwm section of man page
from fvwm3.
from fvwm3. By default debug log is written to
<envar>$FVWM_USERDIR</envar>/fvwm3-output.log . If an absolute path
is specified (starting with /) then <envar>$FVWM_USERDIR</envar> is
Copy link
Collaborator

@somiaj somiaj Jul 9, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Currently the code is 'contains /' not 'starting with /'. I went back and forth on if I should only do starts with slash vs contains, and went with contains. Probably more standard to do starts with slash, so maybe the code needs to be updated to match this statement.

@d-e-e-p
Copy link
Contributor Author

d-e-e-p commented Jul 11, 2020

@somiaj I would vote for path names that contain but not start with slash to be handled as relative paths. So if FVWM3_LOGFILE is set to logs/fvwm.log and FVWM_USERDIR is not set, log is written to ~/.fvwm/logs/fvwm.log :

Perhaps long term make userdir act the same way?

userdir starts_with_slash  $FVWM_USERDIR
userdir contains_slash     $HOME/$FVWM_USERDIR
userdir no_slash           $HOME/$FVWM_USERDIR

logfile starts_with_slash  $FVWM3_LOGFILE
logfile contains_slash     $FVWM_USERDIR/$FVWM3_LOGFILE
logfile no_slash           $FVWM_USERDIR/$FVWM3_LOGFILE

@ThomasAdam
Copy link
Member

Hi @d-e-e-p,

Interesting thought, but I think this is starting to become over engineered.

The whole point of this is for diagnostics. I really don't want anything more complicated or involved than the patch included in this PR provides. It's such a tiny thing that is rarely going to be used.

We've plenty of other items to turn our attention to yet.

libs/log.c Outdated Show resolved Hide resolved
Co-authored-by: Jaimos <[email protected]>
Comment on lines 60 to +63
path = fxstrdup(expanded_path);
if (expanded_path[0] == '/')
{
path = fxstrdup(expanded_path);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The logic here is incorrect (not to mention this is now causing a memory leak).

We set path to expanded path, and then set path to expanded_path again if expanded_path begins with a /.

This isn't right. Can someone please fix this logic?

@@ -57,6 +58,12 @@ log_open(void)
expanded_path = expand_path(path);

path = fxstrdup(expanded_path);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure why this line is here, it should have been deleted (and only used if starts with /).

Suggested change
path = fxstrdup(expanded_path);

Copy link
Member

@ThomasAdam ThomasAdam Jul 11, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Better, but we need to do a bit more, as in:

diff --git a/libs/log.c b/libs/log.c
index 23a65a70c..977c13c51 100644
--- a/libs/log.c
+++ b/libs/log.c
@@ -47,23 +47,21 @@ log_set_level(int ll)
 void
 log_open(const char *fvwm_userdir)
 {
-       char *path;
+       char *path, *logfile_env;
+
        xasprintf(&path, "%s/%s", fvwm_userdir, FVWM3_LOGFILE_DEFAULT);
 
-       if (getenv("FVWM3_LOGFILE")) {
+       logfile_env = getenv("FVWM3_LOGFILE");
+       if (logfile_env != NULL) {
                const char      *expanded_path;
 
                free(path);
-               path = getenv("FVWM3_LOGFILE");
-               expanded_path = expand_path(path);
+               expanded_path = expand_path(logfile_env);
 
-               path = fxstrdup(expanded_path);
                if (expanded_path[0] == '/')
-               {
-                               path = fxstrdup(expanded_path);
-               } else {
-                               xasprintf(&path, "%s/%s", fvwm_userdir, expanded_path);
-               }
+                       path = fxstrdup(expanded_path);
+               else
+                       xasprintf(&path, "%s/%s", fvwm_userdir, expanded_path);
                free((char *)expanded_path);
        }

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Additionally, this code change in its entirety needs to be split off as a separate commit, and not just shoe-horned in to the other commits as it's nothing to do with these on-going documentation changes.

@d-e-e-p -- are you OK doing that?

@d-e-e-p
Copy link
Contributor Author

d-e-e-p commented Jul 11, 2020

Ok, so plan is to do a separate pull request handling FVWM3_LOGFILE .. perhaps starting with version where document changes have been merged in?

@ThomasAdam
Copy link
Member

Hi @d-e-e-p,

If you separate them out, I'll sort out the order I want to merge them in.

@ThomasAdam ThomasAdam added the type:enhancement Augmenting an existing feature label Jul 11, 2020
@ThomasAdam ThomasAdam self-assigned this Jul 11, 2020
@ThomasAdam ThomasAdam added this to the 1.0 milestone Jul 11, 2020
@d-e-e-p
Copy link
Contributor Author

d-e-e-p commented Jul 13, 2020

I'm not sure how to do multiple fork and pull requests--the 2 repos are at:

git request-pull origin/master https://github.com/d-e-e-p/fvwm3_code
git request-pull origin/master https://github.com/d-e-e-p/fvwm3_doc

@ThomasAdam
Copy link
Member

Hi @d-e-e-p,

Thanks for separating those out, but that has a certain overhead which I didn't have in mind....

What I've done instead is taken your commits from this PR, and for the original commit which had both documentation and code changes, I ran:

git rebase -i master

Chose the commit which had the code changes with documentation, selected to 'edit' the commit, and then ran:

git checkout HEAD~1

To ammend the commit in question, split out the files to separate commits, and continued the rebase.

I then squashed your documentation commits together, and updated the code changes with my additional patch.

The result of all of this is here: ta/deep-docs

@d-e-e-p -- you should be able to checkout that branch and change as necessary, and update your PR here with what's there. But I think what's there looks OK. But do please check it over.

@d-e-e-p
Copy link
Contributor Author

d-e-e-p commented Jul 20, 2020

ta/deep-docs looks good. tested the env var settings with following script.
(completely unrelated, libnson in configure.ac should be libbson)

#!/bin/sh -x

unset FVWM_USERDIR FVWM3_LOGFILE


rm -rf /tmp/flog
mkdir  /tmp/flog

rm -f $HOME/.fvwm/fvwm3-output.log                                              
timeout 10s fvwm3 -r -D -v  >> debug.log  2>&1
ls -l $HOME/.fvwm/fvwm3-output.log

export FVWM_USERDIR=/tmp/flog
timeout 10s fvwm3 -r -D -v  >> debug.log  2>&1
ls -l $FVWM_USERDIR/fvwm3-output.log

export FVWM3_LOGFILE=case1.log
timeout 10s fvwm3 -r -D -v  >> debug.log  2>&1
ls -l $FVWM_USERDIR/$FVWM3_LOGFILE

mkdir -p  /tmp/flog/log/log
export FVWM3_LOGFILE=log/log/case2.log
timeout 10s fvwm3 -r -D -v  >> debug.log  2>&1
ls -l $FVWM_USERDIR/$FVWM3_LOGFILE

export FVWM3_LOGFILE=/tmp/flog/case3.log
timeout 10s fvwm3 -r -D -v  >> debug.log  2>&1
ls -l $FVWM3_LOGFILE 

echo "done"

@ThomasAdam
Copy link
Member

ThomasAdam commented Jul 20, 2020

Hi @d-e-e-p,

Thanks -- will look at merging this soon, unless you have anything else you want to change?

As for libbson spelling, see: #162

@ThomasAdam
Copy link
Member

Thanks all. I've merged ta/deep-docs to master now, so I'm closing this.

Next time, let's try and keep these commits small and relevant to the specific changes being made.

@ThomasAdam ThomasAdam closed this Jul 21, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type:enhancement Augmenting an existing feature
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants