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
route.xml:
Its firewall_context has stability whereas other services' do not.
ipfd.c:
It shouldn't be necessary to cast your string literal constants to
const char *.
Your global variables should all be static.
When the connection to the repository is lost, you could lose
notifications of updates. Shouldn't you reprocess all services'
configurations at that point?
ipfd.c`daemonize_self:
This shouldn't fail if close(STDIN_FILENO) fails.
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
}
ipfd.c`is_correct_event:
Why are you interested in restart events?
ipfd.c`repository_event_process:
The syslog of "%s" on like 585 isn't meaningful.
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.
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?
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.
Everyone who calls fmri_to_instance() emits the same error message
on its failure. Move that code here.
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.
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"?
You should only set aux to "administrative_request" when
event_from_tty(inst) != 0 (and possibly only when new_cur_state ==
IIS_MAINTENANCE).
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.
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.
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.
svcadm.c`check_tty:
Instead of inspecting your psinfo to determine if you are being run
interactively, use isatty() on STDIN_FILENO.
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) {'
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().
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.
explain.c`print_aux_fmri_logs:
Shouldn't you scf_instance_destroy() after the call to
scf_handle_decode_fmri()?
Dave
_______________________________________________
networking-discuss mailing list
[email protected]
