Fwd: [PATCHES] simple gpo patches fixes

David Mulder dmulder at suse.com
Thu Apr 12 13:27:22 UTC 2018


I've fixed the couple of issues you mentioned and re-pushed. Thanks!

On 04/11/2018 08:34 PM, Douglas Bagnall via samba-technical wrote:
> hi David,
>
> On 12/04/18 07:04, David Mulder via samba-technical wrote:
>> Douglas could you take a look at the current set of patches on:
>>
>> https://github.com/samba-team/samba/pull/154
> I'm skipping patches 1-6, which are still OK.
>
>> From 5fc6676a3e57181e58a981085d224172a8026e9d Mon Sep 17 00:00:00 2001
>> From: David Mulder <dmulder at suse.com>
>> Date: Tue, 10 Apr 2018 15:07:34 -0600
>> Subject: [PATCH 07/11] param: Add python binding for lpcfg_cache_path
> and this one looks good now too, except for one thing...
>
>> --- a/source4/param/pyparam.c
>> +++ b/source4/param/pyparam.c
>> @@ -358,6 +358,27 @@ static PyObject *py_samdb_url(PyObject *self, PyObject *unused)
>>  	return PyStr_FromFormat("tdb://%s/sam.ldb", lpcfg_private_dir(lp_ctx));
>>  }
>>  
>> +static PyObject *py_cache_path(PyObject *self, PyObject *args)
>> +{
>> +	struct loadparm_context *lp_ctx = PyLoadparmContext_AsLoadparmContext(self);
>> +	char *name, *path;
>> +	PyObject *ret;
> These uninitialised pointers should really be initialised to NULL.
> Sorry for not noticing before, and you wouldn't guess by looking
> around pyparam.c, but our coding standard requires this.
>
> In the interests of getting this in I have fixed this for you.
>
>> +
>> +	if (!PyArg_ParseTuple(args, "s", &name)) {
>> +		return NULL;
>> +	}
>> +
>> +	path = lpcfg_cache_path(NULL, lp_ctx, name);
>> +	if (!path) {
>> +		PyErr_Format(PyExc_RuntimeError,
>> +			     "Unable to access cache %s", name);
>> +		return NULL;
>> +	}
>> +	ret = PyStr_FromString(path);
>> +	talloc_free(path);
>> +
>> +	return ret;
>> +}
>>  
>>  static PyMethodDef py_lp_ctx_methods[] = {
>>  	{ "load", py_lp_ctx_load, METH_VARARGS,
>> @@ -394,6 +415,9 @@ static PyMethodDef py_lp_ctx_methods[] = {
>>  	{ "samdb_url", py_samdb_url, METH_NOARGS,
>>  	        "S.samdb_url() -> string\n"
>>  	        "Returns the current URL for sam.ldb." },
>> +	{ "cache_path", py_cache_path, METH_VARARGS,
>> +		"S.cache_path(name) -> string\n"
>> +		"Returns a path in the Samba cache directory." },
>>  	{ NULL }
>>  };
>>  
>>
>> From 7ed5e92f26c4343ebbe03492e0f959557197696c Mon Sep 17 00:00:00 2001
>> From: David Mulder <dmulder at suse.com>
>> Date: Wed, 11 Apr 2018 12:40:18 -0600
>> Subject: [PATCH 08/11] libgpo: gpo_copy_file() shouldn't explicitly call smb1
>>
>> Don't call cli_openx directly to open a file this
>> calls smb1 code explicitly, which fails if we did
>> a multi-protocol negotiate and negotiated smb2+.
>> Use the higher level cli_open() instead.
>>
>> Signed-off-by: David Mulder <dmulder at suse.com>
>> ---
>>  libgpo/gpo_filesync.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/libgpo/gpo_filesync.c b/libgpo/gpo_filesync.c
>> index 6e3efdaf6c1..bf0bb5381fc 100644
>> --- a/libgpo/gpo_filesync.c
>> +++ b/libgpo/gpo_filesync.c
>> @@ -50,7 +50,7 @@ NTSTATUS gpo_copy_file(TALLOC_CTX *mem_ctx,
>>  	int read_size = io_bufsize;
>>  	off_t nread = 0;
>>  
>> -	result = cli_openx(cli, nt_path, O_RDONLY, DENY_NONE, &fnum);
>> +	result = cli_open(cli, nt_path, O_RDONLY, DENY_NONE, &fnum);
> This one seems very sensible, but frankly I have no idea about this
> SMB stuff. Andrew says it is OK, and on this rare occasion I will
> trust him.
>
>>  	if (!NT_STATUS_IS_OK(result)) {
>>  		goto out;
>>  	}
>>
>> From 27bebbd0e2f2061a11f9a707a0b679431e093585 Mon Sep 17 00:00:00 2001
>> From: David Mulder <dmulder at suse.com>
>> Date: Tue, 10 Apr 2018 15:04:53 -0600
>> Subject: [PATCH 09/11] libgpo: Add python bindings for check_refresh_gpo_list
>>
>> Signed-off-by: David Mulder <dmulder at suse.com>
>> ---
>>  libgpo/pygpo.c | 43 +++++++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 43 insertions(+)
>>
>> diff --git a/libgpo/pygpo.c b/libgpo/pygpo.c
>> index ac6e3237a82..e5f63e1f440 100644
>> --- a/libgpo/pygpo.c
>> +++ b/libgpo/pygpo.c
>> @@ -290,6 +290,46 @@ static PyObject* py_ads_connect(ADS *self)
>>  /* Parameter mapping and functions for the GP_EXT struct */
>>  void initgpo(void);
>>  
>> +static PyObject *py_check_refresh_gpo_list(PyObject * self,
>> +					   PyObject * args)
>> +{
>> +	TALLOC_CTX *frame = talloc_stackframe();
>> +	ADS *ads;
>> +	const char *cache_dir;
>> +	struct GROUP_POLICY_OBJECT *gpo_ptr = NULL;
>> +	PyObject *gpo_list;
>> +	PyObject *gpo_obj;
>> +	NTSTATUS status;
> More uninitialised pointers.
>
> In this case it *really* matters for cache_dir, because if a value
> isn't provided by python...
>
>> +	PyObject *ret = NULL;
>> +
>> +	if (!PyArg_ParseTuple(args, "OO|s", &ads, &gpo_list, &cache_dir)) {
> ...it will be left uninitialised here (being after the "|").
>
>> +		goto out;
>> +	}
>> +	gpo_obj = PyList_GetItem(gpo_list, 0);
> You should check for NULL here and exit (PyList_GetItem sets the
> IndexError for you).
>
>> +	gpo_ptr = (struct GROUP_POLICY_OBJECT *)pytalloc_get_ptr(gpo_obj);
>> +
>> +	if (!cache_dir) {
>> +		cache_dir = cache_path(GPO_CACHE_DIR);
>> +		if (!cache_dir) {
>> +			PyErr_SetString(PyExc_MemoryError,
>> +				"Failed to determine gpo cache dir");
> I'm not convinced that MemoryError is the right thing here. The
> failure *could* be a talloc error, but it could also be an I/O error,
> and I suspect the latter is more likely -- but as pygpo.c seems to
> follow this pattern I guess we're stuck.
>
>> +                  goto out;
>> +		}
>> +	}
>> +
>> +	status = check_refresh_gpo_list(ads->ads_ptr, frame, cache_dir, 0,
>> +					gpo_ptr);
>> +	if (!NT_STATUS_IS_OK(status)) {
>> +		PyErr_SetNTSTATUS(status);
>> +		goto out;
>> +	}
>> +
>> +	ret = Py_True;
>> +out:
>> +	TALLOC_FREE(frame);
>> +	return ret;
>> +}
>> +
>>  /* Global methods aka do not need a special pyobject type */
>>  static PyObject *py_gpo_get_sysvol_gpt_version(PyObject * self,
>>  					       PyObject * args)
>> @@ -497,6 +537,9 @@ static PyMethodDef py_gpo_methods[] = {
>>  	{"gpo_get_sysvol_gpt_version",
>>  		(PyCFunction)py_gpo_get_sysvol_gpt_version,
>>  		METH_VARARGS, NULL},
>> +	{"check_refresh_gpo_list",
>> +		(PyCFunction)py_check_refresh_gpo_list,
>> +		METH_VARARGS, NULL},
>>  	{NULL}
>>  };
>>  
>> From 6ffe43e3286a5f938df86b3bbfafbb4c5d9a4efa Mon Sep 17 00:00:00 2001
>> From: David Mulder <dmulder at suse.com>
>> Date: Wed, 11 Apr 2018 12:45:40 -0600
>> Subject: [PATCH 10/11] gpo: python chardet is not a dep of samba
> Good.
>
>> From dfdb5aedbfff632ec9c33763c0d8bda11a4bc654 Mon Sep 17 00:00:00 2001
>> From: David Mulder <dmulder at suse.com>
>> Date: Mon, 8 Jan 2018 07:17:29 -0700
>> Subject: [PATCH 11/11] gpo: Read GPO versions locally, not from sysvol
> And I think this one is OK too.
>
> So I will get Andrew to look at these and hopefully we'll push them
> all except the "refresh_gpo_list" one that needs work and the "read
> gpos locally" one that depends on it.
>
> cheers,
> Douglas
>
>

-- 
David Mulder
SUSE Labs Software Engineer - Samba
dmulder at suse.com
SUSE Linux GmbH, GF: Felix Imend├Ârffer, Jane Smithard, Graham Norton, HRB 21284 (AG N├╝rnberg)




More information about the samba-technical mailing list