Quoth Tony Nguyen on Wed, Oct 29, 2008 at 04:13:33PM -0700:
> The updated and incremental webrevs are at:
>
> http://cr.opensolaris.org/~tonyn/firewall29Oct2008/
>
> http://cr.opensolaris.org/~tonyn/firewall-inc/
cmd/cmd-inet/usr.lib/inetd/inetd.c
fmri_to_instance(): Most (by number) of the paths through this
function don't set *scf_instp. Won't that cause
inst_reset_aux_fmri() to call scf_instance_handle() with stack
garbage at 343? For paths where scf_handle_create() returned
success, won't that also cause a handle to be leaked?
319: cstyle says that if one arm of an if statement has braces, they
all should.
409: Does this have to happen in inetd? Can it be done in
restarter_set_states()? Otherwise, doesn't every restarter have to
be changed to accommodate this, which is something you were trying
to avoid?
424: Can this also be done in restarter_set_states()?
470: Is this change an easter egg?
cmd/ipf/svc/ipfd.c
47: Please add text here or elsewhere explaining that the means by
which you're observing events is not stable.
85: SBIN_SH is no longer used.
102: I think you should change this to "Could not open /dev/null",
and include strerror(errno).
106: Is this really a fatal error?
112: Shouldn't you print a message explaining that fork() failed?
131: Could you add some text to this explaining that service-specific
ip filter configuration may not be updated properly?
187,192,199: Shouldn't you give the FMRI of the service which you
can't process an event for?
187,192: Since this program is single-threaded, did you consider doing
this once at the beginning of the program so you wouldn't have to
deal with these errors?
238: It appears that you don't use in_maintenance in this case. Did
you consider moving the isrpc check up before smf_get_state()?
329: Would proceeding if ipfilter is not online be harmful?
347: Shouldn't you include the FMRI of the service you can't update?
355: If the exec() failed, shouldn't you print an error or something?
361: Why is this if statement necessary?
371: I don't understand what you mean by "Repository and libscf are
treated...".
384: Why is it ok to return false if these functions fail? Did you
consider hoisting them out of this function?
401,405,408: Why is it ok to return false if these functions fail?
430-5: Did you consider allocating these outside of this function?
That would reduce the probably that we would encounter an error in
repository_event_process(), which would reduce the probability that
we have to emit the grossly imprecise error at 653.
440: This remark about "inst" seems out of place here.
442: Why do you update the pg? Couldn't that cause you to miss
events, if it's being modified quickly enough?
555,586: If the repository connection is broken, then your
notification requests will be cleared, so repository_event_wait()
will have to re-invoke _scf_notify_add_pgtype().
614: Shouldn't you inform the user that you couldn't allocate memory?
617: Shouldn't you inform the user that that you're exiting because
scf_pg_create() failed, and perhaps why it failed?
623: SCF_ERROR_NOT_BOUND should be impossible since scf_pg_create()
succeeded.
649: Please add a comment explaining why it's appropriate to rebind
and loop without notifying the user of the error.
662: Why don't you have to do anything if a service was deleted?
679: scf_handle_create() can fail, so you should check the return
value. You can file a bug to do this later, though.
cmd/svc/servinfo/servinfo.c
*: This is a new binary, right? Shouldn't it have been mentioned in
the ARC case?
27: Please add a comment explaining what this program does.
lib/librestart/common/librestart.c
_restarter_commit_states(): Don't these changes modify the behavior of
restarter_set_states() in an incompatible fashion? That interface
has been contracted out to Sun Cluster. Have you contacted them?
Shouldn't this be documented in your ARC case?
David
