On Thu, Jun 16, 2016 at 04:40:15PM +0200, Pavel Březina wrote: > On 06/16/2016 03:32 PM, Lukas Slebodnik wrote: > > On (16/06/16 10:32), Pavel Březina wrote: > > > On 06/15/2016 09:57 PM, Lukas Slebodnik wrote: > > > > On (15/06/16 21:54), Lukas Slebodnik wrote: > > > > > On (15/06/16 21:08), Lukas Slebodnik wrote: > > > > > > On (15/06/16 19:08), Lukas Slebodnik wrote: > > > > > > > On (15/06/16 14:08), Pavel Březina wrote: > > > > > > > > On 06/15/2016 08:44 AM, Lukas Slebodnik wrote: > > > > > > > > > On (14/06/16 15:30), Pavel Březina wrote: > > > > > > > > > > On 05/16/2016 02:00 PM, Pavel Březina wrote: > > > > > > > > > > > Hi, > > > > > > > > > > > the patches are finally ready to be tested and reviewed. > > > > > > > > > > > It is too huge > > > > > > > > > > > to be sent to the list so please checkout my fedorapeople > > > > > > > > > > > or github repo: > > > > > > > > > > > > > > > > > > > > > > https://fedorapeople.org/cgit/pbrezina/public_git/sssd.git/log/?h=backend > > > > > > > > > > > https://github.com/pbrezina/sssd/tree/backend > > > > > > > > > > > > > > > > > > > > > > Subdomain handlers are not yet converted so subdomain > > > > > > > > > > > support is > > > > > > > > > > > disabled, otherwise everything should work although I'm > > > > > > > > > > > sure you'll find > > > > > > > > > > > some bugs. > > > > > > > > > > > > > > > > > > > > > > I managed to do some simple tests (initgroups, > > > > > > > > > > > authentication) with ldap > > > > > > > > > > > provider so far and will continue testing and fixing so > > > > > > > > > > > if you find a > > > > > > > > > > > bug make sure you run with the latest version before > > > > > > > > > > > reporting it please > > > > > > > > > > > :-) > > > > > > > > > > > > > > > > > > > > > > Since the changes touch almost all areas of SSSD I > > > > > > > > > > > encourage everyone to > > > > > > > > > > > run and try. Some handlers were converted quite easily, > > > > > > > > > > > some took more > > > > > > > > > > > handy work. Areas that are most likely to contain some > > > > > > > > > > > bugs are these > > > > > > > > > > > (please give it extra attention): > > > > > > > > > > > > > > > > > > > > > > - proxy provider > > > > > > > > > > > - if group membership changes during initgroups, nss > > > > > > > > > > > memory cache should > > > > > > > > > > > be clear through dbus call > > > > > > > > > > > - selinux support > > > > > > > > > > > - hbac support > > > > > > > > > > > - change password > > > > > > > > > > > - password migration (ipa) > > > > > > > > > > > > > > > > > > > > > > Don't be alarmed with the number of new lines -- there is > > > > > > > > > > > not that many > > > > > > > > > > > changes. I copied all touched files and suffixed them > > > > > > > > > > > with _new so sssd > > > > > > > > > > > can be compiled and kept working until the latest > > > > > > > > > > > patches. I also wanted > > > > > > > > > > > to keep the original code intact for comparison (it was > > > > > > > > > > > easier for > > > > > > > > > > > development and it may be handy for bug chasing), you can > > > > > > > > > > > simply use > > > > > > > > > > > some diff tool to see the changes. We can squash it in > > > > > > > > > > > the end. > > > > > > > > > > > > > > > > > > > > > > When I will be confident that the patches are stable I > > > > > > > > > > > will do some > > > > > > > > > > > clean up and remove content that is no longer needed. > > > > > > > > > > > > > > > > > > > > Just a quick update: all found bugs were fixed, CI pass, > > > > > > > > > > all downstream tests > > > > > > > > > > which were tried (I think all but IPA) pass. > > > > > > > > > > > > > > > > > > 30 errno_t dp_host_handler(struct sbus_request *sbus_req, > > > > > > > > > 31 void *dp_cli, > > > > > > > > > 32 uint32_t dp_flags, > > > > > > > > > 33 const char *name, > > > > > > > > > 34 const char *alias) > > > > > > > > > 35 { > > > > > > > > > 36 struct dp_hostid_data *data; > > > > > > > > > 37 const char *key; > > > > > > > > > 38 > > > > > > > > > 39 if (name == NULL) { > > > > > > > > > 40 return EINVAL; > > > > > > > > > 41 } > > > > > > > > > 42 > > > > > > > > > 43 data = talloc_zero(sbus_req, struct > > > > > > > > > dp_hostid_data); > > > > > > > > > 44 if (data == NULL) { > > > > > > > > > 45 return ENOMEM; > > > > > > > > > 46 } > > > > > > > > > 47 > > > > > > > > > 48 data->name = name; > > > > > > > > > 49 data->alias = alias[0] == '\0' ? NULL : alias; > > > > > > > > > 50 > > > > > > > > > 51 key = talloc_asprintf("%s:%s", name, (alias == > > > > > > > > > '\0' ? "(null)" : alias)); > > > > > > > > > ^^^ > > > > > > > > > The 1st argument should be a > > > > > > > > > talloc context > > > > > > > > > otherwise it will CRASH. > > > > > > > > > > > > > > > > > > 52 if (key == NULL) { > > > > > > > > > 53 talloc_free(data); > > > > > > > > > 54 return ENOMEM; > > > > > > > > > 55 } > > > > > > > > > > > > > > > > Thanks fixed. > > > > > > > > > > > > > > > > > > > > > > > > > > I can see a gcc warning on my fedora 24 > > > > > > > > > src/providers/ad/ad_subdomains_new.c: In function > > > > > > > > > ‘ad_subdomains_refresh_root_done’: > > > > > > > > > src/providers/ad/ad_subdomains_new.c:575:23: warning: > > > > > > > > > ‘root_attrs’ may be used uninitialized in this function > > > > > > > > > [-Wmaybe-uninitialized] > > > > > > > > > state->root_attrs = root_attrs; > > > > > > > > > ~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~ > > > > > > > > > src/providers/ad/ad_subdomains_new.c:1161:25: note: > > > > > > > > > ‘root_attrs’ was declared here > > > > > > > > > struct sysdb_attrs *root_attrs; > > > > > > > > > ^~~~~~~~~~ > > > > > > > > > src/providers/ad/ad_subdomains_new.c:1194:14: warning: > > > > > > > > > ‘root_id_ctx’ may be used uninitialized in this function > > > > > > > > > [-Wmaybe-uninitialized] > > > > > > > > > subreq = ad_get_slave_domain_send(state, state->ev, > > > > > > > > > state->sd_ctx, > > > > > > > > > > > > > > > > > > ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > > > > > > > > > root_attrs, > > > > > > > > > root_id_ctx->ldap_ctx); > > > > > > > > > > > > > > > > > > ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > > > > > > > > > > > > > > > > I seen this in Coverity also reported with root_attrs maybe > > > > > > > > uninitialized. It > > > > > > > > seems to me as false positive but maybe I miss something? > > > > > > > > > > > > > > > It's verly likely some gcc-6 otimisation. > > > > > > > I will try to take a look. > > > > > > > > > > > > > I think that gcc want to tell us this > > > > > > > > > > > > 1156 static void ad_subdomains_refresh_root_done(struct tevent_req > > > > > > *subreq) > > > > > > 1157 { > > > > > > 1158 struct ad_subdomains_refresh_state *state; > > > > > > 1159 struct tevent_req *req; > > > > > > 1160 struct ad_id_ctx *root_id_ctx; > > > > > > 1161 struct sysdb_attrs *root_attrs; > > > > > > 1162 int dp_error; > > > > > > 1163 errno_t ret; > > > > > > 1164 > > > > > > 1165 req = tevent_req_callback_data(subreq, struct tevent_req); > > > > > > 1166 state = tevent_req_data(req, struct > > > > > > ad_subdomains_refresh_state); > > > > > > 1167 > > > > > > 1168 ret = ad_get_root_domain_recv(state, subreq, &root_attrs, > > > > > > &root_id_ctx); > > > > > > 1169 talloc_zfree(subreq); > > > > > > 1170 if (ret != EOK) { > > > > > > 1171 DEBUG(SSSDBG_CRIT_FAILURE, "Unable to get forest root > > > > > > [%d]: %s\n", > > > > > > 1172 ret, sss_strerror(ret)); > > > > > > 1173 /* We continue to finish sdap_id_op. */ > > > > > > ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ > > > > > > root_attrs and root_id_ctx needn't be initialized > > > > > > after this line. > > > > > > 1174 } > > > > > > 1175 > > > > > > > > > > hmm, > > > > > maybe no. > > > > > > > > > > I tried to initialize variables after DEBUG message and it was not > > > > > enough. > > > > > I still could see a warnings. > > > > > > > > > > After following change I could see warning only for root_id_ctx. > > > > > > > > > > diff --git a/src/providers/ad/ad_subdomains_new.c > > > > > b/src/providers/ad/ad_subdomains_new.c > > > > > index 83539a1..09cc985 100644 > > > > > --- a/src/providers/ad/ad_subdomains_new.c > > > > > +++ b/src/providers/ad/ad_subdomains_new.c > > > > > @@ -1175,7 +1175,9 @@ static void > > > > > ad_subdomains_refresh_root_done(struct tevent_req *subreq) > > > > > > > > > > ret = ad_get_root_domain_recv(state, subreq, &root_attrs, > > > > > &root_id_ctx); > > > > > talloc_zfree(subreq); > > > > > + root_attrs = NULL; > > > > > if (ret != EOK) { > > > > > + root_id_ctx = NULL; > > > > > DEBUG(SSSDBG_CRIT_FAILURE, "Unable to get forest root [%d]: > > > > > %s\n", > > > > > ret, sss_strerror(ret)); > > > > > /* We continue to finish sdap_id_op. */ > > > > > > > > > > I went deeper and compiler does not like macro > > > > > TEVENT_REQ_RETURN_ON_ERROR. > > > > > according to compiler TEVENT_REQ_RETURN_ON_ERROR could return 0 > > > > > > > > > > Here is "version after expansion" of macro TEVENT_REQ_RETURN_ON_ERROR > > > > > diff --git a/src/providers/ad/ad_subdomains_new.c > > > > > b/src/providers/ad/ad_subdomains_new.c > > > > > index 9a2f035..83539a1 100644 > > > > > --- a/src/providers/ad/ad_subdomains_new.c > > > > > +++ b/src/providers/ad/ad_subdomains_new.c > > > > > @@ -969,7 +969,15 @@ static errno_t > > > > > ad_get_root_domain_recv(TALLOC_CTX *mem_ctx, > > > > > struct ad_get_root_domain_state *state = NULL; > > > > > state = tevent_req_data(req, struct ad_get_root_domain_state); > > > > > > > > > > - TEVENT_REQ_RETURN_ON_ERROR(req); > > > > > + enum tevent_req_state TRROEstate; > > > > > + uint64_t TRROEerr; > > > > > + > > > > > + if (tevent_req_is_error(req, &TRROEstate, &TRROEerr)) { > > > > > + if (TRROEstate == TEVENT_REQ_USER_ERROR) { > > > > > + return TRROEerr; > > > > > + } > > > > > + return ERR_INTERNAL; > > > > > + } > > > > > > > > > > *_attrs = talloc_steal(mem_ctx, state->root_domain_attrs); > > > > > *_id_ctx = state->root_id_ctx; > > > > > > > > > > > > > > > and here is diff on top of previous one which makes complier happy > > > > > diff --git a/src/providers/ad/ad_subdomains_new.c > > > > > b/src/providers/ad/ad_subdomains_new.c > > > > > index 83539a1..eab1972 100644 > > > > > --- a/src/providers/ad/ad_subdomains_new.c > > > > > +++ b/src/providers/ad/ad_subdomains_new.c > > > > > @@ -970,10 +970,15 @@ static errno_t > > > > > ad_get_root_domain_recv(TALLOC_CTX *mem_ctx, > > > > > state = tevent_req_data(req, struct ad_get_root_domain_state); > > > > > > > > > > enum tevent_req_state TRROEstate; > > > > > - uint64_t TRROEerr; > > > > > + uint64_t TRROEuint64; > > > > > + errno_t TRROEerr; > > > > > > > > > > - if (tevent_req_is_error(req, &TRROEstate, &TRROEerr)) { > > > > > + if (tevent_req_is_error(req, &TRROEstate, &TRROEuint64)) { > > > > > + TRROEerr = (errno_t) TRROEuint64; > > > > > if (TRROEstate == TEVENT_REQ_USER_ERROR) { > > > > > + if (TRROEerr == 0) { > > > > > + return EINVAL; > > > > > + } > > > > > return TRROEerr; > > > > > > Isn't this change sufficient? > > > > > No because gcc still correctly assume that > > output arguments are not initialized in case of error. > > > > and we do not terminate execution in function > > ad_subdomains_refresh_root_done > > if ad_get_root_domain_recv fails. > > > > > > > } > > > > > return ERR_INTERNAL; > > > > > @@ -1179,6 +1184,8 @@ static void > > > > > ad_subdomains_refresh_root_done(struct tevent_req *subreq) > > > > > DEBUG(SSSDBG_CRIT_FAILURE, "Unable to get forest root [%d]: > > > > > %s\n", > > > > > ret, sss_strerror(ret)); > > > > > /* We continue to finish sdap_id_op. */ > > > > > + root_attrs = NULL; > > > > > + root_id_ctx = NULL; > > > > > } > > > > > > > > > > /* We finish sdap_id_op here since we connect > > > > > > > > > > > > > I forgot to write that another way how to silnce compiler is to > > > > do not declare function ad_get_root_domain_recv as static. > > > > > > So I suppose it tries to inline the function and then screws up? > > > > > yes, > > but compiler is not tottaly wrong. > > > > LS > > Thank you. Do you want to send the patch for the tevent macro then?
nitpick: /sssd/src/providers/data_provider/dp_request_reply.c: In function ‘dp_req_with_reply_step’: /sssd/src/providers/data_provider/dp_request_reply.c:260: warning: declaration of ‘request_key’ shadows a global declaration /usr/include/keyutils.h:104: warning: shadowed declaration is here _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
