[SSSD] [PATCH] DB: Add user/group lookup by SID

Jakub Hrozek jhrozek at redhat.com
Mon Sep 9 15:51:41 UTC 2013


On Mon, Sep 09, 2013 at 02:55:57PM +0200, Ondrej Kos wrote:
> On 09/06/2013 02:12 PM, Jakub Hrozek wrote:
> >On Wed, Sep 04, 2013 at 09:30:37AM +0200, Ondrej Kos wrote:
> >>On 09/03/2013 12:08 AM, Jakub Hrozek wrote:
> >>>On Tue, Aug 27, 2013 at 12:19:39PM +0200, Ondrej Kos wrote:
> >>>>On 08/26/2013 03:53 PM, Jakub Hrozek wrote:
> >>>>>On Mon, Aug 26, 2013 at 02:58:18PM +0200, Ondrej Kos wrote:
> >>>>>>Hi,
> >>>>>>
> >>>>>>Attached patch adds sysdb routine to search users/groups by their
> >>>>>>SID, which will be needed for ticket 1568.
> >>>>>>
> >>>>>>I'm sending it now, because one of the patches I have in this
> >>>>>>working branch (store group SID) was already written and posted on
> >>>>>>the list by Sumit, so not to waste time again :)
> >>>>>>
> >>>>>
> >>>>>There is quite some code duplication between the two functions. Can we
> >>>>>have a single one that would also take a search base and either
> >>>>>objectlass or filter as arguments? The objectclass or filter would then
> >>>>>be and-end with SYSDB_SID_STR=%s. User and group functions could then be
> >>>>>just thin wrappers.
> >>>>>
> >>>>>Also I would prefer a unit test for any new sysdb API.
> >>>>>_______________________________________________
> >>>>>sssd-devel mailing list
> >>>>>sssd-devel at lists.fedorahosted.org
> >>>>>https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
> >>>>>
> >>>>
> >>>>New patch attached.
> >>>
> >>>This is better, but do you need the generic function and the enum
> >>>exposed in the header? Can you make the generic function static and move
> >>>the enum inside the module?
> >>>
> >>>Also, instead of the enum, maybe the function can accept the format
> >>>strings directly and then we wouldn't need the adhoc enum at all.
> >>>_______________________________________________
> >>>sssd-devel mailing list
> >>>sssd-devel at lists.fedorahosted.org
> >>>https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
> >>>
> >>New patch attached
> >>
> >>Ondra
> >
> >Only two more changes:
> >
> >enum sysdb_sid_search_type can be removed, it's not used anywhere.
> oh, i forgot it there
> >
> >sysdb_search_entry_by_sid_str should report failure on a higher DEBUG
> >level. Feel free to also file a ticket or send a patch to fix the other
> >calls. I think in general OP_FAILURE should be much better.
> done, and patch improving debug level of other search functions is
> also attached.
> >_______________________________________________
> >sssd-devel mailing list
> >sssd-devel at lists.fedorahosted.org
> >https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
> >
> 
> 
> -- 
> Ondrej Kos
> Associate Software Engineer
> Identity Management - SSSD
> Red Hat Czech

> From: Ondrej Kos <okos at redhat.com>
> Date: Wed, 21 Aug 2013 15:17:00 +0200
> Subject: [PATCH 1/4] DB: Add user/group lookup by SID

ACK

> From 6fa07dcc178270fbbb2868b58a94c2d814a36408 Mon Sep 17 00:00:00 2001
> From: Ondrej Kos <okos at redhat.com>
> Date: Mon, 9 Sep 2013 14:54:19 +0200
> Subject: [PATCH 2/4] DB: Rise search functions debug levels
> 
> ---
>  src/db/sysdb_ops.c | 36 ++++++++++++++++++------------------
>  1 file changed, 18 insertions(+), 18 deletions(-)
> 
> diff --git a/src/db/sysdb_ops.c b/src/db/sysdb_ops.c
> index 859581aa5b516238db266732a7b9843e23a2edef..bb39ca711b3926f59e14ba73d04f1436c992fae5 100644
> --- a/src/db/sysdb_ops.c
> +++ b/src/db/sysdb_ops.c
> @@ -342,10 +342,10 @@ int sysdb_search_user_by_name(TALLOC_CTX *mem_ctx,
>  
>  done:
>      if (ret == ENOENT) {
> -        DEBUG(SSSDBG_TRACE_FUNC, ("No such entry\n"));
> +        DEBUG(SSSDBG_OP_FAILURE, ("No such entry\n"));

Sorry if I confused you, but when I said "report failure" earlier, I
meant other error codes than ENOENT. ENOENT is very often OK, because
the code would check if an entry exists first before downloading it etc.
In these cases, we shouldn't log at high log level and just let caller
decide if the absence of the entry is a failure or not.

>      }
>      else if (ret) {
> -        DEBUG(SSSDBG_TRACE_FUNC, ("Error: %d (%s)\n", ret, strerror(ret)));
> +        DEBUG(SSSDBG_OP_FAILURE, ("Error: %d (%s)\n", ret, strerror(ret)));

This change is fine.



More information about the sssd-devel mailing list