[PATCH] proxy bug

[Simo Sorce]

This patch "should" fix a bug seen today in the wild concerning the
proxy provider.
I say should because I was not able to reproduce the bug in the first
place, but from the debug I've seen this should fix it.

Simo.

[Sumit Bose]

On Tue, Sep 15, 2009 at 05:49:49PM -0400, Simo Sorce wrote:
> This patch "should" fix a bug seen today in the wild concerning the
> proxy provider.
> I say should because I was not able to reproduce the bug in the first
> place, but from the debug I've seen this should fix it.
>
> Simo.

ACK for 1/2

.....
>    if (debug_level >= 7) {
>        if (!state->grp->gr_mem || !state->grp->gr_mem[0]) {
>            DEBUG(7, ("Group %s has no members!\n",
>                      state->grp->gr_name));
>        } else {
>            int i = 0;
>            while (state->grp->gr_mem[i]) { 
>                /* count */
>                i++;
>            }
>            DEBUG(7, ("Group %s has %d members!\n",
>                      state->grp->gr_name, i));
>        }
>    }

Please make it a macro or a subroutine instead of adding it three times.
>    if (state->grp->gr_mem && state->grp->gr_mem[0]) {
>        members = (const char **)state->grp->gr_mem;
>    } else {
>        members = NULL;
>    }
>    subreq = sysdb_store_group_send(state, state->ev, state->handle,
>                                    state->domain,
>                                    state->grp->gr_name,
>                                    state->grp->gr_gid,
>                                    (const char **)state->grp->gr_mem,
>                                    NULL, NULL);
>                                    members, NULL, NULL);

wouldn't it be better to move the check into sysdb_store_group_send ?

bye,
Sumit

[Simo Sorce]

On Wed, 2009-09-16 at 13:27 +0200, Sumit Bose wrote:
> On Tue, Sep 15, 2009 at 05:49:49PM -0400, Simo Sorce wrote:
> > This patch "should" fix a bug seen today in the wild concerning the
> > proxy provider.
> > I say should because I was not able to reproduce the bug in the first
> > place, but from the debug I've seen this should fix it.
> >
> > Simo.
>
> ACK for 1/2
>
> .....
> >    if (debug_level >= 7) {
> >        if (!state->grp->gr_mem || !state->grp->gr_mem[0]) {
> >            DEBUG(7, ("Group %s has no members!\n",
> >                      state->grp->gr_name));
> >        } else {
> >            int i = 0;
> >            while (state->grp->gr_mem[i]) { 
> >                /* count */
> >                i++;
> >            }
> >            DEBUG(7, ("Group %s has %d members!\n",
> >                      state->grp->gr_name, i));
> >        }
> >    }
>
> Please make it a macro or a subroutine instead of adding it three times.

Yeah, will do.
> >    if (state->grp->gr_mem && state->grp->gr_mem[0]) {
> >        members = (const char **)state->grp->gr_mem;
> >    } else {
> >        members = NULL;
> >    }
> >    subreq = sysdb_store_group_send(state, state->ev, state->handle,
> >                                    state->domain,
> >                                    state->grp->gr_name,
> >                                    state->grp->gr_gid,
> >                                    (const char **)state->grp->gr_mem,
> >                                    NULL, NULL);
> >                                    members, NULL, NULL);
>
> wouldn't it be better to move the check into sysdb_store_group_send ?

No I prefer that callers pass correct values, this way we can change the
way sysdb_store_group_send() behaves in future knowing callers already
behave appropriately.

Simo.

[Simo Sorce]

On Wed, 2009-09-16 at 08:48 -0400, Simo Sorce wrote:
> > Please make it a macro or a subroutine instead of adding it three
>
> times.
>
> Yeah, will do.

Ok new patches attached, with members debug turned into a macro.

Simo.

[Stephen Gallagher]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 09/16/2009 04:53 PM, Simo Sorce wrote:
> On Wed, 2009-09-16 at 08:48 -0400, Simo Sorce wrote:
> > > Please make it a macro or a subroutine instead of adding it three
> >
> > times.
> >
> > Yeah, will do.
>
> Ok new patches attached, with members debug turned into a macro.
>
> Simo.
>
> sssd-devel mailing list
> [e-mail]
> https://fedorahosted.org/mailman/listinfo/sssd-devel

Looks good to me.

Ack.

- -- 
Stephen Gallagher
RHCE 804006346421761

Looking to carve out IT costs?
www.redhat.com/carveoutcosts/
> -----BEGIN PGP SIGNATURE-----
> Version: GnuPG v1.4.9 (GNU/Linux)
> Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org/
>
> iEYEARECAAYFAkqyDnsACgkQeiVVYja6o6O3CQCeJnTnBKr0Gb5ojHF4QTfFDumQ
> Y5UAn3pm4QqfvS2Ktotmd2fhyRc20k7P
> =fhst
> -----END PGP SIGNATURE-----

[Simo Sorce]

On Thu, 2009-09-17 at 06:25 -0400, Stephen Gallagher wrote:
> > Ok new patches attached, with members debug turned into a macro.
> Looks good to me.

Ok pushed both.

Simo.