David Bustos wrote:
> Quoth Liane Praza on Wed, Oct 08, 2008 at 10:40:04PM -0700:
>> David Bustos wrote:
>>> Quoth Liane Praza on Fri, Aug 22, 2008 at 12:34:53PM -0700:
>>>> http://cr.opensolaris.org/~lianep/webrev-20080822/
>>> lib/libscf/common/scf_tmpl.c
>>> 3479: Why isn't this SCF_ERROR_TEMPLATE_INVALID?
>> Because it's defined not to be. Seemed like the most flexible
>> definition and is carried throughout its companion functions in the
>> manpage and the function comments.
>
> Ok, maybe I don't understand. Is there a way to create a property
> template through the manifest which does not have an
> SCF_PROPERTY_TM_TYPE property?
Sorry, thought I was looking at pg_type. I'll fix.
>
>>> 3797,3817: If the string representation of {val_min,val_max} be
>>> negative, wouldn't it have to be of some type other than count, in
>>> which case scf_value_get_count() would fail?
>> It isn't documented to...
>
> Do you mean scf_value_get_count() is not documented to fail if the value
> is of a type other than SCF_TYPE_COUNT? If so, what version of
> scf_value_create(3SCF) do you have? I'm reading the 10 Oct 2007
> version.
I'm not sure what I was saying. Removed this and the _is_str_neg_val()
call as they're the only callers.
>>> 3972: It's not clear to me why it's better to return
>>> SCF_ERROR_NOT_FOUND in this case than SCF_ERROR_TEMPLATE_INVALID, or
>>> just let the caller decide that zero values is bad. Is there some
>>> reasoning for this?
>> Because, generally, for a property with no values there's no useful
>> result to be returned. The value requested wasn't found. I'm not sure
>> why telling the caller the template is invalid is more valuable here,
>> and I hate having to have each caller include the logic to determine
>> that there are zero values. This seemed like the most friendly and
>> error-resistant calling convention.
>
> Telling the caller the template is invalid could be valuable if there's
> no supported way to end up with that property having no values. Is that
> possible? Or is it legitimate to specify that no values are legal for
> a property? Or are you anticipating a future change wherein it may be
> possible to do that in a supported way?
No specific legitimate cases, but given that there's no actual damage or
bad information here, I don't think there's value in telling the caller
the template is invalid. It's a fetch-a-rock game for the user.
I continue to disagree philosophically on this one. Are you insisting?
> Also, if you're going to fail in cases where _read_astrings_values()
> succeeded, don't you have to call scf_values_destroy()? Particularly
> when you return the same error code in that case as you can when
> _read_astrings_values() fails.
Yes. Fixed.
>
>>> 4162,68,72: Is there a reason to return SCF_ERROR_CONSTRAINT_VIOLATED
>>> instead of SCF_ERROR_TEMPLATE_INVALID?
>> Because the template isn't necessarily invalid.
>
> Perhaps I'm missing something. Doesn't this condition mean that the
> property contains an invalid number? Is there a supported way to end up
> in that state?
>
> Also, how do you forsee a client behaving differently between
> _CONSTRAINT_VIOLATED and _TEMPLATE_INVALID?
The general point is that CONSTRAINT_VIOLATED means you probably called
the wrong function for this min,max pair. TEMPLATE_INVALID means the
template is wrong no matter which one you call.
>
>>> 4812: Will this work with xgettext(1)? Or is that not necessary?
>> Why wouldn't it? I guess I'm not clear on what you're asking.
>
> Calls to gettext() must include literal strings rather than pointers to
> strings so that xgettext can find the strings for the translators.
> I presume that rule also applies to dgettext(), but it might not.
Ah. Got it. I'm fixing this by adding the error arrays and strings to
error.c, which is processed with xgettext -a.
>
>>> 6202: It seems that all of the callers of _add_error() know the type,
>>> so why don't they just call the appropriate _add_tmpl_*() function
>>> directly?
>> Keeps all the logic in one place rather than spread all over, I guess.
>> If you feel the other paradigm would be tangibly better, we can change it.
>
> I think it would be better because it would eliminate having to find and
> read _add_error() to determine which arguments should be set.
OK, fixed.
>>> cmd/svc/svcs/svcs.c
>>> cmd/svc/milestone/global.xml
>>> 30: I wonder if we should omit the "Make customizations..." directive
>>> here.
>> Why? With Profiles, that will be exactly the way customizations are
>> possible. I'm also not sure why we would change this one and not all of
>> the manifests which use this block comment.
>
> I meant global.xml specifically, not all manifests. But maybe I'm
> mistaken. Doesn't global.xml correspond to the reading rules of
> svc.startd & librestart, which we control? If so, it seems that there's
> no legitimate reason for a user to customize them. Do you have some use
> case in mind?
An admin may eventually wish to override with a different description,
or perhaps even additional constraint for their site. I believe we
talked about these use cases early in the project.
>>> 78: I think this file would be easier to interpret if you included
>>> a comment here saying who creates general property groups and why,
>>> and what components read general property groups, since this is
>>> essentially a codification of their reading rules. (svc.startd and
>>> librestart in this case.) Similarly for 111, 249, 325, and 492.
>>> Though I suppose for 249 and 325 you can just say that the templates
>>> describe a repository representation which is not stable / not
>>> public.
>> I've chosen to augment the public descriptions rather than add comments
>> seen only by developers and the morbidly curious.
>
> Sorry, which public descriptions are you talking about?
The template "description" for the property group.
>
>>> 89: What would it take to wrap this line in 80 columns?
>> I haven't come up with a satisfactory way yet. :/
>
> Well is it legitimate to have newlines in prop_pattern/description/
> loctext's?
Yes, but then the newline is then encoded in the string.
> If not, you could just break the line and have svccfg join
> them together. Otherwise, couldn't you introduce a way to escape
> newlines? It's ok if these are too distasteful -- I just want to
> understand the alternatives.
This is an existing problem that I'd prefer not to tack onto this
project, but if you insist, we could explicitly choose to strip newlines
from loctexts on import, though I worry about the reduction in later
flexibility to fix essentially code formatting.
>>> cmd/svc/milestone/restarter.xml
>>> 80: Isn't restarter created by librestart? And aren't restarters
>>> required to use librestart? If so, it seems to me that it should be
>>> in global.xml. In any case, I think you should add a comment
>>> describing what creates restarter property groups and what readers
>>> these rules correspond to.
>> Not all restarters use only these properties, or are required to use all
>> of them. The restarter property group is strictly owned by the
>> restarter, even though it is required to set a very small set of the
>> properties.
>
> Are you saying that it's ok for restarters to bypass librestart?
I'm saying not all of the properties are managed by librestart.
>>> 446: I take it that the template validation code doesn't know how to
>>> validate values separated by the declared internal separators?
>>> Should "signal,core" also be included as a valid choice?
>> It isn't in startd right now. (Which should be fixed -- we should fix
>> it to actually accept a value list, but... that's a separate bug that
>> I'm not addressing now.)
>
> By "It isn't in startd right now", do you mean that startd will not
> interpret "signal,core" in the same way as "core,signal"?
My apologies, I was in error. startd does accept both.
> And I don't understand how your response answers my first question;
> please forgive me!
Sorry. No, it currently does not.
>
>>> - Did you consider separate patterns for the start, stop, and
>>> refresh methods?
>> Yes, but I didn't feel like there was enough benefit.
>
> Wouldn't it allow us to declare that start and stop methods are
> required? And won't the proposed change validate methods that we know
> don't do anything, like "restart"?
True. Are you requesting a change?
>>> cmd/cmd-inet/usr.lib/inetd/inetd.xml
>>> 216,259,343,368,411,440,486,495: These should be counts, shouldn't
>>> they? Is there a bug filed for that?
>> Whether or not it was a mistake originally (it was), I don't think
>> breaking all inetd manifests is worth it to change this now or in the
>> future. I'm declining to file a bug.
>
> Won't this cause user interfaces (such as "visual panels") to allow
> negative values to be specified, which will be interpreted as enormous
> timeouts? Maybe I'm missing something.
Perhaps, but I don't see how that risk outweighs against breaking all
inetd manifests in the wild.
>>> 494: How many RPC versions are there?
>> Depends on the service. Not sure what you're trying to get at.
>
> Well if we know what the legitimate versions are, we can specify
> constraints, can't we?
Clearly I'm missing something. Where am I specifying constraints on RPC
versions?
liane
