On Thu, Jul 07, 2016 at 01:26:13PM +0200, Pavel Březina wrote:
On 07/07/2016 01:24 PM, Jakub Hrozek wrote:
> On Thu, Jul 07, 2016 at 12:39:28PM +0200, Pavel Březina wrote:
> > On 07/07/2016 12:34 PM, Jakub Hrozek wrote:
> > > On Thu, Jul 07, 2016 at 10:16:03AM +0200, Sumit Bose wrote:
> > > > resend
> > > > ----- Forwarded message from Sumit Bose <sbose(a)redhat.com>
-----
> > > >
> > > > Date: Wed, 6 Jul 2016 11:13:48 +0200
> > > > From: Sumit Bose <sbose(a)redhat.com>
> > > > To: sssd-devel(a)lists.fedorahosted.org
> > > > Subject: Re: [SSSD] [PATCH] LDAP: Lookup services by all protocols
unless a
> > > > protocol is specified
> > > > Message-ID:
<20160706091348.GD29143(a)p.Speedport_W_724V_Typ_A_05011603_00_009>
> > > > References: <20160705103025.GB24232@hendrix>
> > > > MIME-Version: 1.0
> > > > Content-Type: text/plain; charset=us-ascii
> > > > Content-Disposition: inline
> > > > In-Reply-To: <20160705103025.GB24232@hendrix>
> > > > User-Agent: Mutt/1.6.1 (2016-04-27)
> > > >
> > > > On Tue, Jul 05, 2016 at 12:30:25PM +0200, Jakub Hrozek wrote:
> > > > > Hi,
> > > > >
> > > > > the attached patch makes service lookups great again.
> > > > >
> > > > > To reproduce, just run:
> > > > > getent service -s sss ldap
> > > > > before the patch we would look up ipService="" because
DP gives us an
> > > > > empty string after the recent DP patches.
> > > >
> > > > > From ba9834637b3cc0d7d98f704ba70f9dcb6f9a70e9 Mon Sep 17
00:00:00 2001
> > > > > From: Jakub Hrozek <jhrozek(a)redhat.com>
> > > > > Date: Tue, 5 Jul 2016 12:23:23 +0200
> > > > > Subject: [PATCH] LDAP: Lookup services by all protocols unless a
protocol is
> > > > > specified
> > > > >
> > > > > The DP refactoring changed the way we handle strings from sbus.
We no
> > > > > longer receive NULL strings, but empty strings instead.
> > > > > ---
> > > > > src/providers/ldap/ldap_id_services.c | 2 +-
> > > > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/src/providers/ldap/ldap_id_services.c
b/src/providers/ldap/ldap_id_services.c
> > > > > index
77215127b53297d840eaa4d2f35a75eedb085e43..db47e3fc55eea969371d61c3c5ac7f818196f3d5 100644
> > > > > --- a/src/providers/ldap/ldap_id_services.c
> > > > > +++ b/src/providers/ldap/ldap_id_services.c
> > > > > @@ -114,7 +114,7 @@ services_get_send(TALLOC_CTX *mem_ctx,
> > > > > ret = sss_filter_sanitize(state, name, &clean_name);
> > > > > if (ret != EOK) goto error;
> > > > >
> > > > > - if (protocol) {
> > > > > + if (protocol && protocol[0] != '\0') {
> > > > > ret = sss_filter_sanitize(state, protocol,
&clean_protocol);
> > > > > if (ret != EOK) goto error;
> > > > > }
> > > >
> > > > since the sysdb calls later on in the request expect protocol==NULL
as
> > > > well I would suggest something like
> > > >
> > > > state->sysdb = sdom->dom->sysdb;
> > > > state->name = name;
> > > > state->protocol = protocol;
> > > > + if (state->protocol[0] == '\0') {
> > > > + state->protocol = NULL;
> > > > + }
> > > > state->filter_type = filter_type;
> > > > state->noexist_delete = noexist_delete;
> > > >
> > > >
> > > > in only use state->protocol in the request.
> > >
> > > OK, done (we use state->protocol up to a point, then clean_protocol).
I
> > > also added a check to cover the unlikely situation where sbus might give
> > > us a NULL string, just to error and don't crash.
> >
> > Hi, nice catch. I think it may be better to amend check_and_parse_filter()
> > in dp_target_id.c so it assign NULL instead of "" in
data->filter_value (and
> > other) using SBUS_SET_STRING(). Though I didn't check if other look up
types
> > (user/group/...) don't expect empty string instead.
>
> Yes, this makes sense and a quick git grep shows that the providers
> expect NULL, not empty string (for example for enumeration which we
> already special-case in check_and_parse_filter). But at this point,
> I would prefer to push this patch, release 1.14.0 and then patch
> check_and_parse_filter() and run tests again to make sure we don't break
> some strange corner case with a late sbus fix.
>
> Attached is a new patch that doesn't error out if protocol is NULL.
Ok, I'm fine with this. The code looks good to me.
Looks like my mail in between got delayed, CI passed
http://sssd-ci.duckdns.org/logs/job/47/43/summary.html
ACK
bye,
Sumit
> _______________________________________________
> sssd-devel mailing list
> sssd-devel(a)lists.fedorahosted.org
>
https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org