[PATCH] Improve samba-tool testparm and remove unused script

[Jelmer Vernooij]

On Sat, Jan 25, 2014 at 08:44:46AM +1300, Andrew Bartlett wrote:
> On Fri, 2014-01-24 at 20:30 +0100, Jelmer Vernooij wrote:
> > On Sat, Jan 25, 2014 at 08:17:48AM +1300, Andrew Bartlett wrote:
> > > On Fri, 2014-01-24 at 13:28 +0100, Jelmer Vernooij wrote:
> > > > On Fri, Jan 24, 2014 at 09:06:23PM +1300, Andrew Bartlett wrote:
> > > > > From a2b0fbde9d4118bc8bc14f410511436bda83fa3a Mon Sep 17 00:00:00 2001
> > > > > From: Andrew Bartlett <abartlet at samba.org>
> > > > > Date: Tue, 24 Dec 2013 14:06:47 +1300
> > > > > Subject: [PATCH 1/4] script: Remove unused and no-longer-working
> > > > >  extract_allparms.sh
> > > > > 
> > > > > Signed-off-by: Andrew Bartlett <abartlet at samba.org>
> > > > > Signed-off-by: Garming Sam <garming at catalyst.net.nz>
> > > > Reviewed-By: Jelmer Vernooij <jelmer at samba.org>
> > > > 
> > > > > From ff6495e4683d7cecf4fe289c91d930e2c6f5b5ab Mon Sep 17 00:00:00 2001
> > > > > From: Garming Sam <garming at catalyst.net.nz>
> > > > > Date: Fri, 27 Dec 2013 17:09:35 +1300
> > > > > Subject: [PATCH 2/4] testparm: fix --parameter-name failure to convert to
> > > > >  output string
> > > > 
> > > > > Signed-off-by: Garming Sam <garming at catalyst.net.nz>
> > > > > Signed-off-by: Andrew Bartlett <abartlet at samba.org>
> > > > > ---
> > > > >  python/samba/netcmd/testparm.py | 2 +-
> > > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > > 
> > > > > diff --git a/python/samba/netcmd/testparm.py b/python/samba/netcmd/testparm.py
> > > > > index 9251469..dbf79c7 100644
> > > > > --- a/python/samba/netcmd/testparm.py
> > > > > +++ b/python/samba/netcmd/testparm.py
> > > > > @@ -105,7 +105,7 @@ class cmd_testparm(Command):
> > > > >                      lp[section_name].dump(sys.stdout, lp.default_service,
> > > > >                              verbose)
> > > > >                  else:
> > > > > -                    self.outf.write(lp.get(parameter_name, section_name)+"\n")
> > > > > +                    self.outf.write(str(lp.get(parameter_name, section_name))+"\n")
> > > > 
> > > > What this is trying to avoid? Doesn't sys.stdout.write already cast to string?
> > > 
> > > Correct, we get:
> > > 
> > > ERROR(<type 'exceptions.TypeError'>): uncaught exception - can only concatenate list (not "str") to list
> > >   File "bin/python/samba/netcmd/__init__.py", line 175, in _run
> > >     return self.run(*args, **kwargs)
> > >   File "bin/python/samba/netcmd/testparm.py", line 108, in run
> > >     self.outf.write(lp.get(parameter_name, section_name)+"\n")
> > Do we really want to simply call str in that case though? That will result in
> > output that is fairly python specific, rather than similar to smb.conf syntax:
> > 
> > >>> x = ["a", "b", "c"]
> > >>> str(x)
> > "['a', 'b', 'c']"
> > 
> > A test to verify this behaviour would also be nice. :-)
> 
> The docs.py test is what found and will verify this, because it now (in
> the full branch) checks the output against the 'man smb.conf' contents,
> including formatting. 

I'm not sure if I follow. If the docs.py test found and verifies this, does that
mean that the output is not ['a', 'b', 'c'] for the example given above?

> > > > > Signed-off-by: Garming Sam <garming at catalyst.net.nz>
> > > > > Signed-off-by: Andrew Bartlett <abartlet at samba.org>
> > > > > ---
> > > > >  python/samba/netcmd/testparm.py |  2 +-
> > > > >  source4/param/pyparam.c         | 38 ++++++++++++++++++++++++++++++++++++++
> > > > >  2 files changed, 39 insertions(+), 1 deletion(-)
> > > > > 
> > > > > diff --git a/python/samba/netcmd/testparm.py b/python/samba/netcmd/testparm.py
> > > > > index dbf79c7..7825d25 100644
> > > > > --- a/python/samba/netcmd/testparm.py
> > > > > +++ b/python/samba/netcmd/testparm.py
> > > > > @@ -105,7 +105,7 @@ class cmd_testparm(Command):
> > > > >                      lp[section_name].dump(sys.stdout, lp.default_service,
> > > > >                              verbose)
> > > > >                  else:
> > > > > -                    self.outf.write(str(lp.get(parameter_name, section_name))+"\n")
> > > > > +                    lp.dump_a_parameter(sys.stdout, parameter_name, section_name)
> > > > >              else:
> > > > >                  if not suppress_prompt:
> > > > >                      self.outf.write("Press enter to see a dump of your service definitions\n")
> > > > > diff --git a/source4/param/pyparam.c b/source4/param/pyparam.c
> > > > > index 9874006..0fde10f 100644
> > > > > --- a/source4/param/pyparam.c
> > > > > +++ b/source4/param/pyparam.c
> > > > > @@ -288,6 +288,42 @@ static PyObject *py_lp_dump(PyObject *self, PyObject *args)
> > > > >  	Py_RETURN_NONE;
> > > > >  }
> > > > >  
> > > > > +static PyObject *py_lp_dump_a_parameter(PyObject *self, PyObject *args)
> > > > > +{
> > > > > +	PyObject *py_stream;
> > > > > +	char *param_name;
> > > > > +	char *section_name = NULL;
> > > > > +	FILE *f;
> > > > > +	struct loadparm_context *lp_ctx = PyLoadparmContext_AsLoadparmContext(self);
> > > > > +	struct loadparm_service *service;
> > > > > +
> > > > > +	if (!PyArg_ParseTuple(args, "Os|z", &py_stream, &param_name, &section_name))
> > > > > +		return NULL;
> > > > > +
> > > > > +	f = PyFile_AsFile(py_stream);
> > > > > +	if (f == NULL) {
> > > > > +		PyErr_SetString(PyExc_TypeError, "Not a file stream");
> > > > ^^ Doesn't PyFile_AsFile set a PyErr already?
> > > 
> > > It's just a copy of what was added in:
> > > 
> > > commit de3f9e31d34eac6ddc17e298299d5065f9a86e7c
> > > Author: Jelmer Vernooij <jelmer at samba.org>
> > > Date:   Sun Jun 20 13:40:49 2010 +0200
> > > 
> > >     s4-python: Add LoadparmService.dump()
> > I think the extra call to PyErr_SetString() is incorrect too. There could be
> > other reasons why PyFile_AsFile() fails. Sorry for setting a bad example :-(
> Can we put this in, and you get me a correct patch to fix both?
I'm happy to defer fixing the existing one, but we should not be introducing
new code that is wrong.

Cheers,

Jelmer