David Powell wrote:
> Tony Nguyen wrote:
>> Dave,
>>
>> See my responses inline and the updated webrev at:
>>
>> http://cr.opensolaris.org/~tonyn/firewall13Jan2009-inc/
>> http://cr.opensolaris.org/~tonyn/firewall13Jan2009/
>
> Thank you, thank you, thank you for creating scf_instance_delete_prop.
any time :)
>
> I'm still working through ipf_include.sh.
>
> nfs-server:
>
> What's up with waiting for mountd for a second? Is that even
> effective?
I believe that was for a timing issue where nfs/server isn't yet fully
online when we start to generate IPfilter configuration for it. However,
I can't seem to reproduce the problem to confirm since other changes
may have minimize chances for this timing issue to occur.
Regardless, I agree the sleep isn't adequate. We should check to make
sure nfs/server is "online" before generating its rules.
>
> milestone/global.xml:
>
> exceptions description:
>
> Use the same grammar used in the "apply_to" description. i.e.
>
> The host and network IPs, network interfaces, and ippools to
> exempt from the set policy. That is, those to accept if the
> policy is set to deny, or to deny if the policy is set to
> accept.
will do
>
> svcadm.c:
>
> In some places you uu_die if my_ctname fails, in others you uu_warn.
> The error handling should be consistent.
>
agreed. my_ct_name should uu_warn rather than uu_die if it fails.
> Though it is unlikely that your error messages will ever be seen by
> the user, if they're worth localizing they're also worth making more
> meaningful to a non-developer. e.g. instead of
>
> "Failed to process %s: restarter_setup() failed"
>
> from which, nonintuitively from the reader's perspective, you
> continue, something like this would be better:
>
> "Unable to record FMRI with request. svcs -l output may be
> incomplete."
will do
>
> yp.sh:
>
> Should there be an exit after the call to create_client_ipf_rules?
>
Yes, I'll add it for proper coding. It was not necessary because we'd be
running in the context of network/ipfilter service though that's a not a
good assumption.
> create_client_ipf_rules appears to mishandle multiple IP address for
> a single host name.
>
> If the host name is a substring of another, you'll get erroneous
> results.
>
> If IPv6 addresses are included in your hosts file, I think your
> implementation will malfunction.
Good points. I'll modify the function.
>
> rfc1179.xml:
> ipp-listener.xml:
>
> ipf_method should refer to a full FMRI, not just "print/server".
will do
>
> ipfilter.xml:
>
> Now that you are starting a persistent daemon, this shouldn't be a
> transient service. (Arguably this is an existing bug given ipmon).
Agreed. IIRC, the IPfilter team intended to allow users to interact with
ipf commands to configure IPfilter. Thus the network/ipfilter service
performs initialization and load ipf/ipnat configuration if they exist.
Users can also manipulate IPfilter configuration once network/ipfilter
is enabled, completed the initialization.
The existing transient service implementation doesn't fully take
advantage of the SMF model. If network/ipfilter is a contract service,
users can still manually manipulate IPfilter configuration by following
the steps listed in ipf(1) with the additional starting of ipmon.
There are actually two persistent daemons, existing ipmon and the new
svc.ipfd so a contract service would be more appropriate and consistent.
I'll consult with Darren on this suggested change.
>
> svc/ipfilter:
>
> When you start, there shouldn't be a svc.ipfd to kill.
> Instead of pgrepping, use smf_kill_contract in the stop method.
ah, will make the change.
> What does the 'reload' subcommand do, and where is it used and
> documented?
'Reload' subcommand implements refresh method for network/ipfilter.
>
> It may seem hacky, but I think I'd prefer having the default policy
> be empty, and using that default state as an indicator that our
> initial boot hasn't yet occurred.
That's a clever solution.
> An explicit "upgraded" property
> that gets set once and sticks around forever is unappealing.
> Additionally, it's one more thing that a profile creator will need to
> know to set to avoid their settings being overwritten on first boot
> (e.g. in a jumpstart-like environment).
I agree that it's an additional burden for profile creator. A potential
benefit of an explicit property is making it possible to do more than
one migration (versioning). For example, a package delivering a new
configuration version can set the property to 'false' to signal the
service to perform configuration migration on the next start.
However, resetting a primary property as suggested above would achieve
the same behavior and our current problem doesn't require more than one
migration. I'll make the suggested change.
>
> ipfd.c:
>
> repository_event_wait:
>
> Is the variable 'scratch' still needed?
I believe so. scf_parse_svc_fmri() performs destructive parsing and we
need to preserve the original fmri.
>
> is_correct_event:
>
> The seven nearly identical blocks after determining you're dealing
> with SCF_PG_RESTARTER_ACTIONS could be reduced to a loop through an
> array of property types to test (you would need two arrays: one for
> when you're not in_maintenance, and one for all the time).
>
will do
Thanks,
-tony
