On Mon, Sep 09, 2013 at 06:12:48PM +0200, Jakub Hrozek wrote: > On Mon, Sep 09, 2013 at 06:04:06PM +0200, Ondrej Kos wrote: > > On 09/09/2013 05:51 PM, Jakub Hrozek wrote: > > >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@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@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@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 <o...@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 <o...@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. > > >_______________________________________________ > > >sssd-devel mailing list > > >sssd-devel@lists.fedorahosted.org > > >https://lists.fedorahosted.org/mailman/listinfo/sssd-devel > > > > > > > new patches attached > > ACK to both.
Pushed to master. _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel