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? > >>> 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). > 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? > >>> 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. David
