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.
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).
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.
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 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 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.. :-)
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... :-)
(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... :-)
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 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 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 :-)
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
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.
Created attachment 7639 [details] Updated version of the patch with comments
Looks good to me. Re-assigning to Karolin for inclusion in 3.5.next and 3.6.next. Jeremy.
Pushed to v3-5-test and v3-6-test. Closing out bug report. Thanks!