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.
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()).
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)) {
...
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);
}
...
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
