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