Add FVWM3_LOGDIR envvar for logging to different file #126
Conversation
There was a problem hiding this comment.
Hi @NsCDE,
Thanks for this! I've left a few comments in-line. You're definitely on the right lines here.
However, we should probably standardise on this a little bit, building on your ideas here.
- We already perform a
$HOMElookup infvwm/fvwm3.cwhere we usegetpwuid()-- so we should centralise that check somewhere inlibs/; - We want to not only expand
~/but$HOMEas well viagetenv()-- just like you have withFVWM3_LOGDIR. - This check will be useful for the logfile, and the check in
fvwm/fvwm3.c.
To that end, I've come up with this pull-request:
... which also includes a man page addition for the environment variable.
I've tested this in FvwmConsole by doing this like:
SetEnv FVWM3_LOGFILE "/tmp/fvwm3.log"
Then pkill -USR2 fvwm3 which starts logging to that file. Then back in FvwmConsole:
UnsetEnv FVWM3_LOGFILE
... then pkill -USR2 fvwm3, and logging resumes from ~/fvwm3-output.log.
Let me know what you think, and thanks again for the pull-request, @NsCDE.
| if (strncmp(path, "~/", 2) == 0) | ||
| { | ||
| /* Remove only ~ we need / between homedir and the rest */ | ||
| memmove(path, path + 1, strlen(path)); |
There was a problem hiding this comment.
Excellent start. This memmove though will overrun if path isn't NUL-terminated. strlen(...) + 1 will fix that.
| pwd = getpwuid(getuid()); | ||
| if (pwd) | ||
| { | ||
| path = strcat(pwd->pw_dir, path); |
There was a problem hiding this comment.
path is allocated via fxstrdup() and needs freeing here, if it's to be allocated in this way, via temporary variable, otherwise we'll leak memory.
There was a problem hiding this comment.
Hi Thomas,
I'm glad you expanded this and got idea for consolidating generally things about homedir, shell paths and repetative usage of getpwuid in fvwm code.
Yesterday, while searching for similart things in the code, I saw fvwm/session.c, libs/fsm.c, and modules/FvwmScript/Instructions.c are calling getpwuid to find pw->pw_dir too. Since you produced the whole centralized .c+.h files for handling canonification of homedir and path in all the code, this can be consolidated too.
For a C language, I am what people here is Balkan like to say "I'm not a gynecologist, but I can take a look". :-) That said, I will be more carefull to free() things in the future. I have one patch for FvwmButtons (window name and indicator) for a long time too, but this can probably wait post 1.0, it needs to be revisited for fvwm3 ...
P. S.
Simply drop this and apply #127
Cheers ...
There was a problem hiding this comment.
Yesterday, while searching for similart things in the code, I saw fvwm/session.c, libs/fsm.c, and modules/FvwmScript/Instructions.c are calling getpwuid to find pw->pw_dir too. Since you produced the whole centralized .c+.h files for handling canonification of homedir and path in all the code, this can be consolidated too.
Well, feel free to transition those callers across to this, too -- no rush though.
For a C language, I am what people here is Balkan like to say "I'm not a gynecologist, but I can take a look". :-)
Heh -- an interesting sentiment there. :)
That said, I will be more carefull to free() things in the future. I have one patch for FvwmButtons (window name and indicator) for a long time too, but this can probably wait post 1.0, it needs to be revisited for fvwm3 ...
Do please open up a pull-request with that patch, against FVWM3 and I'll take a look...
P. S.
Simply drop this and apply #127
Will do. Thanks again.
|
Closing per review comments. Superseded by #127 |
Hi Thomas,
I don't know what was more difficult: to make my 1st PR in life or how to do ~/ substitution.
I have put define in log.h and renamed env variable to FVWM3_LOGDIR as you requested. Please review. I hope this is ok.