[SSSD] [sssd][patch] SYSDB: sysdb_getnetgr returns ENOENT

Pavel Reichl preichl at redhat.com
Tue Jun 24 11:33:35 UTC 2014


On Mon, 2014-06-23 at 13:50 +0200, Jakub Hrozek wrote:
> On Thu, Jun 19, 2014 at 05:53:37PM +0200, Pavel Reichl wrote:
> > On Tue, 2014-05-27 at 17:01 +0200, Jakub Hrozek wrote:
> > > On Tue, Apr 22, 2014 at 12:02:55PM +0200, Pavel Reichl wrote:
> > > > On Tue, 2014-04-22 at 08:46 +0200, Lukas Slebodnik wrote:
> > > > > On (17/04/14 18:44), Pavel Reichl wrote:
> > > > > >Hello,
> > > > > >
> > > > > >attached patch is the first of many to solve
> > > > > >https://fedorahosted.org/sssd/ticket/1991
> > > > > >
> > > > > >"The return codes of various sysdb operations differ. Some search
> > > > > >operations would return ENOENT if they don't find a matching object some
> > > > > >would return EOK but an empty result list."
> > > > > >
> > > > > >I think it would be best if in case that no results were found both
> > > > > >ENOENT value and 'properly' empty list were returned.
> > > > > >
> > > > > >Thank for opinions or/and review.
> > > > > >
> > > > > >Pavel Reichl
> > > > > 
> > > > > yet another comment.
> > > > > 
> > > > > >From 1e7f69536acfb50e221807b59446ed9fb584bab9 Mon Sep 17 00:00:00 2001
> > > > > >From: Pavel Reichl <preichl at redhat.com>
> > > > > >Date: Wed, 26 Feb 2014 16:52:26 +0000
> > > > > >Subject: [PATCH 1/5] SYSDB: sysdb_getnetgr returns ENOENT
> > > > > >
> > > > > >sysdb_getnetgr did not return ENOENT althought it was expected.
> > > > > >
> > > > > >Resolves:
> > > > > >https://fedorahosted.org/sssd/ticket/1991
> > > > > >---
> > > > > > src/db/sysdb.h        | 1 +
> > > > > > src/db/sysdb_search.c | 7 ++++++-
> > > > > > 2 files changed, 7 insertions(+), 1 deletion(-)
> > > > > >
> > > > > >diff --git a/src/db/sysdb.h b/src/db/sysdb.h
> > > > > >index 5d337ffd8c0f7fbe6808d50538192369102caf17..45d486c3fa32e393ddb0bfe38426f9c2a3611955 100644
> > > > > >--- a/src/db/sysdb.h
> > > > > >+++ b/src/db/sysdb.h
> > > > > >@@ -455,6 +455,7 @@ struct sysdb_netgroup_ctx {
> > > > > >     } value;
> > > > > > };
> > > > > > 
> > > > > >+/* If no group is found ENOENT is returned and result count is set to 0 */
> > > > > > errno_t sysdb_getnetgr(TALLOC_CTX *mem_ctx,
> > > > > >                        struct sss_domain_info *domain,
> > > > > >                        const char *netgroup,
> > > > > >diff --git a/src/db/sysdb_search.c b/src/db/sysdb_search.c
> > > > > >index 60ad61368d92d12bca3833bc08b0673c581ab882..97192115b90b985cb0ff42516cdc02a44a33acc9 100644
> > > > > >--- a/src/db/sysdb_search.c
> > > > > >+++ b/src/db/sysdb_search.c
> > > > > >@@ -827,7 +827,12 @@ errno_t sysdb_getnetgr(TALLOC_CTX *mem_ctx,
> > > > > >     }
> > > > > > 
> > > > > >     *res = talloc_steal(mem_ctx, result);
> > > > > >-    ret = EOK;
> > > > > >+
> > > > > >+    if((*res)->count == 0) {
> > > > >          ^^^^^^
> > > > >     I do not see a reason of using output argument here. You can use local
> > > > >     variable result.
> > > > OK, fixed.
> > > > 
> > > > > >+        ret =  ENOENT;
> > > > > >+    } else {
> > > > > >+        ret = EOK;
> > > > > >+    }
> > > > > > 
> > > > > > done:
> > > > > >     talloc_zfree(tmp_ctx);
> > > > > >-- 
> > > > > >1.8.4.2
> > > > > >
> > > > > 
> > > > > BTW
> > > > > I would expect more reusable solution for ticket #1991 or do you want to add
> > > > > the same snippet to other function?
> > > > 
> > > > Well, actually I don't know yet. I think that there are too many
> > > > different functions, some of them call directly ldap_search others call
> > > > it indirectly. At first I would prefer to concentrate to fix
> > > > implementation to expected behavior and mainly to don't break anything.
> > > > 
> > > > But of course if/when I will see patterns for reusable solution I will
> > > > propose it.
> > > > 
> > > > > In the function sysdb_error_to_errno ldb error LDB_ERR_NO_SUCH_OBJECT
> > > > > is transformed to the ENOENT. Could you explain why it did not work?
> > > > I don't think there's any problem with function sysdb_error_to_errno, it
> > > > converts LDB_SUCCESS to EOK perfectly.
> > > > 
> > > > > 
> > > > > LS
> > > > > _______________________________________________
> > > > > sssd-devel mailing list
> > > > > sssd-devel at lists.fedorahosted.org
> > > > > https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
> > > > 
> > > 
> > > I think the sss_ldb_search utility function (with a unit test) would be
> > > a good idea. We talked with Sumit about releasing beta2 before I vanish
> > > for vacation next week, so no need to hurry. I think it would be fine to
> > > postpone this work even further.
> > > 
> > Hello,
> > 
> > I attached patches reworked as proposed by Lukas.
> > 
> > Thanks,
> > Pavel Reichl
> > 
> > 
> 
> > From 6e2af6d696fd0e731467c50dbb715756a4bcb0a3 Mon Sep 17 00:00:00 2001
> > From: Pavel Reichl <preichl at redhat.com>
> > Date: Tue, 27 May 2014 19:11:20 +0100
> > Subject: [PATCH 1/4] SYSDB: sss_ldb_search - wrapper around ldb_search
> > 
> > Make sure that if no results were found ENOENT is returned rather than just
> > empty list of results.
> > 
> > Resolves:
> > https://fedorahosted.org/sssd/ticket/1991
> > ---
> >  src/db/sysdb.h     |  5 +++++
> >  src/db/sysdb_ops.c | 47 +++++++++++++++++++++++++++++++++++++++++++++++
> >  2 files changed, 52 insertions(+)
> > 
> > diff --git a/src/db/sysdb.h b/src/db/sysdb.h
> > index dd14190bfa156c6b2842e2af81d7b58efa3ea06f..634df049421586959531d8d1a365a2d50d15d1a6 100644
> > --- a/src/db/sysdb.h
> > +++ b/src/db/sysdb.h
> > @@ -480,6 +480,11 @@ int sysdb_get_netgroup_attr(TALLOC_CTX *mem_ctx,
> >                              const char **attributes,
> >                              struct ldb_result **res);
> >  
> > +errno_t sss_ldb_search(struct ldb_context *ldb, TALLOC_CTX *mem_ctx,
> > +                       struct ldb_result **_result, struct ldb_dn *base,
> > +                       enum ldb_scope scope, const char * const *attrs,
> > +                       const char *exp_fmt, ...);
> 
> You need to use SSS_ATTRIBUTE_PRINTF here.

OK, but it didn't like "" being passed as fmt causing:
-Werror=format-zero-length. 

To avoid this warning, NULL is passed instead of "". sss_ldb_search now
handles explicitly case when fmt is NULL because no filter processing is
required then.

> 
> > From b021855c45a60760a7f7af5b6411dd8a41d58e14 Mon Sep 17 00:00:00 2001
> > From: Pavel Reichl <preichl at redhat.com>
> > Date: Thu, 19 Jun 2014 15:38:59 +0100
> > Subject: [PATCH 2/4] TESTS: add tests for sss_ldb_search
> > 
> > Resolves:
> > https://fedorahosted.org/sssd/ticket/1991
> > ---
> 
> [...]
> 
> > +    /* Filter yeilding no results */
> > +    ret = sss_ldb_search(test_ctx->sysdb->ldb, test_ctx, &res, group_dn,
> > +                         LDB_SCOPE_BASE, NULL, "objectClass=user");
> 
> If you want a negative test here, then I think it would be better to use
> a nonexistent objectclass instead of relying on the fact that there are
> no users in sysdb yet. You can make up anything, ldb is schemaless.
> 
OK, although group_dn has value where no user should be I agree that
better be safe than sorry, so "objectClass=nonExistingObjectClass" is
used instead.  


> > +
> > +    fail_unless(ret == ENOENT, "sss_ldb_search error [%d][%s]",
> > +                ret, strerror(ret));
> > +    talloc_zfree(res);
> > +
> > +    /* Non-existing dn */
> > +    ret = sss_ldb_search(test_ctx->sysdb->ldb, test_ctx, &res, nonexist_dn,
> > +                         LDB_SCOPE_BASE, NULL, "");
> > +
> > +    fail_unless(ret == ENOENT, "sss_ldb_search error [%d][%s]",
> > +                ret, strerror(ret));
> > +    talloc_zfree(res);
> > +
> > +    talloc_zfree(nonexist_dn);
> > +    talloc_zfree(group_dn);
> > +    fail_unless(check_leaks_pop(test_ctx) == true, "Memory leak");
> > +}
> > +END_TEST
> 
> > From 657274de1a0f5f7ab787ec641080f5490cef704a Mon Sep 17 00:00:00 2001
> > From: Pavel Reichl <preichl at redhat.com>
> > Date: Wed, 26 Feb 2014 16:52:26 +0000
> > Subject: [PATCH 3/4] SYSDB: sysdb_getnetgr returns ENOENT
> > 
> > Replace call of ldb_search by sss_ldb_search to make sure that ENOENT is
> > returned if no results were found.
> > 
> > Resolves:
> > https://fedorahosted.org/sssd/ticket/1991
> 
> ACK
Unfortunately it is expected by calling side that in case of ENOENT it
is OK to access result->count. It's not easy to completely fix this
expectation (5th patch only partially solves this issue) so function
will continue to support this behavior.

