On Thu, 03 Dec 2009 15:16:48 +0000
Alan Maguire <Alan.Maguire at Sun.COM> wrote:
> events.c:
>
> I found some of the conditional expressions
> here confusing (I suspect you may have collapsed
> some of them to reduce indenting, which makes sense,
> but the logic is a bit harder to follow). A comment explaining
> each of the more complex ones, or an expansion on the overall
> strategy described on ln560 would be helpful I think.
I assume you mean in nwamd_event_dequeue()? I'll add comments and try
to make things clearer.
>
> 580: we don't seem to initialize waittil unless nsec != 0.
> We can end up here if nsec == 0 and there's
> something on the event queue (event != NULL). I realise
> the guard ensures that we don't enter the loop body if
> event != NULL, but I think it'd read better if we
> unconditionally set waittill (since in the nsec == 0
> case this just results in waittil being equal to the
> result of clock_gettime()).
I've got a better version. I think it is actually correct and reads
better.
>
> 587: the guard of the while loop is pretty complex. What
> I read it boiling down to is that we loop until we get
> an event that has an event time that has expired or
> if we do not get an event and the time we are required
> to wait until has elapsed. By unconditionally setting
> waittil the guard could be slightly simplified to something
> like the following (I think the comment would be useful
> too):
>
> /*
> * Loop until we get an event with an event time that
> * has expired, or until the time specified by nsec has
> * elapsed.
> */
>
> while ((event == NULL || !in_past(event->event_time)) &&
> !in_past(waittil)) {
If nsec is 0 then we have to wait forever. We could set waittil to
some large value but that is gross and not needed.
>
> ...
>
> 609: if the event isn't in the past (hasn't expired yet)
> we set event to NULL here. I think the following would read
> a bit better:
>
> if (event != NULL && in_past(event->event_time)) {
> uu_list_remove(event_queue, event);
> uu_list_node_fini(event, &event->event_node, event_pool);
> } else {
> event = nwamd_event_init(NWAM_EVENT_TYPE_QUEUE_QUIET,
> NWAM_OBJECT_TYPE_UNKNOWN, 0, NULL);
> }
> ...
I agree.
I'll put out a new webrev in a bit after I've tested some.
Michael
>
>
> Alan
>
> Anurag S. Maskey wrote:
> >
> >
> > Renee Danson Sommerfeld wrote:
> >> On Mon, Nov 30, 2009 at 02:02:52AM -0800, Michael Hunter wrote:
> >>
> >>> /net/coupe.eng/builds/mph/nwam1-cr-fixes/webrev
> >>>
> >>
> >> Note: I'm assuming webrev path is actually
> >>
> >> /net/coupe.eng/builds/mph/nwam1_cr_fixes/webrev/
> >>
> >>
> > I looked through the changes and noticed some things. Then, I saw
> > that Renee has already mentioned those things. Having one event queue
> > really simplifies the implementation.
> >
> > Anurag
> >
> >
> >> (underscores instead of hyphens)
> >>
> >>
> >> events.c
> >> 489: s/schedule/scheduled/
> >>
> >> 493: As none of the current set of callers require nanosecond
> >> granularity
> >> (0 is passed for delta_ns in all cases), I think this parameter
> >> should be removed and a value of 0 assumed.
> >>
> >> 528: Why is this here? Seems like this parameter should just be
> >> removed,
> >> as this odd line is the only reference to it now.
> >>
> >> 548: This needs to be on two lines.
> >>
> >> -renee
> >> _______________________________________________
> >> nwam-dev mailing list
> >> nwam-dev at opensolaris.org
> >> http://mail.opensolaris.org/mailman/listinfo/nwam-dev
> >>
> > _______________________________________________
> > nwam-dev mailing list
> > nwam-dev at opensolaris.org
> > http://mail.opensolaris.org/mailman/listinfo/nwam-dev
>
