[SSSD] [PATCH] Fix startup race condition causing backend restarts

Dmitri Pal dpal at redhat.com
Mon Aug 10 14:54:14 UTC 2009


Stephen Gallagher wrote:
> On 08/10/2009 08:36 AM, Dmitri Pal wrote:
> > Martin Nagy wrote:
> >> On Mon, 10 Aug 2009 06:52:11 -0400, Stephen Gallagher
> >> <sgallagh at redhat.com> wrote:
> >>
> >>   
> >>> -----BEGIN PGP SIGNED MESSAGE-----
> >>> Hash: SHA1
> >>>
> >>> On 08/10/2009 03:38 AM, Martin Nagy wrote:
> >>>     
> >>>> On Fri, 07 Aug 2009 13:59:49 -0400, Stephen Gallagher
> >>>> <sgallagh at redhat.com> wrote:
> >>>>       
> >>>>> When the backends start up, the monitor was immediately sending a
> >>>>> getIdentity request. However, as we've added more processing to
> >>>>> the initialization routines over time, we started introducing
> >>>>> latency between when we open the connection and when we're able to
> >>>>> process requests on that connection.
> >>>>>
> >>>>> I've updated the monitor to check for NoReply as an error message
> >>>>> and queue the getIdentity check for retries until the service
> >>>>> answers or one wallclock second (literally, ten tries at 10ms
> >>>>> each) has passed.
> >>>>>         
> >>>> Hi Steven, the patch looks good to me. Ack.
> >>>> I do have some minor suggestions though, apply them at your own
> >>>> discretion.
> >>>>
> >>>>       
> >>>>>  static void identity_check(DBusPendingCall *pending, void *data)
> >>>>> @@ -1862,8 +1886,11 @@ static void
> identity_check(DBusPendingCall *pending, void *data)
> >>>>>      DBusError dbus_error;
> >>>>>      dbus_uint16_t svc_ver;
> >>>>>      char *svc_name;
> >>>>> +    const char *error_name;
> >>>>>      dbus_bool_t ret;
> >>>>>      int type;
> >>>>> +    struct timeval tv;
> >>>>> +    struct tevent_timer *te;
> >>>>>         
> >>>> I would put these two variables closer to where they are needed
> (marked
> >>>> below).
> >>>>
> >>>> [snip]
> >>>>
> >>>>       
> >>>>> +        error_name = dbus_message_get_error_name(reply);
> >>>>> +        if (strcmp(error_name, DBUS_ERROR_NO_REPLY) == 0) {
> >>>>>         
> >>>> Put the 'tv' and 'te' variable declarations here.
> >>>>
> >>>>       
> >>> I generally would, but the coding style within the SSSD prefers
> >>> declaration at the top of the function. If I have misunderstood our
> >>> coding style here, someone please tell me.
> >>>     
> >> You're right. I will send an email proposing to change this in the
> >> coding style.
> >>
> >>   
> > I am not against the change in general but I suggest we do not do it
> now.
> > This is a significant change to the way the code is structured or can be
> > structured.
> > Changing it now IMO is too late. It might make more harm than good.
>
> We're not proposing to go through the code and change what is already
> done. We're proposing that, going forward, we start doing it a new way.

Which will make the code harder to read since the styles will be mixed.
>
> > _______________________________________________
> > sssd-devel mailing list
> > sssd-devel at lists.fedorahosted.org
> > https://fedorahosted.org/mailman/listinfo/sssd-devel
>
>
_______________________________________________
sssd-devel mailing list
sssd-devel at lists.fedorahosted.org
https://fedorahosted.org/mailman/listinfo/sssd-devel

-- 
Thank you,
Dmitri Pal

Engineering Manager IPA project,
Red Hat Inc.


-------------------------------
Looking to carve out IT costs?
www.redhat.com/carveoutcosts/




More information about the sssd-devel mailing list