Add debug_level/debug_file options #448

sileht · 2017-03-02T08:32:27Z

What does this PR do?

This change allows to change debug_level and debug_file on the fly.

Are there points in the code the reviewer needs to double check?

We can't really debug the early startup of mutt with these new options.

Why was this PR needed?

To solve #445

Does this PR meet the acceptance criteria?
- Documentation created/updated
- All builds are passing
- Added doxygen code documentation
  syntax
- Code follows the style guide
What are the relevant issue numbers?

Closes #445

sileht · 2017-03-02T08:34:28Z

Does the init.h options documentation are enough for this advanced stuff (they are appended at the end of manual), or should I create a new section in the manual ?

gahr · 2017-03-02T08:52:30Z

I think we should be consistent with respect to log rotation. In your current approach:

if debug_file is not set, the default ~/.muttdebug.%d is used and rotated
if debug_file is set, no rotation happen

Wouldn't it be acceptable to have a .%d appended automatically and rotation happening every time the debug is re-initialized?

If we proceed like this, you could have DebugFile default to "~/.muttdebug" and avoid the need for a second debugfilepath variable. This one also leaks memory, although it's not too severe a problem.

sileht · 2017-03-02T09:40:15Z

I have hesitated to do append the '%d' too, so if other agree, I will do it.

guyzmo

so, there's one error that breaks compilation with debug disabled, that shall be fixed ☺

Then I agree with @gahr, the log rotate logic should be applied no matter what, if only to make it less likely to overwrite an existing log file.

I believe an environment variable and/or CLI argument should be used in complement of the setting value for setting up target of startup logs.

