On (20/04/16 17:21), Jakub Hrozek wrote:
>On Wed, Apr 20, 2016 at 09:59:19AM -0400, Simo Sorce wrote:
>> On Wed, 2016-04-20 at 14:16 +0200, Jakub Hrozek wrote:
>> > On Tue, Apr 05, 2016 at 02:54:10PM -0400, Simo Sorce wrote:
>> > > On Tue, 2016-04-05 at 12:57 -0400, Simo Sorce wrote:
>> > > > Thanks, IIRC the int-instead of enum use is intentional, I will
look
>> > > > at the others.
>> > >
>> > > The last coverity/clang thing is a false positive, but I initialized
>> > > reply to NULL anyway, I expect now it will start complaining of
possible
>> > > NULL dereference :-)
>> > >
>> > > Attached find patches that fixes all other issues (hopefully), one of
>> > > them simply dropped an entire function as it turned out I wasn't
using
>> > > it.
>> > >
>> > > Simo.
>> > >
>> > > --
>> > > Simo Sorce * Red Hat, Inc * New York
>> >
>> > > From 4610b546cb37a150ebaee12559c19a17e422708c Mon Sep 17 00:00:00
2001
>> > > From: Simo Sorce <simo(a)redhat.com>
>> > > Date: Thu, 7 Jan 2016 10:17:38 -0500
>> > > Subject: [PATCH 05/15] Responders: Add support for socket activation
>> >
>> > ACK (visual at this point) with a question - do we want to check
>> > that the fd we received is a UNIX socket using sd_is_socket_unix()?
>> >
>> > The sd_listen_fds() manpage recommends that.
>>
>> If they recommend it we should, yes.
>
>OK, same as with the responder issue, I will prepare a fixup patch and
>ask you to check it before squashing into your patches..
>
>>
>> > > From 3755b157de1309f554a380e58c42c38dcd9cc5aa Mon Sep 17 00:00:00
2001
>> > > From: Simo Sorce <simo(a)redhat.com>
>> > > Date: Wed, 20 Jan 2016 10:33:39 -0500
>> > > Subject: [PATCH 06/15] ConfDB: Add helper function to get
"subsections"
>> > >
>> > > The secrets database will have "subsections", ie sections
that are in the
>> > > "secrets" namespace and look like this:
[secrets/<path>]
>> > >
>> > > This function allows to source any section under secrets/ or under
any
>> > > arbitrary sub-path.
>> > >
>> > > Related:
>> > >
https://fedorahosted.org/sssd/ticket/2913
>
>[...]
>
>> > > +int confdb_get_sub_sections(TALLOC_CTX *mem_ctx,
>> > > + struct confdb_ctx *cdb,
>> > > + const char *section,
>> > > + char ***sections,
>> > > + int *num_sections)
>> > > +{
>> > > + TALLOC_CTX *tmp_ctx = NULL;
>> > > + char *secdn;
>> > > + struct ldb_dn *base = NULL;
>> > > + struct ldb_result *res = NULL;
>> > > + static const char *attrs[] = {"cn", NULL};
>> > > + char **names;
>> > > + int base_comp_num;
>> > > + int num;
>> > > + int i;
>> >
>> > Can you use size_t here so that clang doesn't complain about
"comparison
>> > of integers of different signs: 'int' and 'unsigned
int'" in the for
>> > loop below?
>>
>> meh, ok :-)
>
>Trivial, I can also fix this locally and ask you if it's OK to squash.
>
>>
>> > > + int ret;
>> > > +
>> > > + tmp_ctx = talloc_new(mem_ctx);
>> > > + if (tmp_ctx == NULL) {
>> > > + return ENOMEM;
>> > > + }
>> > > +
>> > > + ret = parse_section(tmp_ctx, section, &secdn, NULL);
>> > > + if (ret != EOK) {
>> > > + goto done;
>> > > + }
>> > > +
>> > > + base = ldb_dn_new(tmp_ctx, cdb->ldb, secdn);
>> > > + if (base == NULL) {
>> > > + ret = ENOMEM;
>> > > + goto done;
>> > > + }
>> > > +
>> > > + base_comp_num = ldb_dn_get_comp_num(base);
>> > > +
>> > > + ret = ldb_search(cdb->ldb, tmp_ctx, &res, base,
LDB_SCOPE_SUBTREE,
>> > > + attrs, NULL);
>> > > + if (ret != LDB_SUCCESS) {
>> > > + ret = EIO;
>> > > + goto done;
>> > > + }
>> > > +
>> > > + names = talloc_zero_array(tmp_ctx, char *, res->count + 1);
>> > > + if (names == NULL) {
>> > > + ret = ENOMEM;
>> > > + goto done;
>> > > + }
>> > > +
>> > > + for (num = 0, i = 0; i < res->count; i++) {
>> > > + const struct ldb_val *val;
>> > > + char *name;
>> > > + int n;
>> > > + int j;
>> >
>> > Every time I see variables declared in a scope in C except loop control
>> > variables I think "This should be a static function of its own"
:-)
>>
>> Should it be in this case ? </lazy>
>
>Not sure, I'll make up my mind when I fix the other trivial issues.
>
>
>[...]
>
>> > > From aa6203a0a6cb1f3ac60428887e77fe176489c3e0 Mon Sep 17 00:00:00
2001
>> > > From: Christian Heimes <cheimes(a)redhat.com>
>> > > Date: Fri, 8 Jan 2016 13:26:22 +0100
>> > > Subject: [PATCH 08/15] Secrets: m4 macros for jansson and http-parser
>> > >
>> > > Prepares autoconf for the new Secrets Provider dependencies
>> > >
>> > > Related:
>> > >
https://fedorahosted.org/sssd/ticket/2913
>> > >
>> >
>> > [...]
>> >
>> > > +PKG_CHECK_MODULES([HTTP_PARSER], [http_parser],
[found_http_parser=yes], [found_http_parser=no])
>> >
>> > There is no pkgconfig for http-parser-devel, so it seems to be this line
>> > is redundant.
>> >
>> > Otherwise ACK.
>>
>> I wonder why this is not failing then ?
>
>Because we find the library using the next AC_CHECK_LIB call. Again, I
>will fixup this locally and send a branch for review later.
We have PKG_CHECK_MODULES and then fallback to AC_CHECK_LIB AC_CHECK_HEADER
on many places. It's not a problem from my POV.
If we would like to simplify it. We could get rid of fallback detection on many
places. And http_parser can provide pkgconfig file in future.
I would prefer to be "kind of consistent" in detection.
But I do not insist on it.
I am not sure if you arguing to keep the checks as they are in my patch
or to remove the pkgconfig based check :)
Simo.
--
Simo Sorce * Red Hat, Inc * New York