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 <s...@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 <s...@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 <chei...@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.
_______________________________________________
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org

Reply via email to