On Thu, Aug 16, 2007 at 07:00:27PM -0700, David Bustos wrote:
> cmd/svc/configd/rc_node.c
> 1985: Why don't we do composition on snapshots? Wouldn't that expose
> instance property values which are protected by the service's
> read_authorization?
Initially, I had done it this way and then changed it not to, because
I had for some reason gotten to thinking that the snapshot would
include the service properties (which would be good, because the
enabling authorization name and the protected properties would be kept
together). Obviously, that's wrong, so I've restored this to its
original behaviour. I don't really think this is a good way to use
this, though - for most if not all use cases, the read_authorization
property should be at the same level as the properties it is intended
to protect - and this seems already to be true of existing
authorization usage. Otherwise, if the nature of the properties in
the group are changed (perhaps policy is becoming less restrictive
because a particularly sensitive value is being removed), a snapshot
could accidentally expose the value to users with authorizations that
would not have been sufficient to read the snapshotted values when
they were current.
> 4031-4314: Can you move these functions to a new section above the
> Iteration comment at old 3809?
Sure.
> 4131,41,42,46: Shouldn't these be indented?
Yes.
> 4134,63: I don't think you can dereference prop after you have
> rc_node_rele()'ed it.
Probably best not to do that, yes.
> 4193: Are there times when rn_type is NULL?
It doesn't appear to be possible, no. I'll remove the test.
> 4212: Why don't you return _DELETED if rc_svc_prop_exists() returned
> _DELETED?
Because rc_svc_prop_exists() does composition; we don't care if the
property group has been deleted in the service (for example). All we
care about is whether or not the property exists somewhere. If it
doesn't, it makes no difference whether that's because the property
group containing it is being deleted or just never existed in the
first place.
I think the right way to solve this is to return _DELETED from
rc_svc_prop_exists() only if ent (the instance or snaplevel) or one of
its ancestors has been deleted. In those cases, we do want to pass
that through. I've changed these functions accordingly.
> 4215: Please change this to
>
> case REP_PROTOCOL_NO_RESOURCES:
> return (ret);
>
> default:
> bad_error("rc_svc_prop_exists", ret);
>
> 4222,4223s/npp/np/ ?
>
> 4248: Please document this in the comment.
Ok.
> 4313: Did you consider reducing indentation by doing
>
> if (client_is_privileged())
> return (REP_PROTOCOL_SUCCESS);
>
> #ifdef NATIVE_BUILD
> return (REP_PROTOCOL_FAIL_PERMISSION_DENIED);
> #else
>
> ret = rc_node_parent(np, &pgp);
> ...
>
> ?
No, but I'm happy to make that change. I was generally aiming for
consistency with e.g. rc_node_modify_permission_check().
> 4343: Please document _NO_RESOURCES in the comment.
Ok.
> 4345: I don't think it's safe to use np after you've rc_node_rele()ed
> it.
Right.
> 4389: I don't think this hold is necessary. I believe the iter should
> already have a hold, which is why the function didn't add a hold in
> the first place. In that case, you should be able to do
>
> RC_NODE_CHECK(np);
> ret = rc_node_property_may_read(np);
>
> if (ret != REP_PROTOCOL_SUCCESS)
> return (ret);
>
> RC_NODE_CHECK_AND_LOCK(np);
> ...
Agreed.
> cmd/svc/startd/graph.c
> 5583: Shouldn't you add an SCF_ERROR_PERMISSION_DENIED case to this
> switch?
>
> cmd/svc/startd/libscf.c
> old 966,1012,2321,2346,2372,2882,2958,2983: Shouldn't you add EACCES
> cases to these switches?
>
> old 1685,1814,3159,3486: Shouldn't you add SCF_ERROR_PERMISSION_DENIED
> cases to these switches?
>
> cmd/svc/startd/startd.c
> 508: Shouldn't you add an SCF_ERROR_PERMISSION_DENIED case to this
> switch?
This is a can't happen case, but for consistency with the existing
error handling I've added them to the bad_error() paths.
> cmd/svc/svccfg/svccfg.h
> SCE_NO_VALUES: I took this to mean that we would export a manifest
> with no values whatsoever. Please clarify by either explaining it's
> only used for individual properties (in particular,
> lscf_service_export() does not expect it), or moving it to
> svccfg_libscf.c .
I just killed it completely; we now use SCE_ALL_VALUES, which has a
more inclusive comment. This seems clearer.
> cmd/svc/svccfg/svccfg_internal.c
> old 1046: Shouldn't you add an EACCES case to this switch?
Yes.
> cmd/svc/svccfg/svccfg_libscf.c
> old 3036,3082,3345,3471,4044: Shouldn't you add _PERMISSION_DEINED
> cases to these switches?
As with startd, this can never happen. I've added them to the
bad_error case. Additionally, I've added EACCES cases for load_pg
calls (since the change at svccfg_internal.c:1046 can return it).
> 7274: Did you consider
>
> if (flags & SCE_NO_VALUES)
> goto empty;
>
> ? I think that would improve readability.
Agreed.
> 7302: Shouldn't you tell the user what you couldn't read? And
> shouldn't you produce an empty property element anyway?
No. We reach this case only if the user has given -a; that is,
explicitly requested all property values. If we are unable to obtain
one, but generate the manifest/archive anyway, its contents will not
be what the user requested. The documentation changes discussed in
the ARC case make this clear.
> pg_is_read_protected(): Wouldn't it be better to ask svc.configd about
> this?
I was hoping to avoid this, but it seems like the right thing to do.
I've made the change in a manner similar to the implementation of
scf_pg_get_flags.
> 7382: Don't create a new property handle. Just use exp_prop.
Ok.
> 7398: Aren't there ways to tell lint & the compiler that scfdie()
> doesn't return?
One can tell gcc about it, and we do with __NORETURN. Studio and lint
aren't smart enough. You can see other NOTREACHED comments after
scfdie() in the same file; e.g, create_entity().
> 7402: The "Returns 0..." bit seems inappropriate, since the function
> has type void.
Yep.
> 8939: Doesn't this bzero() have no effect on the outcome of the
> program?
Yes, but I'm being cautious; the contents of struct export_args could
grow later, and that should not break this code.
> lib/libscf/common/midlevel.c
> 309: Won't this allow scf_simple_prop_get() to fail with
> _PERMISSION_DENIED? That doesn't seem to be in your ARC case.
Yes, this should be in the documentation changes. I'll update
6538447; the change is obvious enough.
> - Shouldn't you add cases to the switches at line 1390 of
> cmd/svc/svcs/explain.c, line 365 of cmd/svc/svcs/svcs.c, and lines
> 1059, 1356, and 2374 of lib/librestart/common/librestart.c ?
Added.
Thanks for your feedback, Dave. I've posted an updated webrev which
incorporates your comments and also includes a few other changes.
--
Keith M Wesolowski "Sir, we're surrounded!"
FishWorks "Excellent; we can attack in any direction!"
