On 21/10/08 03:22 PM, Tony Nguyen wrote:
> Dareen,
>
> Thanks for your comments. See my responses inline.
>
> Darren Reed wrote:
>> Just to get things moving along...
>>
>> repval.c lines 829-829
>> Type: E
>> Priority: 1
>> Comment: what doe this function do and why?
> This is a convenient function for deleting a repository property.
> Changes in inetd and SMF restarter and startd code implement a
> mechanism to communicate that a service is placed in maintenance by
> request of another service. This allows us to place a service with
> misconfigured firewall policy into maintenance and communicate to
> users via svcs -x that network/ipfilter has placed the service into
> maintenance. User can then look at the network/ipfilter log file for
> details reasons.
The point here is that what is being done is not obvious from either the
function
name or quickly reading the code. A comment would really help.
>> ipfd.c line 63
>> Type: T
>> Priority: 3
>> Comment: calling the variable "pid" is a bit misleading, it is the
>> parent pid
>>
> Isn't it the child's pid returned to the parent? What do you suggest?
No. you're right, my brain was flipped.
>> ipfd.c line 273
>> Type: T
>> Priority: 1
>> Comment: If we get here from 158 then aren't we using v before it
>> has been initialised?
> v is initialized to NULL.
Hmm? On which line?
148 static boolean_t
149 is_correct_event(const char *fmri, const scf_propertygroup_t *pg,
150 const boolean_t isrpc)
151 {
152 char *pg_name, *value, *state = NULL;
153 scf_value_t *v;
154 boolean_t ret = B_FALSE, in_maintenance = B_FALSE;
155
156 if ((pg_name = umem_alloc(max_scf_value_size, UMEM_DEFAULT))
157 == NULL)
158 goto out;
...
268 out:
269 if (state)
270 free(state);
271
272 umem_free(pg_name, max_scf_value_size);
273 scf_value_destroy(v);
274
275 return (ret);
276 }
>> ipfd.c line 402-543
>> Type: T
>> Priority: 2
>> Comment: This appears to be a busy-wait loop and of concern is that
>> there doesn't appear to be anything to moderate the looping. Consider
>> adding in a delay/pause/sleep for a short period of time, if only so
>> that
>> syslog doesn't get spam'd with messages.
>>
> Not quite. _scf_notify_wait results in a blocked thread, see
> rc_notify_info_wait in rc_node.c, until an interested event happened.
In the absence of a man page for _scf_notify_wait(), it might be
worthwhile adding
a commit here that mentions the behaviour.
Darren
