Quoth Tony Nguyen on Wed, Nov 05, 2008 at 03:44:26PM -0800: > 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 ... > > 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.
Right. It depends on whether service_request should be an explicit feature or a transparent feature. I suspect the main criterion for that should be whether either way would bring inherent problems to the interface, and secondarily, which is more convenient for developers. It seems to me that transparency is more convenient, but I guess I'm not clear on whether it's legitimate to use service_request independently of restarter_inst_validate_aux_fmri(), or not to use service_request even though restarter_inst_validate_aux_fmri() succeeds. Do you know? > > 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"? ... > > cmd/ipf/svc/ipfd.c ... > > 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. Except that error is logged at LOG_DEBUG. repository_event_wait() does log an error if repository_event_process() fails, but it doesn't include the FMRI. ... > > 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. Indeed. My rationale was eliminating error cases, but that's not decisive, so do whatever you think is best. > > 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. ... > > 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. What if the property exists but has the wrong type? Wouldn't emitting an error make that easier to debug? Or is it not worth the effort in your judgement? ... > > 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. Actually the comment is correct. If a service or instance is deleted, you will receive events, and their FMRIs will be placed in your buffer. (All notification clients receive all deletion events.) > 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. That was my point. I presume you think this corner case is so rare that you don't need to handle it? > 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? I think you can ignore deletion events for property groups, and only pay attention to deletion events for instances. I think the ideal program would handle the events and clean up applicable rules. I don't know how common it will be, how bad the consequences of not doing it are, or how much work it would take. I'll leave those for you to judge, though if you decide not to do it, then you should probably add a comment and file a change request after integration. ... > > 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. Doesn't section 7d of the contract require ARC approval for the change? David _______________________________________________ networking-discuss mailing list [email protected]
