Bug 8975 - winbindd can't coredump
winbindd can't coredump
Status: RESOLVED FIXED
Product: Samba 3.5
Classification: Unclassified
Component: Winbind
3.5.14
All All
: P5 normal
: ---
Assigned To: Karolin Seeger
Samba QA Contact
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-06-04 16:34 UTC by Matthieu Patou
Modified: 2012-06-13 17:49 UTC (History)
1 user (show)

See Also:


Attachments
Proposed patch to fix the problem, it's for master but apply cleanly on 3.5.x branch (1.48 KB, patch)
2012-06-04 17:33 UTC, Matthieu Patou
obnox: review-
Details
Updated version of the patch (1.74 KB, patch)
2012-06-05 05:52 UTC, Matthieu Patou
obnox: review+
Details
Updated version of the patch with comments (2.33 KB, patch)
2012-06-12 17:45 UTC, Matthieu Patou
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Matthieu Patou 2012-06-04 16:34:26 UTC
This happen if winbindd is build a given prefix (let's say /home/mat/build123/samba) but installed in another directory (let's say /home/run/samba).

Specifying logbasename or log file didn't help.
Comment 1 Matthieu Patou 2012-06-04 17:21:26 UTC
The issue is present also in master branch.
Smbd is not subject to the same issue. The problems lies into the fact that dump_core_setup use the path of the logfile (if defined) or get_dyn_LOGFILEBASE as the basename for the cores directory.

In winbindd this function is called before the parsing of the command line so if a custom configuration file is passed or logbasename or logfile is set on the command line. The net effect of this design is that it's only default value for those two parameters that are used (based on what has been specified during the configuration phase).
Comment 2 Matthieu Patou 2012-06-04 17:33:16 UTC
Created attachment 7621 [details]
Proposed patch to fix the problem, it's for master but apply cleanly on 3.5.x branch

Proposed fix, it calls core_dump_setup after the configuration file and command line have been parsed.
The first core_dump_setup() call is left to allow  to have a coredump on more traditional setup if the failure occur before the configuration has been parsed.
Comment 3 Matthieu Patou 2012-06-05 05:52:34 UTC
Created attachment 7629 [details]
Updated version of the patch

As discussed with Micheal on IRC today, we are now calling dump_core_setup 3 times:

* at the very beginning to be able to catch dump if the configuration permits (dir exists ...)
* after parsing the command line so that location can be altered if builtin values are not valid in the run environment 
* after reading the configuration file so that core folder destination is coherent with the log folder destination
Comment 4 Michael Adam 2012-06-05 08:25:07 UTC
Comment on attachment 7629 [details]
Updated version of the patch

In principle this is as discussed, but the place of the last call to dump_core_setup() is too early:
we need to call that after the first reload_services_file(). At the present stage, only "lp_load_inital_only()" has been called, which is in incomplete loading of the config in certain situations: The loading of the config has been split due to a chicken-and-egg problem in the clustering environment, combined with registry configuration. The initial config sets "clustering = yes" and does _not_ load the registry config. In order to load the full config, we need to establish a connection to ctdbd first (in order to be able to access the clustered registry databasse).
Comment 5 Michael Adam 2012-06-05 08:51:56 UTC
Comment on attachment 7621 [details]
Proposed patch to fix the problem, it's for master but apply cleanly on 3.5.x branch

This is good (except I'd add a blank line.. :-)
Comment 6 Michael Adam 2012-06-05 08:52:45 UTC
Ok.....

after looking more closely, I have to consent, that loading the configuration can currently _not_ change the core path!

The core path is always based on logfilebase, which can be changed via the command line, ("-l" or "--log-basename"). But as of now, there is no configuration parameter to change the log base (only the concrete log file).

I am sorry for the confusion. So your first patch is better after all... :-)
Comment 7 Matthieu Patou 2012-06-10 07:40:18 UTC
(In reply to comment #6)
> Ok.....
> 
> after looking more closely, I have to consent, that loading the configuration
> can currently _not_ change the core path!
> 
In my test it can, as if you start winbindd this way:

./bin/winbindd --log-basename=/tmp
and have in your config 
log file = /var/log/samba.log 

If I call core_dump_setup before the config file is loaded cores will be in /tmp if after they will bin /var/log/

> The core path is always based on logfilebase, which can be changed via the
> command line, ("-l" or "--log-basename"). But as of now, there is no
> configuration parameter to change the log base (only the concrete log file).
> 
> I am sorry for the confusion. So your first patch is better after all... :-)
Comment 8 Matthieu Patou 2012-06-12 08:26:32 UTC
Jeremy, 
Can I have your review too (micheal seems ok), it would be really great if we can ship this patch in our next release (at my work) but for this I need it to be in samba's git tree.
Comment 9 Michael Adam 2012-06-12 08:51:23 UTC
Comment on attachment 7621 [details]
Proposed patch to fix the problem, it's for master but apply cleanly on 3.5.x branch

Ok, after discussing this further, the extended version of the patch is in fact correct. sorry for the noise :-)
Comment 10 Michael Adam 2012-06-12 08:51:34 UTC
Comment on attachment 7629 [details]
Updated version of the patch

Ok, after discussing this further, the extended version of the patch is in fact correct. sorry for the noise :-)
Comment 11 Michael Adam 2012-06-12 08:53:27 UTC
Privet Matthieu,

technically, you as team member providing the patch and me giving review is enough. But if you want to wait for Jeremy's review, that is fine.

May I suggest, that you als do an updated version of the patch that includes the comment (as done in master)?

Cheers - Michael
Comment 12 Matthieu Patou 2012-06-12 09:06:05 UTC
Privet micheal,

If one person is ok then tomorrow I do a merge patch and ask for commit in 3.5.x branch to karolin.

Basically the merge will just bring the comments.
Comment 13 Matthieu Patou 2012-06-12 17:45:47 UTC
Created attachment 7639 [details]
Updated version of the patch with comments
Comment 14 Jeremy Allison 2012-06-13 17:39:25 UTC
Looks good to me. Re-assigning to Karolin for inclusion in 3.5.next and 3.6.next.
Jeremy.
Comment 15 Karolin Seeger 2012-06-13 17:49:44 UTC
Pushed to v3-5-test and v3-6-test.
Closing out bug report.

Thanks!