Thanks Dave. See my responses inline.
-tony David Powell wrote: > Tony Nguyen wrote: >> The latest and greatest is at >> >> http://cr.opensolaris.org/~tonyn/firewallNov262008/ >> >> The new webrev includes Darren's comments and changes for >> >> 6236609 svc.startd resets auxiliary state on svcadm mark maintenance >> 6762307 SMF - expressing a service's maintenance state by request of >> another service >> >> which are now captured in >> >> PSARC/2008/730 SMF - improved maintenance diagnosis >> >> The original host-based firewall ARC case will be updated to mention >> the above case. > > I haven't yet reviewed the script changes. These comments cover only > the C code and manifests. > > general: > > You need to supply template data for firewall_context and > firewall_config, in ipfilter.xml for ipfilter (effectively replaces > the comments) and in restarter.xml for the rest ah, yes :) > > route.xml: > > Its firewall_context has stability whereas other services' do not. > IIRC, we decided to not include the stability. I'll remove it. > > ipfd.c: > > It shouldn't be necessary to cast your string literal constants to > const char *. done > Your global variables should all be static. done > > When the connection to the repository is lost, you could lose > notifications of updates. Shouldn't you reprocess all services' > configurations at that point? Good idea. At the moment, whenever network/ipfilter starts (enabled, restarted, and refreshed), it configures the global and all services' policies before starting svc.ipfd. Thus, the entire firewall is built only when network/ipfilter starts and svc.ipfd is responsible for monitoring service events and requesting update to service specific firewall configuration. We can extend the current implementation to request configuration update for network/ipfilter whenever a repository connection is lost which would rebuild the global and all services' configuration. This solution is simple and the entire firewall can be quickly rebuilt by a single request and is my preferred approach. The alternative would be to separately request update for each services. This would prevent us from taking down the default global policy though it would take a lot longer to rebuild the entire firewall, since each update requires spawning a new process and unloading/reloading of the global override policy. What do you think? > > ipfd.c`daemonize_self: > > This shouldn't fail if close(STDIN_FILENO) fails. ok > > ipfd.c`pg_get_prop_value: > > Instead of > > if (scf_pg_get_property() == -1) { > error handling > } > > if (scf_property_get_value() == -1) { > identical error handling > } > > do > > if (scf_pg_get_property() == -1 || scf_property_get_value() == -1) { > error handling > } done > > ipfd.c`is_correct_event: > > Why are you interested in restart events? for services that may have different port after restart. At the moment, RPC services and in general, services that use ephemeral ports would have that behavior. Most of these services, if non-RPC, will mostly likely have ipf_method, should we accept restart events only if the service has ipf_method? > > ipfd.c`repository_event_process: > > The syslog of "%s" on like 585 isn't meaningful. Leftover debug, it's removed. > > ipfd.c`repository_event_wait: > > If the fmri strlcpyed on line 658 is too long, _scf_notify_wait is > malfunctioning. You don't need to handle this error. ok. we can omit the error and continue to listen for new notification. > > ipfd.c`main: > > Why are you using SCF_LIMIT_MAX_VALUE_LENGTH and asserting it's at > least as large as SCF_LIMIT_MAX_NAME_LENGTH... instead of just > using SCF_LIMIT_MAX_NAME_LENGTH? Remnant from earlier code, it's removed. > inetd.c`fmri_to_instance: > > If make_handle_bound() fails, you'll call scf_instance_destroy() on > the uninitialized 'scf_inst'. > > You unnecessarily call make_handle_bound() a second time in the > first iteration of your for loop. Perhaps you could remove the > call outside the loop (also solving the first problem), or move > this to the bottom of the loop. The first make_handle_bound call is removed. > > Everyone who calls fmri_to_instance() emits the same error message > on its failure. Move that code here. done > > inetd.c`inst_set_aux_fmri: > inetd.c`inst_reset_aux_fmri: > inetd.c`event_from_tty: > inetd.c`inst_validate_ractions_aux_fmri: > > All these functions are called from the same place, and the only > thing any of them do with the instance_t is convert it to an > scf_instance_t with fmri_to_instance(). Do this once in > update_instance_states() and instead pass the scf_instance_t to > these functions. Actually, if the conversion is removed, these functions are one-liner wrappers for librestart functions, I'll remove these functions and directly call the librestart functions. > inetd.c`update_instance_states: > > Why can't the 'if (strcmp(aux, "service_request") == 0) {' block > just with the code that sets aux to "service_request"? I thought it'd be more readable. Do you prefer to move this block? > > You should only set aux to "administrative_request" when > event_from_tty(inst) != 0 (and possibly only when new_cur_state == > IIS_MAINTENANCE). good point. > > librestart.c`restarter_inst_delete_prop: > > There are already nearly identical versions of this function in > startd, inetadm, and svcadm. I won't review a fourth. yes, there should be an effort to pull functions like these into libscf. > > librestart.c`restarter_inst_set_astring_prop: > > This also appears to be cut-and-pasted from somewhere. It does a > lot of intricate error handling which isn't used by its consumers. > Either simplify this code or reimplement it in terms of existing > functionality. simplified. should we do the same for the restarter_inst_delete_prop()above? > > svcadm.c`set_astring_prop: > > Are you making any changes to this function other than to move it? > If not, leaving it in its original place and adding a prototype > would make your changes easier to review and understand. > Yes, adding a declaration is much simpler. > svcadm.c`check_tty: > > Instead of inspecting your psinfo to determine if you are being run > interactively, use isatty() on STDIN_FILENO. done > svcadm.c`my_ct_name: > > Opening procfs to obtain your contract ID is already implemented by > the getctid(); you should use that instead of reimplementing it. > > Be more explicit: > 'if ((errno = ct_status_read()) != 0) {' > > The error handling for ct_pr_status_get_svc_fmri uses an > uninitialized errno. You should instead say (as above): > 'if ((errno = ct_pr_status_get_svc_fmri()) != 0) {' done > > svcadm.c`clear_instance, svcadm.c`force_degraded: > > You should only call restarter_setup() in the cases where you > attempt to call set_inst_action(). > > You would get this automatically if instead of meticulously calling > restarter_setup() before each call to set_inst_action() or > set_inst_enabled(), you simply put the call to restarter_setup *in* > the set_inst_action() and set_inst_enabled(). much better. > > explain.c`add_instance: > > Though no-one is freeing inst_t.aux_state, I recommend assigning > safe_strdup(AUX_STATE_INVALID) to it instead of AUX_STATE_INVALID > in case someone ever does. > done > explain.c`print_aux_fmri_logs: > > Shouldn't you scf_instance_destroy() after the call to > scf_handle_decode_fmri()? > done