A timestamp should be written at the start of any given log file (not directly related to the PR, but as you're at it 😉)

guyzmo · 2017-03-02T11:42:09Z

init.c

@@ -2336,6 +2403,8 @@ static int parse_set (BUFFER *tmp, BUFFER *s, unsigned long data, BUFFER *err)
 	  strfcpy (scratch, tmp->data, sizeof (scratch));
 	  mutt_expand_path (scratch, sizeof (scratch));
 	  *((char **) MuttVars[idx].data) = safe_strdup (scratch);
+      if (mutt_strcmp (MuttVars[idx].option, "debug_file") == 0)
+	    restart_debug ();


build is broken, certainly because when DEBUG is not defined, this line cannot compile.

https://travis-ci.org/neomutt/neomutt/builds/206916932#L483

guyzmo · 2017-03-02T11:44:35Z

init.c

+      {
+	if (*ptr < 0)
+	  *ptr = 0;
+	restart_debug ();


sileht · 2017-03-02T16:29:24Z

timestamps are already added by mutt_debug()

sileht · 2017-03-14T20:53:02Z

@guyzmo all your points have been fixed.

gahr · 2017-03-14T20:59:10Z

init.c

+     safe_fclose(&debugfile);
+   }
+
+   memcpy(&debuglevel, &DebugLevel, sizeof(int));


debuglevel = DebugLevel ?

obviously. done.

gahr · 2017-03-15T08:59:58Z

I'm not sure it's wise to restart debug upon a change in debug_level. Why is that needed?

sileht · 2017-03-28T06:55:12Z

@gahr I have added new logic to not restart debug when just debug_level is changed.

gahr

LGTM

flatcap

The code looks nice, but a simple use case doesn't work: setting a different log file/level in muttrc.

set debug_file = "~/logs/muttdebug"
set debug_level = 3

Now run, mutt -d2

Mutt creates the file ~/.muttdebug0 containing a line: "... debugging at level 2"

Within mutt:

set ?debug_file replies ~/logs/muttdebug
set ?debug_level replies 3

sileht · 2017-03-30T20:11:40Z

@flatcap that was just a logging issue, when the level change nothing was printed, I fixed that.

Also in current implementation, muttrc is loaded after the debug is started, so "set devel_level = 3" will override any mutt -d option.

flatcap · 2017-03-30T22:14:06Z

OK, that's better.

muttrc is loaded after the debug is started

Hmm... Is that what we want? I'd expect the command line to have a higher priority than the config file.
(I suppose ideally, we'd have a debugfile command line option too)

ATM, if I run mutt -d 2 then mutt creates ~/muttdebug0 before it reads the set debug_file line in my config. I can't prevent it creating that file.

sileht · 2017-03-31T06:06:47Z

Hmm... Is that what we want? I'd expect the command line to have a higher priority than the config file. (I suppose ideally, we'd have a debugfile command line option too)

I would prefer to have command line that override muttrc, too.

ATM, if I run `mutt -d 2` then mutt creates `~/muttdebug0` before it reads the `set debug_file` line in my config. I can't prevent it creating that file.

I have kept that to allow to debug the init as earlier as possible, especially to have the muttrc parsing. But adding a new command line option to override debug_file on startup will solve that. So to resume my next version: * setting only debug* in muttrc, will start the debug when the line is read from the config file. * Setting debug* on command line will start the debug at init beginning. * Setting both, command line always wins.

flatcap · 2017-03-31T13:49:35Z

I have kept that to allow to debug the init as earlier as possible, especially to have the muttrc parsing

Ah, I see.

Hmm... does the file parsing actually have any debugging?
Aren't the errors just printed to stderr?

NeoMutt/20170306 (1.8.0) debugging at level 2
getdnsdomainname(): flatcap.org
Reading configuration file '/home/mutt/.muttrc'.
Reading configuration file '/home/mutt/.mutt/lists.rc'.
In mutt_reflow_windows
Reading configuration file '/home/mutt/.mutt/alternates.rc'.
Reading configuration file '/home/mutt/.mutt/crypto.rc'.
Reading configuration file '/home/mutt/.mutt/sidebar-dev.rc'.
In mutt_reflow_windows
In mutt_reflow_windows
In mutt_reflow_windows
Reading configuration file '/home/mutt/.mutt/imap.rc'.
Reading configuration file '/home/mutt/.mutt/mailbox.rc'.
Reading configuration file '/home/mutt/.mutt/colours-dev.rc'.
Reading configuration file '/home/mutt/.mutt/status-color.rc'.
Reading configuration file '/home/mutt/.muttrc.local'.
NeoMutt/20170306 (1.8.0) stop debugging

Could we do something like the linux kernel at startup? It stores its messages in a ring buffer, that gets dumped as soon as there's a tty.
Hmm... too complicated and if mutt crashes nothing will get logged.

</thinking>

So to resume my next version:

Great! Thanks.

flatcap · 2017-05-12T16:04:53Z

Merged in 40f81d1. Thanks.

ThomasAdam added the status:backlog label Mar 2, 2017

sileht force-pushed the sileht/debug branch from 0bee24d to d0a5fe4 Compare March 2, 2017 08:34

guyzmo suggested changes Mar 2, 2017

View reviewed changes

sileht force-pushed the sileht/debug branch from d0a5fe4 to bed6d90 Compare March 2, 2017 16:31

flatcap added status:in-progress and removed status:backlog labels Mar 7, 2017

gahr requested changes Mar 14, 2017

View reviewed changes

sileht force-pushed the sileht/debug branch from bed6d90 to afddfbc Compare March 14, 2017 21:11

sileht force-pushed the sileht/debug branch from afddfbc to 89bb6d7 Compare March 16, 2017 07:27

flatcap force-pushed the master branch from 10290f0 to b8622bb Compare March 17, 2017 00:03

sileht force-pushed the sileht/debug branch from 89bb6d7 to 952dc88 Compare March 28, 2017 06:25

sileht force-pushed the sileht/debug branch from 952dc88 to 1dfac7d Compare March 28, 2017 07:00

gahr approved these changes Mar 29, 2017

View reviewed changes

flatcap requested changes Mar 30, 2017

View reviewed changes

sileht force-pushed the sileht/debug branch from 1dfac7d to f6f713b Compare March 30, 2017 20:09

sileht force-pushed the sileht/debug branch 3 times, most recently from 48f03ff to 9d672bb Compare March 31, 2017 09:55

sileht force-pushed the sileht/debug branch from 9d672bb to 8ff88cb Compare April 10, 2017 07:21

sileht force-pushed the sileht/debug branch from 8ff88cb to 9ae8b94 Compare April 17, 2017 16:42

sileht force-pushed the sileht/debug branch 2 times, most recently from 6dd8276 to a730c99 Compare May 2, 2017 06:00

flatcap closed this May 12, 2017

flatcap force-pushed the sileht/debug branch from a730c99 to 40f81d1 Compare May 12, 2017 16:04

flatcap removed the status:in-progress label May 12, 2017

sileht deleted the sileht/debug branch November 16, 2018 06:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add debug_level/debug_file options #448

Add debug_level/debug_file options #448

sileht commented Mar 2, 2017

sileht commented Mar 2, 2017

gahr commented Mar 2, 2017 •

edited

sileht commented Mar 2, 2017

guyzmo left a comment

guyzmo Mar 2, 2017

guyzmo Mar 2, 2017

sileht commented Mar 2, 2017

sileht commented Mar 14, 2017

gahr Mar 14, 2017

sileht Mar 14, 2017

gahr commented Mar 15, 2017

sileht commented Mar 28, 2017 •

edited

gahr left a comment

flatcap left a comment

sileht commented Mar 30, 2017

flatcap commented Mar 30, 2017

sileht commented Mar 31, 2017 via email •

edited

flatcap commented Mar 31, 2017

flatcap commented May 12, 2017

Add debug_level/debug_file options #448

Add debug_level/debug_file options #448

Conversation

sileht commented Mar 2, 2017

sileht commented Mar 2, 2017

gahr commented Mar 2, 2017 • edited

sileht commented Mar 2, 2017

guyzmo left a comment

Choose a reason for hiding this comment

guyzmo Mar 2, 2017

Choose a reason for hiding this comment

guyzmo Mar 2, 2017

Choose a reason for hiding this comment

sileht commented Mar 2, 2017

sileht commented Mar 14, 2017

gahr Mar 14, 2017

Choose a reason for hiding this comment

sileht Mar 14, 2017

Choose a reason for hiding this comment

gahr commented Mar 15, 2017

sileht commented Mar 28, 2017 • edited

gahr left a comment

Choose a reason for hiding this comment

flatcap left a comment

Choose a reason for hiding this comment

sileht commented Mar 30, 2017

flatcap commented Mar 30, 2017

sileht commented Mar 31, 2017 via email • edited

flatcap commented Mar 31, 2017

flatcap commented May 12, 2017

gahr commented Mar 2, 2017 •

edited

sileht commented Mar 28, 2017 •

edited

sileht commented Mar 31, 2017 via email •

edited