David Bustos wrote: > Quoth Tony Nguyen on Thu, Nov 06, 2008 at 03:55:47PM -0800: >>>>>> 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 >>>>> 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? > ... >> The short answer is: I don't have a definitive answer. Once we have >> enough data to make service_request mandatory, however, I agree that the >> code should be placed in librestart. > > Ok; can you add a comment to librestart.h before restarter_set_states() > explaining when restarter_inst_validate_aux_fmri() should be called and > "service_request" should be passed? >
ok >>>>> 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. >>> I don't understand. Why don't we always clear it if we exit >>> maintenance? And why do you not clear it if we enter online from >>> "IIS_IN_REFRESH_METHOD"? >> Clearing it when we exit maintenance is probably sufficient. However, I >> prefer to keep this function here rather than in librestart until we >> decide to mandate service_request. > > I suppose. Shouldn't the property-deleting functionality be in > librestart to complete the encapsulation begun by > restarter_inst_validate_aux_fmri(), though? In any case, please add > a comment to librestart.h explaining when the property should be deleted > (or the function invoked). ok > >> I originally wanted to clear only when the service becomes online. Using >> svccfg (and svcadm refresh) to populate the auxiliary_fmri will cause a >> transition from IIS_IN_REFRESH_METHOD to IIS_ONLINE which resets >> auxiliary_fmri. > > Sorry, but I don't understand. Why does populating auxiliary_fmri cause > a transition from IIS_IN_REFRESH_METHOD to IIS_ONLINE? > This is not specific to auxiliary_fmri; it's just how inetd transitions to online state for refresh action. >>>>> cmd/ipf/svc/ipfd.c > ... >>>>> 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. >>> I suppose modern systems have a lot of memory, so this isn't a big deal. >> Hmm, not sure I follow your thought here. If we don't fork an update >> process, why would there be memory concern. Or do you suggest that >> smf_get_state() may fail for lack of memory error and not elegantly handled? > > Yes, I usually prefer to avoid operations which might fail if memory is > low when possible. Though you're right that we're going to fork() > anyway, which will require a lot of memory. > > I suppose I have two real concerns. One is that this is the wrong place > to make this decision. If someday we change IPF_UPDATE_CMD to work even > when network/ipfilter is not online, then we have to change this code. > Which is a maintenance burden, but not a big one, I suppose. Worse is > probably when someone needs to debug this, and they don't expect this > decision here. That's difficult to quantify, though, so I guess it's > not a big deal. > > My second concern is that checking the state of the service is less > precise than the subsystem's commands themselves. There is a window > while ipfilter is starting where the functionality works, but the > service is not marked online yet. Given that you don't do anything if > the service is not online, I suppose you're optimizing for speed and > memory usage, but this isn't a performance-critical path, so I would opt > for maintainability by not duplicating this decision (which is > presumably being made by the ipfilter subsystem anyway), until it's > clear that this is a performance problem. Not a big deal, I guess. > Currently this daemon is running as part of network/ipfilter but those are reasonable concerns, I'll remove the state check for network/ipfilter. -tony
