Dmitri Pal wrote:
> 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
> > >> <sgall...@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
> > >>>> <sgall...@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.
I think this really is not a question of style. I'd vote for removing
the paragraph from the coding style and not mention it at all. This
should just be left for the programmers discretion. And I don't think
that mixing would have such a profound negative impact that it would not
be worth it. It shouldn't be a dogma you have to follow, but it
certainly helps if you'd need 10 variables in a longer function -- such
a code simply isn't readable.
Martin
_______________________________________________
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://fedorahosted.org/mailman/listinfo/sssd-devel
