What is lp_load_ex() in source3/param/loadparm.c?
Andrew Bartlett
abartlet at samba.org
Sun Aug 22 16:15:56 MDT 2010
On Sun, 2010-08-22 at 23:31 +0200, Michael Adam wrote:
> Andrew Bartlett wrote:
> > On Fri, 2010-08-20 at 11:22 +0200, Michael Adam wrote:
> > > Hi Andrew,
> > >
> > > Andrew Bartlett wrote:
> > > > I've been looking over the source3/param code with a view to finishing a
> > > > port of lp_set_cmdline() (by tridge). In doing that I noticed that
> > > > lp_load_ex() isn't called from outside loadparm.c, and could be made
> > > > static.
> > > >
> > > > However, it looks like it was meant to be, or it's a public API that I
> > > > can't find the public definition for. Before I make any foolish
> > > > mistakes, I wonder if someone might happen to be able to fill me in on
> > > > the history here?
> > >
> > > lp_load_ex() is the most flexible lp_load function.
> > > The need for greater flexibility was born with the creation
> > > of clustered samba and registry configuration.
> > > We have wrappers to call it with special parameters:
> > >
> > > lp_load() - standard
> > >
> > > lp_load_initial_only()
> > > wrapper to only load the smb.conf file
> > > without including registry config (globals):
> > > This is called in daemon startup before messaging etc is set
> > > up: if "clustering = yes", we need to connect to the ctdb
> > > daemon before we can load the registry config...
> > >
> > > lp_load_with_registry_shares()
> > > the registry shares are not usually loaded at lp_load() time,
> > > but on demand by the server smbd/services.c .
> > > But for testparm, e.g. we want to load everything, hence
> > > this wrapper.
> > >
> > > Possibly lp_load_ex was called outside of loadparm.c once,
> > > but now that we have these wrappers, it is not any more.
> > >
> > > So I have just pushed a patch to master that makes it static.
> > > Thanks for being that careful!
> >
> > Thanks for checking that over, and for the explaination. Now I'm just
> > wondering what the first_time_only parameter of init_globals() is really
> > for. It seems to be an override, to say that despite the static, please
> > do re-init the globals.
> >
> > In the second of the patches I attach I wondered if this would clarify
> > things. What do you think?
>
> Yeah very much indeed. Lol, it was ridiculous with the inverse logic. ;)
> I pushed this one directly.
Thanks.
> > Also, as I have been reading over the code, I developed these patches to
> > help me understand what each caller wanted. Could you have a look at
> > them for me?
>
> Yes I started, and I really like these simple warppers to
> lp_load very much, especially the default config wrappers.
> I am not quite sure about the "_for_client()" naming scheme
> though. You seem to refer to the false,true,true,false lp_load
> call scheme, but this is also used in nmbd, for instance.
Yeah, I guess another name would be lp_load_without_shares()
> I'd like to go over all callers of lp_load myself and check
> whether all the parameters to lp_load are correct at all
> (there may still be regressions introduced by the introduction
> of registry shares/registry global config), and verify the
> correct wrappers.
>
> I also think that there are some inaccuracies in the patches
> changing callers of lp_load to the new wrappers. Some are simply
> assigned to the wrong commit (not bad but irritating at first
> sight), and in at least two places you changed a call to
> "lp_load_initial_only" which does not load registry globals
> to a full blown lp_load wrapper. I would like to check all
> the instances. But I don't think I will make it tonight. :-)
>
> I would like to hold off pushing the introduction of the
> wrappers until I have gone over the callers in detail, if
> that's ok for you.
I very much agree. The call change to lp_load_initial_only() in
libsmbclient is indeed a bug - sorry about that. In the first
generation of this patch I had incorrectly assumed it was equivalent to
what I later added as lp_load_for_client(), before I re-read your mail.
I've just pushed 'loadparm-changes-lp-set-cmdline' to my git repo, in
case you want to work from there. I agree it's really hard to stare
more than a dozen simple replacements and remain convinced that there
isn't a typo or behaviour change in there, or on the other hand to find
it!
> > I've also included the start of an implementation of lp_set_cmdline()
> > that I've been working on with tridge, in case it can get some comment.
>
> That looks very interesting and useful, indeed!
> I guess the lp_set_cmdline() should be integrated in argv parsing
> in the main functions? The cmdline_override() function in the
> patch is not used (yet). So I guess the patch is not quite
> finished yet?
Correct. One of the problems at the moment is that we call init_globals
too often, and so I started to look at the callers to try and understand
what they wanted.
In looking over all the callers, I strongly suspect a lot of copy and
paste here - for example, winbindd loads the smb.conf with the shares,
and with IPC$ added. I can't see any reason why it would need either.
If we can understand what the callers want better, we can hopefully
start to move the lp_load() calls into the popt code for most of the
utilities like we do in the source4 code. This could make the
lp_set_cmdline much easier to implement. If not, we at least need to
avoid wiping the table with the initialise_globals parameter after we
have set the command line options.
Andrew Bartlett
--
Andrew Bartlett http://samba.org/~abartlet/
Authentication Developer, Samba Team http://samba.org
Samba Developer, Cisco Inc.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 190 bytes
Desc: This is a digitally signed message part
URL: <http://lists.samba.org/pipermail/samba-technical/attachments/20100823/15ab2b6d/attachment.pgp>
More information about the samba-technical
mailing list