Quoth Tony Nguyen on Sun, Oct 19, 2008 at 02:49:40AM -0700:
> This is for 6761070 PSARC 2008/580 Solaris host-based firewall
> <http://monaco.sfbay/detail.jsf?cr=6761070>*
>
> *The webrev and the new event message are respectively at:
>
> http://cr.opensolaris.org/~tonyn/firewall/
>
> http://cr.opensolaris.org/~tonyn/firewall/8000-R4
I've only reviewed the mentioned files so far. I'll continue after
you've addressed these comments.
One general question I have is what the long-term plan is for observing
service changes. Here you're using _scf_notify_wait(), but the
restarter protocol is far better suited for this sort of information.
I presume you're not using that because you don't want to be the
responsible restarter for these services. Which makes me wonder why
you're not modifying the restarters themselves. Otherwise, it seems
that you're working around a hole in the SMF architecture for agents
which want to tie behaviors of the interfaces a service implementation
uses to the state of the service implementation itself. Which I vaugely
recall coming up before, in the form of an agent wanting to observe
state changes in services, but I don't remember the details.
David
cmd/cmd-inet/usr.lib/inetd/inetd.c
291: Where is this handle destroyed?
341: If you're going to document one return value, please document all
of them.
354: Please add a comment explaining why it's ok to return the same
value when fmri_to_instance() fails and instance_validate_aux_fmri()
fails.
405
- Doesn't lint complain since you're not casting this return value
to void?
- Please add a comment explaining why it's ok not to notify anyone
when this fails.
cmd/cmd/inet/usr.lib/inetd/repval.c
828: If you're going to document one return value, please document all
of them.
scf_inst_delete_prop(): Since this function isn't in libscf, please
use a different prefix.
899: My manpage says scf_entry_destroy() has a void return type. Is
yours different?
cmd/ipf/svc/ipfd.c
26: Please add a comment explaining what this program is supposed to
do.
88: I believe commented-out code is frowned upon.
102: Why is it ok to return 0 if scf_handle_bind() fails for reasons
other than SCF_ERROR_NO_RESOURCES?
141: I don't understand what "Restarter events are notified twice"
means, or how it's relevent to this function.
146: Please add an explanation of why it's ok to return false when
there's a memory allocation error.
157: cstyle says lines should not begin with binary operators.
163: Please add
assert(max_scf_value_size >= scf_limit(SCF_LIMIT_MAX_NAME_LEN) + 1);
somewhere before this.
164: What should a user do if he sees this log message?
197: So I suppose you're relying on the fact that svc.startd removes
all of the values from the property when it's done, eh? I believe
that's an unstable interface. Shouldn't that have been included in
your ARC case?
310: I presume you considered launching ipfilter in a separate process
contract?
317: Is there a reason why you launch through the shell? It doesn't
seem that any of the arguments need to allow shell substitions.
312: What should a user do if he sees this log message?
318: Why do you use _exit() here rather than exit()?
service_is_rpc(): Please add a comment explaining why it's ok to
return B_FALSE when the property exists with the value "false" and
when there's an error.
352: Is there a reason you don't use SCF_SNAPSHOT_RUNNING here?
359: I didn't see this behavior of preferring inetd/isrpc over
firewall_context/isrpc in the ARC case. Did I miss it? Or does it
not matter?
369: Is there a reason you're using scf_value_get_as_string() instead
of scf_value_get_boolean()? With the latter, you wouldn't have to
allocate value.
405-12: Too much indentation.
407: Please add a comment explaining why it's ok to continue of
repository_rebind() failed.
414: It's not clear to me that it's ok to retry in such a tight loop
for all errors of _scf_notify_add_pgtype() caught by the default
case. It would help for you to catch the errors explicitly and
abort() on unknown errors.
418: "for (;;)" is the preferred style for infinite loops in O/N. It
also eliminates the need for "/*CONSTCOND*/".
420: Note that using _scf_notify_wait() will increase the incidence of
6531656. We may want to raise its priority to avoid a fire drill.
422: Please add a comment explaining why it's ok to ignore errors from
repository_rebind().
426-535: Is there a reason this code can't be in a separate function?
436: SCF_ERROR_NOT_BOUND should be impossible since _scf_notify_wait()
succeeded.
438: I'm not sure it's appropriate to log this message without
explaining what its consequences are.
443: I believe SCF_ERROR_NOT_SET signifies a bug in libscf or
svc.configd. Shouldn't you abort() if it occurs?
446: What should a user do if he sees this log message?
453: SCF_ERROR_NOT_BOUND should be impossible since scf_pg_update()
succeeded.
458: Please add a comment explaining why it's ok to ignore the other
errors.
462: This buffer is one character too small. In theory, you should
allocate a buffer of size scf_limit(SCF_LIMIT_MAX_FMRI_LENGTH + 1).
466: SCF_ERROR_NOT_BOUND should be impossible since
scf_pg_get_parent_instance() succeeded.
471: Please add a comment explaining why it's ok to ignore the other
errors.
490: SCF_ERROR_NOT_BOUND should be impossible since
scf_instance_to_fmri() succeeded.
495: Please add a comment explaining why it's ok to ignore the other
errors. And why when the running snapshot doesn't exist, we don't
fall back to the current properties like service_is_rpc().
503: What should a user do if he sees this log message?
512: SCF_ERROR_NOT_BOUND should be impossible since
scf_instance_get_snapshot() succeeded.
515: Please add a comment explaining why it's ok to continue for other
errors.
522: What should a user do if he sees this log message?
527: SCF_ERROR_NOT_BOUND should be impossible since
scf_instance_get_snapshot() succeeded.
532: Please add a comment explaining why it's ok to continue on other
errors.
lib/librestart/common/librestart.c
1608: If you're going to document one return value, please document
all of them.
instance_validate_aux_fmri(): Is there a reason this function is in
this part of the file?
1631: Why doesn't this test val for NULL?
1638: Is there a reason this function doesn't use the running
snapshot? If so, please document the fact that it doesn't in the
function's comment.
1642: Since you don't use aux_inst, did you consider using
scf_parse_fmri() instead?
lib/librestart/common/librestart.h
240
- It seems that all of the other symbols in this file begin with
"restarter". Is there a reason instance_validate_aux_fmri()
doesn't?
- Please add a comment documenting the function.
