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

Pavel Reichl preichl at redhat.com
Wed May 21 15:50:03 UTC 2014


On Mon, 2014-05-19 at 13:32 +0200, Lukas Slebodnik wrote:
> On (22/04/14 12:02), 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
> 
> >From a587d4c9013cbf7dd099a9d1c961b4157bea6e05 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.
> 
> We need to move with this patch.
> 
> I wrote tests for some sysdb_search functions [1]. But I realized you
> wrote patch for different function. They are very similar.
> 
> src/db/sysdb_search.c:errno_t sysdb_getnetgr
> errno_t sysdb_getnetgr(TALLOC_CTX *mem_ctx,
>                        struct sss_domain_info *domain,
>                        const char *netgroup,
>                        struct ldb_result **res);
> 
> src/db/sysdb_ops.c:
> int sysdb_search_netgroup_by_name(TALLOC_CTX *mem_ctx,
>                                   struct sss_domain_info *domain,
>                                   const char *name,
>                                   const char **attrs,
>                                   struct ldb_message **msg);
> 
> The part of refactoring should also be to unify these functions
> (if it is possible) or to reuse one function from another.

I've spent some time studying these functions and I really don't feel
like changing the way they are implemented. Both were introduced (even
sysdb_get_netgroup_attr function) in one single commit. So I guess from
begging they were devised to serve different purposes which was
amplified with many patches that were applied mainly to sysdb_getnetgr.

I don't say that your proposal is not a good idea, but I'm afraid that
it is too much effort outside main purpose of #1991.

PS:
I think it would be possible to replace sysdb_get_netgroup_attr as it is used in tests only IIRC.



> Function sysdb_search_netgroup_by_name already returns ENOENT.
> 
> LS
> 
> [1] https://lists.fedorahosted.org/pipermail/sssd-devel/2014-May/019601.html
> _______________________________________________
> sssd-devel mailing list
> sssd-devel at lists.fedorahosted.org
> https://lists.fedorahosted.org/mailman/listinfo/sssd-devel





More information about the sssd-devel mailing list