David Bustos wrote: > 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?
agreed. callers should set *scf_instp to NULL and destroy the handle only if scf_instp is not NULL. For paths where scf_handle_create() returned success, make sure the handle is destroyed if we fail later in the function. > > 319: cstyle says that if one arm of an if statement has braces, they > all should. done > > 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? While it's possible to do this in restarter_set_states(), I'm a little concerned about librestart changing a value explicitly passed in by the restarter. > 424: Can this also be done in restarter_set_states()? Restarter may have different transitioning logic to determine online state of its instances. restarter_set_state() will need to understand possible restarter specific state transitions to correctly reset auxiliary fmri. > 470: Is this change an easter egg? Unintentional,it's removed. > > 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. done > > 85: SBIN_SH is no longer used. done > > 102: I think you should change this to "Could not open /dev/null", > and include strerror(errno). done > > 106: Is this really a fatal error? ah, we can simply call close() and ignore the return value since that's consistent with the closefrom() later. > > 112: Shouldn't you print a message explaining that fork() failed? done > > 131: Could you add some text to this explaining that service-specific > ip filter configuration may not be updated properly? done > > 187,192,199: Shouldn't you give the FMRI of the service which you > can't process an event for? These errors result in a return of -1 to the repository_event_process() which will log an error with the FMRI. > > 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? good idea. I'll do so. > > 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()? we would lose debug information. If we decide to remove the debug information then it'd make sense to move this case to the beginning of the function or better yet check for isrpc from the caller. > > 329: Would proceeding if ipfilter is not online be harmful? Not harmful just pointless since the ipf command wouldn't succeed and the update script will not run if network/ipfilter isn't online. > > 347: Shouldn't you include the FMRI of the service you can't update? done > > 355: If the exec() failed, shouldn't you print an error or something? done > > 361: Why is this if statement necessary? yup, it's not. > > 371: I don't understand what you mean by "Repository and libscf are > treated...". That's because the comments should read "Repository and libscf errors are treated as if the service isn't an RPC service, returning B_FALSE to indicate validation failure" > > 384: Why is it ok to return false if these functions fail? Did you > consider hoisting them out of this function? hoisting, it is. > > 401,405,408: Why is it ok to return false if these functions fail? Services are not required to have a "rpc" property. The function can't verify an RPC service if the property is not found or if there's an error getting value. > > 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. I'll do so. > > 440: This remark about "inst" seems out of place here. removed > > 442: Why do you update the pg? Couldn't that cause you to miss > events, if it's being modified quickly enough? True. We're only reading so probably don't need to update the pg. > > 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(). true. I'll figure out how to that. > > 614: Shouldn't you inform the user that you couldn't allocate memory? done > > 617: Shouldn't you inform the user that that you're exiting because > scf_pg_create() failed, and perhaps why it failed? done > > 623: SCF_ERROR_NOT_BOUND should be impossible since scf_pg_create() > succeeded. removed > > 649: Please add a comment explaining why it's appropriate to rebind > and loop without notifying the user of the error. yes, user should be notified. > > 662: Why don't you have to do anything if a service was deleted? The comment is incorrect. The returned fmri is the deleted pg's fmri not the fmri of a deleted service or instance. If a service is deleted via 'svccfg delete -f fmri', we'll have service specific leftover IPfilter configuration. I see this as a corner case. To support the above force service deletion, we'll need to derive an action from a returned pg, i.e. if the deleted pg is an instance's 'general' pg, update the instances' IPfilter configuration. Do you think we should handle this? > > 679: scf_handle_create() can fail, so you should check the return > value. You can file a bug to do this later, though. > actually, it should check the return value and give meaningful error. > cmd/svc/servinfo/servinfo.c > *: This is a new binary, right? Shouldn't it have been mentioned in > the ARC case? argh, will do. > 27: Please add a comment explaining what this program does. > will do > 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? > These changes address an existing SMF bug 6236609 svc.startd resets auxiliary state on svcadm mark maintenance so I'm not sure if it should be mentioned in this ARC case. Sun Cluster is now aware of the changes and will update their code once the fix is integrated.
