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

Stephen Gallagher sgallagh at redhat.com
Mon Aug 10 10:52:11 UTC 2009


-----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.

>> +            /* Queue up another try in 10ms */
>> +            fake_svc->restarts++;
>> +            gettimeofday(&tv, NULL);
>> +            tv.tv_usec += 1000;
>> +            te = tevent_add_timer(fake_svc->mt_ctx->ev, fake_svc, tv,
>> +                                  do_identity_check, fake_svc);
>> +            if (te == NULL) {
>> +                DEBUG(0, ("Failed to add do_identity_check event for [%s]!\n",
>> +                          fake_svc->name));
>> +                sbus_disconnect(conn_ctx);
>> +                return;
>> +            }
>> +            DEBUG(3, ("getIdentity reported NoReply. Retrying.\n"));
> 
> Move this debug statement right at the beginning of the if() body.
> 
> Martin

I put this at the end because it is only true if tevent_add_timer()
succeeded here. (Otherwise we are not retrying, we've just failed).

Thank you for the review.

- -- 
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/

iEYEARECAAYFAkp/+9YACgkQeiVVYja6o6PJKwCgizu18ymzsbeqPMmwiSWf9h5W
qzcAnRqIhjWzJTfoO7jwVr04HNS3MceQ
=4VfB
-----END PGP SIGNATURE-----



More information about the sssd-devel mailing list