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. Sorry for the confusion. I'll file a separate RFE for these changes and include it in the update webrev. > 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? > ipfd.c lines72-78 > Type: T > Priority: 2 > Comment: Consider moving this before the fork and exit so that any error > messages have synchronised output with the program's execution. > Additionally, you may want to explicitly close STDIN_FILENO before > doing the open on /dev/null, although you still need the if() and dup2 > even if you do this. > done > ipfd.c line 72 > Type: T > Priority: 3 > Comment: At 54 you have a #define for the name of sh, why not one > for /dev/null too? done > ipfd.c line 84 > Type: T > Priority: 3 > Comment: Why is this even necessary? It seems dangerous.. You're quite right. It's removed. > ipfd.c line 272 > Type: T > Priority: 1 > Comment: While umem_free(NULL,0) is allowed, if we get here from > line 158, then I'm expecting the call to be umem_free(NULL,>0)... Agreed, simply returns if umem_alloc returns NULL. > 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. > ipfd.c line 375-378 > Type: T > Priority: 1 > Comment: If we get here from 347, there is potential for many of these > calls to be made with uninitialised variables. Please make sure all of > the pointers are initialised to NULL at 338-341. done > > ipfd.c line 396-399 > Type: T > Priority: 1 > Comment: I suspect that if some of these succeed then you need to be > making appropriate 'undo' calls if you want to exit gracefully. If any of these fails, the process terminates. We don't have any persistent session information on the door server so there isn't any necessary cleanup. > 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. > ipfd.c line 538-541 > Type: T > Priority: 2 > Comment: Somehow I don't think that this syslog() with the accompanying > comment is an adequate "do something"... > > ipfd.c > Type: T > Priority: 3 > Comments: Doing a "return (-1)" from main() doesn't make a whole lot of > sense because $? in sh is an unsigned value of 0-255. See wait(3c). And > as an illustrated example: > $ sh > $ exit -1 > -1: bad number > $ echo $? > 1 > $ sh > $ exit 255 > $ echo $? > 255 > yes, it should return 1. Thanks, -tony
