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