[PATCH] Improve lib/param handling and remove another unused script

Michael Adam obnox at samba.org
Tue Jan 28 06:42:42 MST 2014


Hi,

I reviewed the patchset, and generally it looks good,
but let me add comments and/or review-tags inline below.

On 2014-01-24 at 21:45 +1300, Andrew Bartlett wrote:
> I realise a 650 patch monster branch is hard to start on, even if each
> part is small and sensible, so I'm hoping to get some momentum by
> posting some smaller and independent parts for review.
> 
> These patches are also part of our branch at
> git://git.catalyst.net.nz/samba.git polished-param2
> 
> Please review/push these small improvements to the loadparm system and
> help us tidy up the docs build.
> 
> Thanks,
> 
> Andrew Bartlett
> 
> -- 
> Andrew Bartlett                       http://samba.org/~abartlet/
> Authentication Developer, Samba Team  http://samba.org
> Samba Developer, Catalyst IT          http://catalyst.net.nz/services/samba
> 

> >From 986711005e4433aed283b871239092ffd64cba6e Mon Sep 17 00:00:00 2001
> From: Garming Sam <garming at catalyst.net.nz>
> Date: Tue, 7 Jan 2014 15:21:42 +1300
> Subject: [PATCH 1/8] param: remove unnecessary checks in dump a parameter and
>  fix an offset bug

Shouldn't these be 2 commits?
One for lpcfg_set_cmdline() and one for dump_a_parameter()?

