Quoth Keith M Wesolowski on Fri, Aug 03, 2007 at 01:45:56PM -0700:
> I'd appreciate review of my changes for
>
> PSARC 2007/177 SMF read-protected property storage
> 6537749 SMF should support read-protection of data
> 6538452 svccfg delete leaks memory on syntax error with options
>
> The webrev may be found at
> http://cr.opensolaris.org/~wesolows/6537749/webrev/. Note that this
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?
4031-4314: Can you move these functions to a new section above the
Iteration comment at old 3809?
4131,41,42,46: Shouldn't these be indented?
4134,63: I don't think you can dereference prop after you have
rc_node_rele()'ed it.
4193: Are there times when rn_type is NULL?
4212: Why don't you return _DELETED if rc_svc_prop_exists() returned
_DELETED?
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.
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);
...
?
4343: Please document _NO_RESOURCES in the comment.
4345: I don't think it's safe to use np after you've rc_node_rele()ed
it.
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);
...
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?
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 .
cmd/svc/svccfg/svccfg_internal.c
old 1046: Shouldn't you add an EACCES case to this switch?
cmd/svc/svccfg/svccfg_libscf.c
old 3036,3082,3345,3471,4044: Shouldn't you add _PERMISSION_DEINED
cases to these switches?
7274: Did you consider
if (flags & SCE_NO_VALUES)
goto empty;
? I think that would improve readability.
7302: Shouldn't you tell the user what you couldn't read? And
shouldn't you produce an empty property element anyway?
pg_is_read_protected(): Wouldn't it be better to ask svc.configd about
this?
7382: Don't create a new property handle. Just use exp_prop.
7398: Aren't there ways to tell lint & the compiler that scfdie()
doesn't return?
7402: The "Returns 0..." bit seems inappropriate, since the function
has type void.
8939: Doesn't this bzero() have no effect on the outcome of the
program?
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.
- 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 ?
David
