On Thu, Sep 13, 2007 at 08:34:31PM -0700, David Bustos wrote:
> I'm confused. I think you're saying that when reading from a snapshot,
> the snapshot's version of the service's read_authorization should be
> used if the property doesn't exist at the instance. But the current
> version of perm_add_enabling_values() seems to always walk up the tree
> to find the live service, rather than looking for the service snaplevel
> in the snapshot case.
I see. Yes, that's a bug. I tested the snapshot case with instances,
but not with services. I'll fix this.
> Furthermore, the current comment confuses me
>
> * If pg is a child of an instance or snapshot, we want to compose the
> * authorization property with the service's (if it exists). The
> * snapshot case applies only to read_authorization. In all other
> * cases, the pg's parent will be the instance.
>
> From "[t]he snapshot case applies only to read_authorization" I infer
> that you will only execute snapshot-specific code when the property is
> named read_authorization, but I think you're trying to say that we don't
> need to care because the other authorizations affect modification, which
> is invalid for snapshots. The "In all other cases..." makes me think
This is what I'm trying to say. I'll clarify using your wording.
> > > 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.
>
> They can't happen if the user doesn't do anything stupid. Since they're
> outside the control of svc.startd, they should be handled.
The user has no control of these things; startd is reading from
framework property groups, for which read_authorization has no effect.
See rc_node_pg_check_read_protect() near 4049. Neither a bad manifest
nor administrator error can trigger these code paths. This is
described in the ARC case and clarified by Gary's addendum. The only
way these can fail on my account is a bug in configd.
Is there some manner in which this can fail that I'm overlooking?
> > > cmd/svc/svccfg/svccfg_libscf.c
> ...
> > > 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.
>
> Ok, but the documentation says "an error results". I think I was asking
> you to make the error contain the name of the property group, rather
> than just "permission denied".
I'll file an RFE for this but I don't intend to implement it. The
documentation is sufficiently clear that no unprivileged user should
expect export/archive -a to be successful.
> common/svc/repcache_protocol.h
> 232: Could you clarify this to say that the request gets an attribute
> of the entity (rather than just the name), and refer to the
> RP_ENTITY_NAME_ macros?
Sure.
> REPOSITORY_DOOR_VERSION: Shouldn't you bump this?
Yes.
> cmd/svc/configd/rc_node.c
> 2013: Since this function can already fail with _NO_RESOURCES, why not
> malloc() this?
At old 1977, this was allocated on the stack. I see little reason to
change that.
> 2031: I think "inherit" would be more understandable than "compose"
> here.
Will change.
> 2274: Please state that _NO_RESOURCES can only happen when answertype
> is _PGREADPROT.
Will change.
> 2325: Can you put this with the other PG cases, like at 2308?
Sure.
> 2326: cstyle.ms.pdf says this brace should be on the case line.
Will fix.
> 2340: Can you make this
>
> case REP_PROTOCOL_NO_RESOURCES:
> return (ret);
>
> default:
> bad_error("rc_node_pg_check_read_protect", ret);
> }
>
> ?
I don't think _DELETED is a bad_error() here, is it? I think
case REP_PROTOCOL_FAIL_NO_RESOURCES:
case REP_PROTOCOL_FAIL_DELETED:
return (ret);
default:
bad_error("rc_node_pg_check_read_protect", ret);
}
is correct. We can already return _DELETED above.
> 2701:
> - Please add a comment saying that we're doing this here to avoid
> holding locks across permission checks.
Will do. I did add a note to this effect in the block comment at the
top.
> - Would you mind moving the type == REP_PROTOCOL_ENTITY_PROPERTYGRP
> check before rc_node_modify_permission_check()?
No, I don't mind. That seems fine and a slight improvement.
> 2729: Is there any reason not to move this up with the
> rc_node_modify_permission_check() invocation?
I was extremely careful to preserve the exact precedence of error
codes in all of these cases. I'd prefer to leave it as it is.
> 3774: Please add a comment saying that we're doing this here to avoid
> holding locks across permission checks.
Ok.
> 3782-99: These might as well be before
> rc_node_modify_permission_check(), eh?
>
> 3802: This might as well be up with rc_node_modify_permission_check(),
> eh?
Again, I do not want to change the error precedence, even if it's not
documented. That just seems like unnecessary risk.
> 3920-2,67-8,4002-3: You might as well use rc_node_rele_locked() here,
> eh?
Agreed.
> 4101: This seems excessive. Is there a reason for it?
Agreed. I will move it to 4119.
> 4421: I think you've got more locking than necessary here. I believe
>
> RC_NODE_PTR_GET_CHECK_AND_HOLD(np, npp);
>
> ret = rc_node_property_may_read(np);
> if (ret != REP_PROTOCOL_SUCCESS) {
> rc_node_rele(np);
> return (ret);
> }
>
> RC_NODE_CHECK_AND_LOCK(np);
>
> should work, as long as you replace the subsequent
> pthread_mutex_unlock()s with rc_node_rele_locked()s.
Agreed.
> 4706: I think this is supposed to be indented.
cstyle(1) says otherwise. If you look closely, you'll see that it's
already part of a continuation, and is indented 4 spaces. I agree
with your sense of aesthetics, but if this is a bug it's in cstyle.
> lib/libscf/common/lowlevel.c
> datael_get_name(): You must add _NO_RESOURCES for the PGREADPROT case.
Ok.
> lib/libscf/common/midlevel.c
> 307: This should be an scf_error_t.
Ok.
> cmd/svc/startd/libscf.c
> 992,1050,2353,2380,2406,2918,2991,3017: I don't think bad_error() is
> the right thing to do here. Why shouldn't we log a "misconfigured"
> error for this service and continue? At the very least, a better
> error message is warranted.
>
> 1709,1846: I don't think bad_error() is the right thing to do here.
> Why shouldn't we log a misconfiguration error and continue? At the
> very least, a better error message is warranted.
>
> 3201: I don't think assert(0); abort() is the right thing to do here.
> Why don't we just continue to clear the action, as in the
> SCF_ERROR_CONSTRAINT_VIOLATED case?
See my response for cmd/svc/startd/graph.c:5583 et al above. There's
no way for a misconfiguration to have this result, as we are reading
from framework property groups. If configd denies read access to a
framework property group, it's a bug.
> 3519: I don't think bad_error() is the right thing to do here. Why
> shouldn't we pass the error up the stack?
I actually agree on this one. This function at present has only one
caller, which passes in a pg of type dependency, so my response above
applies. But in the future it could in theory be used for other
things. I'll make this change.
> cmd/svc/startd/graph.c
> 3384,6: Isn't this change unnecessary?
Syntactically, yes. To make cstyle(1) happy, no. I'll file a bug
against cstyle for future generations.
> 5598: I don't think bad_error() is the right thing to do here. Why
> don't we log a misconfiguration problem for this service and perhaps
> try to recreate the property group? At the very least, a better
> error message is warranted.
All actions are represented wholly within framework property groups.
> cmd/svc/svccfg/svccfg.y
>
> 47: SCC_RESTORE should be just after SCC_ARCHIVE.
Ok.
> cmd/svc/svccfg/svccfg_libscf.c
> 3074,3121,3387,3517: I don't think bad_error() is the right thing to
> do here. Why not warn about corruption as for
> SCF_ERROR_CONSTRAINT_VIOLATED? At the very least, a better error
> message is warranted.
I suppose I can re-use the "corrupt last-import" error message here.
> 3188,3253,3435,3625,3678: I don't think bad_error() is the right thing
> to do here. Why not propagate the error up the stack? At the very
> least, a better error message is warranted.
>
> 3742,3848,4230: I don't think bad_error() is the right thing to do
> here. Why not do something similar to ECANCELED? At the very
> least, a better error message is warranted.
>
> 4101: I don't think bad_error() is the right thing to do here. Why
> not do something similar to SCF_ERROR_CONSTRAINT_VIOLATED? At the
> very least, a better error message is warranted.
Clearly I'm just dense. What error should we display, "Your configd
is buggy; it incorrectly denies read access to framework property
groups. Please return it to Sun Microsystems, Inc. for a free
replacement."? And how is that really any different from dropping a
core that will tell us the exact same thing in about 5 seconds of
debugging?
> 4269: "(backend access denied)" doesn't apply to your new error.
> Wouldn't it be better to return EPERM for it?
I'll remove the (backend access denied). What is the advantage of
returning EPERM instead of EACCES?
> 7287: This should be SCF_ERROR_NONE.
Ok.
> 7306: I don't understand how this comment applies to this code.
I'll replace this with a more relevant comment.
> 7342: This check for _PERMISSION_DENIED seems unnecessary.
Agreed.
> cmd/svc/svccfg/svccfg_help.c
> 21: This change seems unnecessary.
>
> help_messages: Shouldn't you add an entry for restore?
Yes. I added
{ SCC_RESTORE, "restore file\n\n"
"Restore the contents of a previously-generated archive."
},
in response to another reviewer's similar comment.
> lib/librestart/common/librestart.c
> 1072,1371: You shouldn't abort here. You should return an error
> somehow.
SCF_PG_RESTARTER is a framework property group.
Based on this review, I'll be filing four bugs/RFEs:
inheritance of read_authorization from svc incorrect for snapshots
clean up 6537749 et al to reflect late-breaking code review comments
svccfg export/archive -a could provide better error messages
cstyle(1) should allow do-while without braces
and fixing the first two of these. I'll have a webrev in a couple of
days. The only open issue on which we seem to disagree is
error-handling for _PERMISSION_DENIED failures on reads from framework
property groups.
--
Keith M Wesolowski "Sir, we're surrounded!"
FishWorks "Excellent; we can attack in any direction!"