> Previously, it was possible to compare offsets between
> loadparm_service and loadparm_global.  This meant in some cases
> incorrectly skipping certain parameters.
> 
> In dump_a_parameter we now allow dumping of a parameter by alias name,
> and so this check is not required.
> 
> Signed-off-by: Garming Sam <garming at catalyst.net.nz>
> Signed-off-by: Andrew Bartlett <abartlet at samba.org>
> ---
>  lib/param/loadparm.c     | 8 ++++++--
>  source3/param/loadparm.c | 5 +----
>  2 files changed, 7 insertions(+), 6 deletions(-)
> 
> diff --git a/lib/param/loadparm.c b/lib/param/loadparm.c
> index 07196c9..787e75e 100644
> --- a/lib/param/loadparm.c
> +++ b/lib/param/loadparm.c
> @@ -1575,10 +1575,14 @@ bool lpcfg_set_cmdline(struct loadparm_context *lp_ctx, const char *pszParmName,
>  	lp_ctx->flags[parmnum] |= FLAG_CMDLINE;
>  
>  	/* we have to also set FLAG_CMDLINE on aliases */
> -	for (i=parmnum-1;i>=0 && parm_table[i].offset == parm_table[parmnum].offset;i--) {
> +	for (i=parmnum-1;i>=0
> +	       && parm_table[i].p_class == parm_table[parmnum].p_class
> +	       && parm_table[i].offset == parm_table[parmnum].offset;i--) {
>  		lp_ctx->flags[i] |= FLAG_CMDLINE;
>  	}
> -	for (i=parmnum+1;i<NUMPARAMETERS && parm_table[i].offset == parm_table[parmnum].offset;i++) {
> +	for (i=parmnum+1;i<NUMPARAMETERS
> +	       && parm_table[i].p_class == parm_table[parmnum].p_class
> +	       && parm_table[i].offset == parm_table[parmnum].offset;i++) {
>  		lp_ctx->flags[i] |= FLAG_CMDLINE;

You are adding a check for equality of class
(LOCAL/GLOBAL/SEPARATOR/NONE). It took me a bit to grasp
it: so the offset can be the same for a LOCAL and a GLOBAL
parameter, and if they happen to be neighbors in the param
table, we might illegally turn option A read only
by setting option B on the command line. Nice catch. did
it ever happen?

Apart from that I don't really like the formatting, it is
counter-intuitive to me (Apart from not really adhering to
README.coding... :-). My sugestion would be s/th like:

	for (i = parmnum;
	     i >= 0 &&
	     parm_table[i].p_class == parm_table[parmnum].p_class &&
	     parm_table[i].p_offset == parm_table[parmnum].p_offset;
	     i++)
	{
		lp_ctx->flags[i] |= FLAG_CMDLINE;
	}

But one additional though: 
Here (already previously) the direct neighbors of the current parameter
are checked. Is that assumption always safe? In theory this
aliases need not be neighbors in the parm_table, right?
Shouldn't we play it safe and walk the whole table and move
the class and offset checks into the body of the loop?

>  	}
>  
> diff --git a/source3/param/loadparm.c b/source3/param/loadparm.c
> index 66d7704..bfce05c 100644
> --- a/source3/param/loadparm.c
> +++ b/source3/param/loadparm.c
> @@ -3745,10 +3745,7 @@ bool dump_a_parameter(int snum, char *parm_name, FILE * f, bool isGlobal)
>  
>  	for (i = 0; parm_table[i].label; i++) {
>  		if (strwicmp(parm_table[i].label, parm_name) == 0 &&
> -		    !(parm_table[i].flags & FLAG_META) &&
> -		    (parm_table[i].p_class == p_class || parm_table[i].flags & flag) &&
> -		    (*parm_table[i].label != '-') &&
> -		    (i == 0 || (parm_table[i].offset != parm_table[i - 1].offset))) 
> +		    (parm_table[i].p_class == p_class || parm_table[i].flags & flag))
>  		{
>  			void *ptr;

OK. but I'd suggest a separate patch.

>
> -- 
> 1.8.4.2
> 

> >From 40c93e1496550b2505547cfbd3f8729ec281db65 Mon Sep 17 00:00:00 2001
> From: Garming Sam <garming at catalyst.net.nz>
> Date: Tue, 7 Jan 2014 16:08:50 +1300
> Subject: [PATCH 2/8] docs: update XInclude year to conform with current
>  standard
> 
> This allows the inbuilt python XML parser to handle these includes.
> 
> Signed-off-by: Garming Sam <garming at catalyst.net.nz>
> Signed-off-by: Andrew Bartlett <abartlet at samba.org>
> ---
>  docs-xml/wscript_build | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/docs-xml/wscript_build b/docs-xml/wscript_build
> index a752758..d6ef434 100644
> --- a/docs-xml/wscript_build
> +++ b/docs-xml/wscript_build
> @@ -98,7 +98,7 @@ def smbdotconf_generate_parameter_list(task):
>      parameter_all = task.outputs[0].bldpath(task.env)
>      articles = task.inputs
>  
> -    t = '<section xmlns:xi="http://www.w3.org/2003/XInclude">\n'
> +    t = '<section xmlns:xi="http://www.w3.org/2001/XInclude">\n'
>      for article in articles:
>          t += "<xi:include href='file://" + article.abspath(task.env) + "' parse='xml'/>\n"
>      t += "</section>\n"
> -- 
> 1.8.4.2
> 
Reviewed-by: Michael Adam <obnox at samba.org>


> >From cba9cb1d623f80d7e1e2c57bf9c3df7fcf992ee9 Mon Sep 17 00:00:00 2001
> From: Garming Sam <garming at catalyst.net.nz>
> Date: Tue, 7 Jan 2014 17:09:39 +1300
> Subject: [PATCH 3/8] docs: remove the file prefix from included path names
> 
> This allows the inbuilt python xml modules to perform the include.
> 
> Signed-off-by: Garming Sam <garming at catalyst.net.nz>
> Signed-off-by: Andrew Bartlett <abartlet at samba.org>
> ---
>  docs-xml/wscript_build | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/docs-xml/wscript_build b/docs-xml/wscript_build
> index d6ef434..d1b3ec5 100644
> --- a/docs-xml/wscript_build
> +++ b/docs-xml/wscript_build
> @@ -100,7 +100,7 @@ def smbdotconf_generate_parameter_list(task):
>  
>      t = '<section xmlns:xi="http://www.w3.org/2001/XInclude">\n'
>      for article in articles:
> -        t += "<xi:include href='file://" + article.abspath(task.env) + "' parse='xml'/>\n"
> +        t += "<xi:include href='" + article.abspath(task.env) + "' parse='xml'/>\n"
>      t += "</section>\n"
>      save_file(parameter_all, t , create_dir=True)
>      return 0
> -- 
> 1.8.4.2
> 
Reviewed-by: Michael Adam <obnox at samba.org>


> >From f59619666b23a4a33582b2f613c649a95ce7f817 Mon Sep 17 00:00:00 2001
> From: Garming Sam <garming at catalyst.net.nz>
> Date: Wed, 8 Jan 2014 12:51:26 +1300
> Subject: [PATCH 4/8] lib/param: fix copy service to include the case for
>  P_CHAR
> 
> Signed-off-by: Garming Sam <garming at catalyst.net.nz>
> Signed-off-by: Andrew Bartlett <abartlet at samba.org>
> ---
>  lib/param/loadparm.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/lib/param/loadparm.c b/lib/param/loadparm.c
> index 787e75e..877b9d0 100644
> --- a/lib/param/loadparm.c
> +++ b/lib/param/loadparm.c
> @@ -938,6 +938,10 @@ static void copy_service(struct loadparm_service *pserviceDest,
>  					*(int *)dest_ptr = *(int *)src_ptr;
>  					break;
>  
> +				case P_CHAR:
> +					*(char *)dest_ptr = *(char *)src_ptr;
> +					break;
> +
>  				case P_STRING:
>  					lpcfg_string_set(pserviceDest,
>  						   (char **)dest_ptr,
> -- 
> 1.8.4.2
> 
Reviewed-by: Michael Adam <obnox at samba.org>


> >From f87343ca0bbebc93853bc784d1962b38ba70f049 Mon Sep 17 00:00:00 2001
> From: Garming Sam <garming at catalyst.net.nz>
> Date: Wed, 8 Jan 2014 14:29:36 +1300
> Subject: [PATCH 5/8] param: don't ignore some parameters when performing
>  map_parameter
> 
> Signed-off-by: Garming Sam <garming at catalyst.net.nz>
> Signed-off-by: Andrew Bartlett <abartlet at samba.org>
> ---
>  lib/param/loadparm.c     | 3 ---
>  source3/param/loadparm.c | 3 ---
>  2 files changed, 6 deletions(-)
> 
> diff --git a/lib/param/loadparm.c b/lib/param/loadparm.c
> index 877b9d0..188d19a 100644
> --- a/lib/param/loadparm.c
> +++ b/lib/param/loadparm.c
> @@ -807,9 +807,6 @@ static int map_parameter(const char *pszParmName)
>  {
>  	int iIndex;
>  
> -	if (*pszParmName == '-')
> -		return -1;
> -
>  	for (iIndex = 0; parm_table[iIndex].label; iIndex++)
>  		if (strwicmp(parm_table[iIndex].label, pszParmName) == 0)
>  			return iIndex;
> diff --git a/source3/param/loadparm.c b/source3/param/loadparm.c
> index bfce05c..66e8855 100644
> --- a/source3/param/loadparm.c
> +++ b/source3/param/loadparm.c
> @@ -1919,9 +1919,6 @@ static int map_parameter(const char *pszParmName)
>  {
>  	int iIndex;
>  
> -	if (*pszParmName == '-' && !strequal(pszParmName, "-valid"))
> -		return (-1);
> -
>  	for (iIndex = 0; parm_table[iIndex].label; iIndex++)
>  		if (strwicmp(parm_table[iIndex].label, pszParmName) == 0)
>  			return (iIndex);
> -- 
> 1.8.4.2
> 
Reviewed-by: Michael Adam <obnox at samba.org>

But you might explain a bit more in the commit msg:
This was supposed to protect from something in the past
what changed that makes this protection not necessary any more?
Or put differently, do we have parameters starting with "-" now?



> >From eef6d6ac09eab697a98a7b62b04022edc98c3d92 Mon Sep 17 00:00:00 2001
> From: Andrew Bartlett <abartlet at samba.org>
> Date: Tue, 14 Jan 2014 11:22:16 +1300
> Subject: [PATCH 6/8] docs: Remove find_missing_manpages replaced by docs.py
>  and dependent on a Makefile.in file
> 
> Signed-off-by: Andrew Bartlett <abartlet at samba.org>
> ---
>  docs-xml/Makefile                      |  5 ---
>  docs-xml/scripts/find_missing_manpages | 71 ----------------------------------
>  2 files changed, 76 deletions(-)
>  delete mode 100755 docs-xml/scripts/find_missing_manpages
> 
> diff --git a/docs-xml/Makefile b/docs-xml/Makefile
> index f9c60d0..98b1a71 100644
> --- a/docs-xml/Makefile
> +++ b/docs-xml/Makefile
> @@ -37,7 +37,6 @@ help:
>  	@echo " html - Build multi-file HTML versions"
>  	@echo " html-single - Build single-file HTML versions"
>  	@echo " htmlman - Build HTML version of manpages"
> -	@echo " undocumented - Output list of undocumented smb.conf options"
>  	@echo " samples - Extract examples"
>  
>  $(PDFDIR)/Samba3-ByExample.pdf $(PSDIR)/Samba3-ByExample.ps $(DOCBOOKDIR)/Samba3-ByExample.xml Samba3-ByExample.tex: $(wildcard Samba3-ByExample/*.xml)
> @@ -262,10 +261,6 @@ $(PEARSONDIR)/%.report.html: $(PEARSONDIR)/%.xml
>  %-validate: %/index.xml
>  	cd $(<D) && $(XMLLINT) --xinclude --noent --postvalid --noout $(<F)
>  
> -# Find undocumented parameters
> -undocumented: $(SMBDOTCONFDOC)/parameters.all.xml scripts/find_missing_manpages
> -	$(PERL) scripts/find_missing_manpages $(SRCDIR)/source3
> -
>  samples: $(DOCBOOKDIR)/Samba3-HOWTO.xml xslt/extract-examples.xsl scripts/indent-smb.conf.pl
>  	@mkdir -p examples
>  	$(XSLTPROC) --xinclude xslt/extract-examples.xsl $< > /dev/null 2> examples/README
> diff --git a/docs-xml/scripts/find_missing_manpages b/docs-xml/scripts/find_missing_manpages
> deleted file mode 100755
> index baa5809..0000000
> --- a/docs-xml/scripts/find_missing_manpages
> +++ /dev/null
> @@ -1,71 +0,0 @@
> -#!/usr/bin/python
> -# Copyright (C) 2007,2012 Jelmer Vernooij <jelmer at samba.org>
> -
> -# This program is free software; you can redistribute it and/or modify
> -# it under the terms of the GNU General Public License as published by
> -# the Free Software Foundation; either version 3 of the License, or
> -# (at your option) any later version.
> -#
> -# This program is distributed in the hope that it will be useful,
> -# but WITHOUT ANY WARRANTY; without even the implied warranty of
> -# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> -# GNU General Public License for more details.
> -#
> -# You should have received a copy of the GNU General Public License
> -# along with this program.  If not, see <http://www.gnu.org/licenses/>.
> -#
> -
> -import optparse
> -import os
> -import re
> -
> -parser = optparse.OptionParser("source_dir")
> -
> -(opts, args) = parser.parse_args()
> -
> -invar = False
> -
> -if len(args) == 1:
> -    topdir = args[0]
> -else:
> -    topdir = "."
> -
> -progs = []
> -
> -f = open(os.path.join(topdir, "Makefile.in"), "r")
> -try:
> -    for l in f.readlines():
> -        l = l.strip()
> -        if invar:
> -            invar = (l[-1] == "\\")
> -            progs.extend(l.rstrip("\\").split(" "))
> -        else:
> -            m = re.match("^([^ ]*)_PROGS([0-9]*) = (.*?)([\\\\])$", l)
> -            if m:
> -                progs.extend(m.group(3).split(" "))
> -                invar = (m.group(4) == "\\")
> -            else:
> -                invar = False
> -finally:
> -    f.close()
> -
> -#$progs =~ s/@([^@]+)@//g;
> -#$progs =~ s/\$\(.*?\)//g;
> -
> -for prog in progs:
> -    prog = prog.strip()
> -    if prog == "":
> -        continue
> -    if prog[0] in ("@", "$"):
> -        continue
> -    prog = prog[len("bin/"):]
> -
> -    found = False
> -
> -    for i in range(9):
> -        p = "manpages/%s.%d.xml" % (prog, i)
> -        if os.path.exists(p):
> -            found = True
> -
> -    if not found:
> -        print "'%s' does not have a manpage" % prog
> -- 
> 1.8.4.2
> 
Reviewed-by: Michael Adam <obnox at samba.org>

> >From eeffdc012a62ca0b8d78117a55b847e54df5d963 Mon Sep 17 00:00:00 2001
> From: Andrew Bartlett <abartlet at samba.org>
> Date: Tue, 14 Jan 2014 12:35:25 +1300
> Subject: [PATCH 7/8] docs: Remove unused parameters.global.xml and
>  parameters.service.xml
> 
> Signed-off-by: Andrew Bartlett <abartlet at samba.org>
> ---
>  docs-xml/.gitignore |  2 --
>  docs-xml/Makefile   | 10 +---------
>  2 files changed, 1 insertion(+), 11 deletions(-)
> 
> diff --git a/docs-xml/.gitignore b/docs-xml/.gitignore
> index 6dab9bb..8beaa2d 100644
> --- a/docs-xml/.gitignore
> +++ b/docs-xml/.gitignore
> @@ -11,8 +11,6 @@ configure
>  test.d
>  tmp
>  smbdotconf/parameters.all.xml
> -smbdotconf/parameters.global.xml
> -smbdotconf/parameters.service.xml
>  *.d
>  output/manpages
>  Samba3-ByExample.tex
> diff --git a/docs-xml/Makefile b/docs-xml/Makefile
> index 98b1a71..d9ae92b 100644
> --- a/docs-xml/Makefile
> +++ b/docs-xml/Makefile
> @@ -221,19 +221,11 @@ $(MANPAGEDIR)/smb.conf.5.xml: parameters
>  # any smbdotconf/*/*.xml file ...
>  .PHONY: parameters
>  
> -parameters: $(SMBDOTCONFDOC)/parameters.all.xml $(SMBDOTCONFDOC)/parameters.service.xml $(SMBDOTCONFDOC)/parameters.global.xml
> +parameters: $(SMBDOTCONFDOC)/parameters.all.xml
>  
>  $(SMBDOTCONFDOC)/parameters.all.xml: $(wildcard $(SMBDOTCONFDOC)/*/*.xml) $(SMBDOTCONFDOC)/generate-file-list.sh
>  	$(SMBDOTCONFDOC)/generate-file-list.sh $(SMBDOTCONFDOC) > $@
>  
> -$(SMBDOTCONFDOC)/parameters.global.xml: $(SMBDOTCONFDOC)/parameters.all.xml $(SMBDOTCONFDOC)/generate-context.xsl
> -	@echo "Generating list of global smb.conf options" 
> -	$(XSLTPROC) --xinclude --param smb.context "'G'" --output $(SMBDOTCONFDOC)/parameters.global.xml $(SMBDOTCONFDOC)/generate-context.xsl $<
> -
> -$(SMBDOTCONFDOC)/parameters.service.xml: $(SMBDOTCONFDOC)/parameters.all.xml $(SMBDOTCONFDOC)/generate-context.xsl
> -	@echo "Generating list of share-mode smb.conf options" 
> -	$(XSLTPROC) --xinclude --param smb.context "'S'" --output $(SMBDOTCONFDOC)/parameters.service.xml $(SMBDOTCONFDOC)/generate-context.xsl $<
> -
>  $(OUTPUTDIR):
>  	test -d $@ || mkdir $@
>  
> -- 
> 1.8.4.2
> 
Reviewed-by: Michael Adam <obnox at samba.org>

> >From ca13fc99f62d769ba100c0cc646303a7738c2a85 Mon Sep 17 00:00:00 2001
> From: Andrew Bartlett <abartlet at samba.org>
> Date: Wed, 15 Jan 2014 14:13:13 +1300
> Subject: [PATCH 8/8] lib/param: Fix copy_service to handle BOOLREV
> 
> Signed-off-by: Andrew Bartlett <abartlet at samba.org>
> ---
>  lib/param/loadparm.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/lib/param/loadparm.c b/lib/param/loadparm.c
> index 188d19a..cc5724a 100644
> --- a/lib/param/loadparm.c
> +++ b/lib/param/loadparm.c
> @@ -925,6 +925,7 @@ static void copy_service(struct loadparm_service *pserviceDest,
>  
>  			switch (parm_table[i].type) {
>  				case P_BOOL:
> +				case P_BOOLREV:
>  					*(bool *)dest_ptr = *(bool *)src_ptr;
>  					break;
>  
> -- 
> 1.8.4.2
> 
Reviewed-by: Michael Adam <obnox at samba.org>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 215 bytes
Desc: Digital signature
URL: <http://lists.samba.org/pipermail/samba-technical/attachments/20140128/126f56d9/attachment.pgp>


More information about the samba-technical mailing list