On Mon, Jun 30, 2014 at 10:17:48AM +0200, Sumit Bose wrote:
> On Sat, Jun 28, 2014 at 01:30:36AM -0400, Yassir Elley wrote:
> >
> >
> > ----- Original Message -----
> > > On (25/06/14 23:33), Yassir Elley wrote:
> > > >
> > > >I have attached a revised first patch which addresses the code review
> > > >comments. I'm not sure if I needed to (since it has already been ACK'ed),
> > > >but I have also attached an unchanged second patch.
> > > >
> > > >Thanks,
> > > >Yassir.
> > >
> > > I know that Sumit already ACKed these patches, but I used them to test
> > > CI script and I found some minor issues
> > > + warnings from static analysers (as usual :-)
> > >
> > > >From b58f39645872bc1dd878ac650861af89964101f9 Mon Sep 17 00:00:00 2001
> > > >From: Yassir Elley <yel...@redhat.com>
> > > >Date: Wed, 21 May 2014 15:11:36 -0400
> > > >Subject: [PATCH 1/2] AD-GPO: Add gpo-smb implementation in gpo_child
> > > >process
> > > >
> > > >---
> > > > Makefile.am | 22 +
> > > > contrib/sssd.spec.in | 2 +-
> > > > src/external/samba.m4 | 8 +
> > > > src/providers/ad/ad_gpo.c | 887
> > > > +++++++++++++++++++++++++++++++++++++++-
> > > > src/providers/ad/ad_gpo_child.c | 637 +++++++++++++++++++++++++++++
> > > > 5 files changed, 1543 insertions(+), 13 deletions(-)
> > > > create mode 100644 src/providers/ad/ad_gpo_child.c
> > > >
> > > >diff --git a/Makefile.am b/Makefile.am
> > > >index
> > > >83999f3584fe130320ebfa6ea459f5706992c6a4..6cf5d8cb0c7c68314dad49461e7cd1fb4452bc1b
> > > >100644
> > > >--- a/Makefile.am
> > > >+++ b/Makefile.am
> > > >@@ -121,6 +121,9 @@ endif
> > > > if BUILD_IFP
> > > > sssdlibexec_PROGRAMS += sssd_ifp
> > > > endif
> > > >+if BUILD_SAMBA
> > > >+sssdlibexec_PROGRAMS += gpo_child
> > > >+endif
> > > >
> > > >
> > > > if BUILD_PAC_RESPONDER
> > > >@@ -2298,6 +2301,25 @@ ldap_child_LDADD = \
> > > > $(DHASH_LIBS) \
> > > > $(KRB5_LIBS)
> > > >
> > > >+gpo_child_SOURCES = \
> > > >+ src/providers/ad/ad_gpo_child.c \
> > > >+ src/util/atomic_io.c \
> > > >+ src/util/util.c \
> > > >+ src/util/signal.c
> > > >+gpo_child_CFLAGS = \
> > > >+ $(AM_CFLAGS) \
> > > >+ $(POPT_CFLAGS) \
> > > >+ $(KRB5_CFLAGS) \
> > > >+ $(INI_CONFIG_CFLAGS) \
> > > >+ $(SMBCLIENT_CFLAGS)
> > > >+gpo_child_LDADD = \
> > > >+ libsss_debug.la \
> > > >+ $(TALLOC_LIBS) \
> > > >+ $(POPT_LIBS) \
> > > >+ $(DHASH_LIBS) \
> > > >+ $(INI_CONFIG_LIBS) \
> > > >+ $(SMBCLIENT_LIBS)
> > > >+
> > > > proxy_child_SOURCES = \
> > > > src/providers/proxy/proxy_child.c \
> > > > src/providers/data_provider_iface_generated.c \
> > > >diff --git a/contrib/sssd.spec.in b/contrib/sssd.spec.in
> > > >index
> > > >b5e7903713ccc6944fbed4f84c2f0413a257b760..10704ae48cf5636de2a5cbbb42fc00b99052144f
> > > >100644
> > > >--- a/contrib/sssd.spec.in
> > > >+++ b/contrib/sssd.spec.in
> > > >@@ -97,7 +97,7 @@ BuildRequires: libtdb-devel
> > > > BuildRequires: libldb-devel
> > > > BuildRequires: libdhash-devel >= 0.4.2
> > > > BuildRequires: libcollection-devel
> > > >-BuildRequires: libini_config-devel
> > > >+BuildRequires: libini_config-devel >= 1.1
> > > > BuildRequires: dbus-devel
> > > > BuildRequires: dbus-libs
> > > > %if 0%{?rhel5_minor} >= 7
> > > >diff --git a/src/external/samba.m4 b/src/external/samba.m4
> > > >index
> > > >735cc5a187dba7514a9c7436a6d3ba948ad522b0..bf790365468da5ccdd40a260d74893490b7e7be8
> > > >100644
> > > >--- a/src/external/samba.m4
> > > >+++ b/src/external/samba.m4
> > > >@@ -19,4 +19,12 @@ If you do not want to build these providers it is
> > > >possible to build SSSD
> > > > without them. In this case, you will need to execute configure script
> > > > with argument --without-samba
> > > > ]]))
> > > >+
> > > >+ PKG_CHECK_MODULES(INI_CONFIG, ini_config >= 1.0.0, ,
> > > ^^^^^
> > > 1.1.0
> > > You used correct version in spec file.
> > > configure will pass with old version of ding-libs and compilation fail in
> > > gpo_child.
> > >
> > >
> > > >+ AC_MSG_ERROR([[Please install libini_config development
> > > >libraries.
> > > >+libini_config libraries are necessary for building ad and ipa provider.
> > > ^^^^^^^^^
> > > You touch this code. I might be
> > > good
> > > to
> > > slightly modify text. "ad with
> > > gpo"
> > > or better wording.
> > > Sorry, for nitpicking. I would ignore it there was not problem with
> > > version.
> > >
> > > >+If you do not want to build these providers it is possible to build SSSD
> > > >+without them. In this case, you will need to execute configure script
> > > >+with argument --without-samba
> > > >+ ]]))
> > > > fi
> > >
> > > //snip
> > >
> > > >+/* == ad_gpo_process_cse_send/recv helpers
> > > >================================= */
> > > >+
> > > >+static errno_t
> > > >+create_cse_send_buffer(TALLOC_CTX *mem_ctx,
> > > >+ char *smb_uri,
> > > >+ struct io_buffer **io_buf)
> > > >+{
> > > >+ struct io_buffer *buf;
> > > >+ size_t rp;
> > > >+ int smb_uri_length;
> > > >+
> > > >+ smb_uri_length = strlen(smb_uri);
> > > >+
> > > >+ DEBUG(SSSDBG_TRACE_FUNC, "smb_uri: %s\n", smb_uri);
> > > >+ DEBUG(SSSDBG_TRACE_FUNC, "strlen(smb_uri): %d\n", smb_uri_length);
> > > >+
> > > >+ buf = talloc(mem_ctx, struct io_buffer);
> > > >+ if (buf == NULL) {
> > > >+ DEBUG(SSSDBG_CRIT_FAILURE, "talloc failed.\n");
> > > >+ return ENOMEM;
> > > >+ }
> > > >+
> > > >+ buf->size = 1 * sizeof(uint32_t);
> > > >+ if (smb_uri) {
> > > I thing that this test is redundant, because strlen would crash
> > > anyway
> > > with NULL.
> > >
> > > Error: REVERSE_INULL
> > > sssd-1.11.92/src/providers/ad/ad_gpo.c:2792: deref_ptr_in_call:
> > > Dereferencing
> > > pointer "smb_uri".
> > > sssd-1.11.92/src/providers/ad/ad_gpo.c:2804: check_after_deref:
> > > Null-checking
> > > "smb_uri" suggests that it may be null, but it has already been
> > > dereferenced
> > > on all paths leading to the check.
> > >
> > > >+ buf->size += smb_uri_length;
> > > >+ }
> > > >+
> > > >+ DEBUG(SSSDBG_TRACE_ALL, "buffer size: %zu\n", buf->size);
> > > >+
> > > >+ buf->data = talloc_size(buf, buf->size);
> > > >+ if (buf->data == NULL) {
> > > >+ DEBUG(SSSDBG_CRIT_FAILURE, "talloc_size failed.\n");
> > > >+ talloc_free(buf);
> > > >+ return ENOMEM;
> > > >+ }
> > > >+
> > > >+ rp = 0;
> > > >+ /* smb_uri */
> > > >+ if (smb_uri) {
> > > >+ SAFEALIGN_SET_UINT32(&buf->data[rp], smb_uri_length, &rp);
> > > >+ safealign_memcpy(&buf->data[rp], smb_uri, smb_uri_length, &rp);
> > > >+ } else {
> > > >+ SAFEALIGN_SET_UINT32(&buf->data[rp], 0, &rp);
> > > >+ }
> > > >+
> > > >+ *io_buf = buf;
> > > >+ return EOK;
> > > >+}
> > >
> > > I am attaching another html report from clang.
> > >
> > > I let final ACK for Sumit.
> > >
> > > LS
> > >
> >
> > Revised patches attached.
> >
> > Regards,
> > Yassir.
>
> ACK
>
> bye,
> Sumit
Great work, thank you!
Pushed to master:
19d3aba12c70528708be9440aca66038a291f29e
d3ca320a1ddea52fe86c052dd5521b8f98bb4f9f
_______________________________________________
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
