> I looked at the review here: http://cr.grommit.com/~tonyn/6537863/webrev/ > > Oops, you know exactly what I was referring to :^) > * FYI, I was briefly worried about the changes being a bit weird from > a auths perspective. That is, consumers who might have sufficient > auths to modify the property but not to create the property group > would get a SCF_ERROR_PERMISSION_DENIED from > smf_[enable|disable]_instance(). I believe I've convinced myself > that's at worst OK, and at best entirely appropriate, but wanted to > at least raise the point for your consideration. Make sure you've > thought through the auths and are similarly convinced you're doing > the right thing too. > > Good catch. I agree that we're probably ok here.
In the general temporary enable situations, general_ovr pg will be created. If user doesn't have sufficient auths, we will fail at attempting to create general pg if it doesn't exist which is perfectly reasonable. In the case where general_ovr already exists and general pg doesn't, we will fail when attempting to create general pg. This will at worst comparable to the current behavior where we allow users to temporary enable which doesn't really have any effect. If we fail because user doesn't have appropriate auths, it's at least an explicit error. > svcadm.c: > 582 I'd be more explicit in the comment. "An instance's configuration > is incomplete if general/enabled doesn't exist. Create both the > property group and property here if they don't exist.", or something. > > Agree and done. > 593 Set explicitly to disabled, not to enable's value. > > Just need to be consistent with the comment :^) Corrected. > 653 But, if we hit "again" it's because someone else deleted the > property out from under us. (ECANCELED from get_bool_prop().) > I think you need to leave the previous pg_get_or_add() call here. > > You're right. I thought making sure general/enabled exists earlier still leaves a window where the property may get deleted. I put back the pg_get_or_add call. > midlevel.c > 545 You need to use SCF_PG_GENERAL_FLAGS, not pgflags here. > Yes, corrected. Thanks, Liane. tony
