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
>
> > 2888: Shouldn't this fail if flags contains bits other than
> > SCF_PROP_TMPL_FLAG_REQUIRED?
>
> Traditionally, we haven't done this type of checking.
Hmm, scf_*_add_pg() does, but scf_maintain_instance() and
scf_handle_decode_fmri() do not. I suppose there's not much advantage
to it.
> > 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?
> > 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.
> > 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?
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.
> > 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?
> > 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.
> > 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.
> > 7297: Shouldn't you fail if flags contains invalid flags?
>
> Again, this isn't typical checking.
Ok.
> >cmd/svc/svcs/svcs.c
>
> > 2175: Will we print property groups which only contain hidden
> > properties?
>
> Yes, as I'm sure you've already noticed, we will currently print the
> name and type of the property group. I suppose I could delay the
> printing if you think it's important not to.
I think this boils down to an estimation of the users. If there's no
way to mark a property group hidden, and a developer marks all of the
properties in a property group as hidden, would it cause unnecessary
consternation in his users to list the property group? My estimation is
yes, but I trust your judgement here better than mine.
> >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?
> > 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?
> > 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? 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.
> >cmd/svc/milestone/make-console-login-xml
>
> > 233: I think this should be qualified with a time.
>
> I don't really understand. I've rephrased using some of the manpage
> wording to try to make things clearer, but there's still no statement
> about the time.
Yes, I don't understand either.
> >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?
> > 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"?
And I don't understand how your response answers my first question;
please forgive me!
> > - 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"?
> >cmd/cmd-inet/usr.lib/inetd/inetd.xml
> > 60: Is this change necessary?
>
> No, but it makes the formatting consistent with the rest of the file.
Ok.
> > 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.
> > 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?
> > 916: Why no pattern for contracts?
>
> Because they're not stored in the repository anymore.
Oh, right.
David
