[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