Darren Reed wrote:
> On 21/10/08 03:22 PM, Tony Nguyen wrote:
>> Darren,
>>
>> 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.
>
Will do.
>
>>> 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 }
line 153 though it's unnecessary now that we simple return on 158. The
code now reads:
char *pg_name, *value, *state = NULL;
scf_value_t *v = NULL;
boolean_t ret = B_FALSE, in_maintenance = B_FALSE;
if ((pg_name = umem_alloc(max_scf_value_size, UMEM_DEFAULT))
== NULL)
return (ret);
>
>>> 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.
will do.
Thanks,
tony