> 
> 
> > From 31837f5bb025e1210dedfc89faf9ab4f489df725 Mon Sep 17 00:00:00 2001
> > From: Pavel Reichl <preichl at redhat.com>
> > Date: Thu, 19 Jun 2014 16:18:12 +0100
> > Subject: [PATCH 4/4] TESTS: sysdb_getnetgr - return ENOENT
> 
> ACK
> _______________________________________________
> sssd-devel mailing list
> sssd-devel at lists.fedorahosted.org
> https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

Patch 5 removes needles check on result->count values if ENOENT was
returned. 

Thanks,

PR
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-SYSDB-sss_ldb_search-wrapper-around-ldb_search.patch
Type: text/x-patch
Size: 3113 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20140624/ef66ddeb/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0002-TESTS-add-tests-for-sss_ldb_search.patch
Type: text/x-patch
Size: 4833 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20140624/ef66ddeb/attachment-0001.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0003-SYSDB-sysdb_getnetgr-returns-ENOENT.patch
Type: text/x-patch
Size: 1892 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20140624/ef66ddeb/attachment-0002.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0004-TESTS-sysdb_getnetgr-return-ENOENT.patch
Type: text/x-patch
Size: 1084 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20140624/ef66ddeb/attachment-0003.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0005-NSS-lookup_netgr_step-don-t-access-result-on-ENOENT.patch
Type: text/x-patch
Size: 2663 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20140624/ef66ddeb/attachment-0004.bin>


More information about the sssd-devel mailing list