Quoth Liane Praza on Fri, Aug 15, 2008 at 03:41:37PM -0700:
> Tom, Antonello, and I would like to invite reviews of the code for
> extending the existing SMF templates to incorporate general service
> property metadata.
>
> We previously published and discussed ARC materials on this alias, so
> the interfaces and goals of the project shouldn't be much of a surprise.
>
> I'm re-posting our manpage diffs and interface information here for
> reference, though it is not under review:
> http://cr.opensolaris.org/~lianep/templates-0815/
While reading about the interfaces, I came across some text which
I think many users will find unclear. And during the review, I had some
questions. Since this documentation is not under review, however, feel
free to ignore.
smf_template(5)
"Templates may be defined for a service to describe metadata about
the service in general...": I think this would be more useful to
users if it were more concrete. Specifically by describing when
users might want to produce or consume templates. Perhaps saying
that service developers use templates to describe what
configuration values are valid, administrators can use templates
to validate configuration, and tool developers can use templates
to provide more helpful configuration user interfaces.
"svcs -lv...": Perhaps you should specify that the output of svcs
-lv and svccfg describe are human-readable, and for programmatic
or stable access the libscf interfaces should be used.
"A template is created...": This paragraph confuses me because the
first sentence says what creates a template, but the rest
describes, I think, how service developers should write templates
in their manifests. Though maybe it's trying to describe which
templates should be used for which properties. I suppose it turns
on the definition of "templated".
"Templates defined globally...": I think you should clarify when
a template is considered to re-define another template. Do they
have individual names? Or is it based on their matching criteria?
It seems this information is most relevent to template authors,
except for when template consumers need to understand that
configuration validation can fail due to miscoordination between
different template authors. So I'm not sure that the "Template
Composition" section is the best place for this.
"Future work...": Is future work appropriate for a manpage?
"Capital letters...": I suspect this should be qualified with "in
English locales".
"If name or type is omitted, it acts like a wildcard.": I think you
mean that the template acts as though it has a wildcard for that
field. Though saying that the template can act in such a way
might be considered bad diction.
"This property lets us declare which.": First person doesn't seem
appropriate for a manpage.
"internal_separators tells us...": Can this be deprecated now that
SMF Property Value Ordering has been integrated?
svccfg(1M):
"... if invoked with the -s option...": -s is missing from the
synopsis.
scf_tmpl_pg_name(3SCF):
"this funtion will return a string containing SCF_TMPL_WILDCARD":
SCF_TMPL_WILDCARD doesn't seem to be in the interface table of the
ARC case. Is it appropriate to mention it here?
"The caller is responsible for freeing the *out buffer on success.":
I presume you mean the caller should use free(3C). Though in the
past, the library was required to provide its own freeing function
in case malloc() or free() were interposed, or the function used
an allocator in a library other than libc. Is that no longer
necessary due to direct bindings?
"The scf_tmpl_pg_target() function will retrieve the property
group's target as currently templated...": I don't understand what
you mean by a property group's target. Do you mean the template's
target?
scf_tmpl_prop_name(3SCF):
scf_tmpl_prop_internal_seps(): I don't understand how this function
will fill the scf_values_t. Will there always be a single element
for the entire string of separators, or will there be a separate
element for each character of the separators string?
scf_tmpl_validate_fmri(3SCF):
"The template validation functions offer a way to validate an
FMRI...": Really you're validating the configuration data associated
with an instance, eh?
"By default, the validation functions look up composed data...": Is
this referring to the data to be validated, or the data to
validate against?
> The code review, parented to the onnv_95 snapshot is here:
> http://cr.opensolaris.org/~lianep/webrev-20080815/
So far I've reviewed svccfg's code.
cmd/svc/svccfg/Makefile: ok
cmd/svc/svccfg/svccfg.h
184: Please add a comment explaining that sc_pgroup_composed is
created & destroyed by compose_props() & composed_pg_destroy().
cmd/svc/svccfg/svccfg.l: ok
cmd/svc/svccfg/svccfg.y
133: This doesn't implement the -s option documented in
svccfg_1m.diff. Which is correct?
143: I'm not sure this should be flagged as a syntax error. It seems
more like a semantic error to me (and therefore should use
semerr()), since the syntax of the command is correct, but the
command is wrong due to the context.
420: cstyle calls for NULL instead of 0 for pointers.
cmd/svc/svccfg/svccfg_engine.c
584: lxml_get_bundle_file() seems to call semerr() before returning
any errors. So it seems that this error is superfluous. Am
I missing something?
engine_import(): svccfg_1m.diff says -V is the default in interactive
mode, but I don't see where that is implemented.
599: This error isn't very informative. Are there cases where
tmpl_errors_print() doesn't print an informative error message?
608: I believe lscf_bundle_import() prints an informative error
message before returning an error code. So it seems that this error
is superfluous. Am I missing something?
cmd/svc/svccfg/svccfg_help.c
33: Shouldn't this be updated?
102-3: This is the only place "settings" is used. Can we use
"properties" instead?
cmd/svc/svccfg/svccfg_internal.c
internal_pgroup_create_unique(): I think
internal_pgroup_create_strict() would be a more intuitive name for
this function, though I suspect internal_pgroup_create() with
a comment would be sufficient.
load_instance():
- Please note in the comment that the property and property group
patterns of the composed instance are loaded also.
- Shouldn't inst be scf_instance_destroy()ed before returning?
- I think you should return ENOENT for a lot of the
SCF_ERROR_DELETED failures, since the caller shouldn't care
whether the instance didn't exist in the beginning, or was deleted
halfway through.
1186: I think you should note that fmri will be referenced by
*inst_ptr (rather than being copied) in the function comment.
1205: This should be "scf_pg_get_type", since that's the function that
returned the bad error. And the following break is superfluous
since bad_error() never returns.
1222: This should be bad_error("load_pg", rc). And the following
break is superfluous since bad_error() never returns.
cmd/svc/svccfg/svccfg_libscf.c
66: I think it would help future modifiers of this code to add
a comment explaining when these macros are used.
5810: Why are you changing this? From what I can tell, imp_tsname is
allocated to be max_scf_name_len + 1 bytes long, at 6754.
lscf_validate_fmri(): Please add a comment explaining what this
function is supposed to do.
9428: Shouldn't inst_sz be of type size_t?
9439: Is there a reason this isn't semerr(gettext("..."))?
9444,59: I don't understand why these messages end with colons and
newlines before exiting. But they doesn't seem helpful anyway.
I think it would be more helpful to inform the user that either
there is a defect in libscf, or memory was corrupted.
9445: This should be superfluous since uu_die() should never return.
9448-54: It doesn't seem wise to me to use lscf_select() in this way.
I suppose because lscf_select() is an entry point, so someone who
modifies it should be entitled to the assumption that it is not
being called from another entry point. I think it would be better
to factor lscf_select() into an entry point and an
argument-accepting utility, which is invoked here. I suppose it
doesn't need to be changed before you integrate, but at the very
least, a comment should be added before lscf_select() saying that
it's being used in this way.
9460: This should be superfluous since uu_die() should never return.
9478: By my reading of
http://cr.opensolaris.org/~lianep/templates-0815/scf_tmpl_validate_fmri.3SCF.new
,
scf_tmpl_validate_fmri() doesn't accept SCF_PG_TMPL_FLAG_CURRENT.
Do you mean SCF_TMPL_VALIDATE_FLAG_CURRENT?
9478: Since the only statement in this if statement tests ret, this
test seems redundant. Couldn't we improve readability by
eliminating it?
ret = scf_tmpl_validate_fmri(...);
if (ret == -1) {
...
} else if (ret == 1 && errs != NULL) {
...
}
if (errs != NULL)
...
9486: len should be const. And shouldn't it be size_t? And there
should probably be a comment explaining its value, even if it's
arbitrary.
lscf_validate_file(): Shouldn't this be static?
9539: I believe this (char *) cast could be eliminated by making str
const char *.
list_pg_tmpl(): I think it would help future modifiers of this code to
add a comment which explains what list_pg_tmpl() should do, and
particularly what how the templates argument affects behavior.
10894: scf_value_get_astring() returns the length of the string,
rather than SCF_SUCCESS, on success. I think you mean "== -1".
10921,30,11162: Won't we print this if scf_tmpl_pg_name() fails? Why
don't you print an error? Do you mean to detect the wildcard? If
so, the ARC materials say you should test for "*".
10935,11180: Shouldn't "true" & "false" be in gettext()? They are in
svccfg_tmpl.c.
10995: I don't understand why you'd use quote_and_print() if the
string contains a space, tab, newline, or parentheses, since
quote_and_print() doesn't quote any of those characters.
11018,64: Is there a reason not to use for (i = 0;
i < values.value_count; ++i) here?
list_prop_tmpl(): Please add a comment explaining how prop and
templates arguments affect the behavior of this function.
11198: What's the difference between this line and the other side of
the if?
get_template_pg(): This function doesn't seem to be template-specific.
Can you change the name? And move it to an area for utility
functions, either the top or the bottom of the file.
11252: Why die on SCF_ERROR_DELETED?
11270: This break should be superfluous because uu_die() shouldn't
return.
11294: Why do we continue if the repository connection was broken?
11304: Should there be a new macro for indentations twice as long as
TMPL_INDENT?
11342: cstyle says labels must be on their own lines.
11368,97: Why are these hardcoded for the "C" locale?
11370: Why die if pg was deleted? Why not act as though it never
existed?
11383,410: Why goto done if scf_value_get_ustring() fails with
SCF_ERROR_NOT_FOUND? It's not documented to do so, so I think you
should inform the user that libscf was defective, or memory was
corrupted, and maybe abort.
11428: Why die if the current selection was deleted? Why not issue an
error and return?
11440: val & iter are never destroyed.
11444: Is this legal? I've only seen ARGSUSED before the function.
11493: Won't prop_name always be NULL here? Why not just use NULL?
13662: Can you add a comment explaining that we have to set optind to
0 because the arguments start at argv[0] rather than argv[1]?
13685: This line seems superfluous since argc isn't used again.
cmd/svc/svccfg/svccfg_main.c: ok
cmd/svc/svccfg/svccfg_tmpl.c
33: Could you mention the entry points and main data structures of
this file? And maybe the main sections of code in the file? E.g.,
the im_tmpl_error_print() code could be called out as a significant
block.
75-294: There are a lot of types here. Can you add a comment which
gives an overview of which types are used in what parts of the code?
This could go into the file comment. Or maybe put in a few comments
that divide the types into sections.
composed_pg_destroy(): If cpg_instance_pg->sc_pgroup_composed is
destroyed by composed_pg_destroy(), why isn't it in composed_pg_t?
Are there places were you have access to the instance pgroup_t but
not the composed_pg_t?
431: Shouldn't you pass pg or cpg as the parent?
511: Why not pass inst as the parent here?
build_composed_property_groups(): This function is only called at the
end of build_composed_instance(). Does it make sense as a separate
function?
620: Please add a comment explaining that we don't need to free the
strings because they should point to buffers owned by property_t's.
626: "inialize and" should probably be "initialize an".
778: Isn't this an error that we should catch with an assertion?
809: Couldn't v be NULL?
find_count_value(),find_tmpl_property(): I didn't think properties
can have values of different types. In that case, it should suffice
to just use the first value on the list.
find_tmpl_property(): Doesn't this do the same thing as
find_astring_value_in_pg()?
find_restarter(): Why doesn't this function use sc_composed?
get_ranges(): If count is a count of elements, it should be an
unsigned integer rather than a size_t.
1005: This won't detect garbage after the number. For that you should
test that *endptr2 == '\0'.
scf_terrors_create(), scf_terrors_destroy(): Would you mind renaming
these functions so they don't begin with scf_? Otherwise it's easy
to think they're defined in libscf rather than svccfg.
1041: Shouldn't this say "Given a property group and ..."?
1043: I find "template prop_pattern information" difficult to
understand. Do you mean just "template information"?
1067: I believe you should allocate a buffer of limit + 1 bytes, not
limit.
1073: Couldn't the string be too long if prop_name is too long, which
is more of a problem with the manifest rather than the template? Or
is it impossible to tell which is too long?
im_tmpl_error_create(): This function is only called by
tmpl_errors_add_im(). Is it worth being on its own?
im_tmpl_error_destroy(): This function is only called by
tmpl_errors_destroy(). Is it worth being on its own?
1140: Would you mind describing the overall architecture here?
Specifically that im_tmpl_error_t's are printed by
im_tmpl_error_print(), which dispatches out to the im_perror_*()
functions, which use im_perror_item() as a utility. And maybe the
general form of how the output will look.
1215: Are you sure the strings won't have embedded quotes? If so,
please add a comment.
1390: I believe you're missing a space between "attribute" & "is".
LLDIGITS: Isn't sizeof (uint64_t) always 8? Do you mean
sizeof (long long)? Can you add to the comment that this is based
on the idea that 10 digits are required to represent 32 bits?
Though if we're going to print numbers that large, I suspect we
should use thousands separators. For the users.
1464: The lltostr(3C) manpage I read says the function is undefined
for negative values. Don't you have to pass the absolute value, and
prepend a negative yourself? Maybe it would be easier to use
snprintf().
add_scf_error(): This function is long enough to justify a block
comment explaining what it should do, I think.
1548: Please break this assertion up into the two sub-conditions:
assert(einfo != NULL);
assert(einfo->ei_type == EIT_CARDINALITY);
This allows us to tell which condition failed from just the error
message.
1682: All values of a property should have the same type, so searching
for one of type boolean is unnecessary. You should just use the
first value on the list.
load_general_templates(): Restarters are attributes of instances, not
services. I suspect this means you need to move the sc_restarter
field of entity_t from sc_service to sc_instance.
1727: You should not give up on the restarter because the the
service instance in question is a restarter -- restarters can be
delegated to restarters, too.
1832: Why not use composition here?
1980: Since this iterator is intended to iterate over multiple
instances, this comment should indicate such.
pg_iter_next(): I think the name of this function is too generic.
I recommend something like next_possible_pattern_pg() or similar.
2132: By scope, do you mean the entity which specifies the pattern?
If so, I suspect 'origin' or the like would be easier for future
coders to understand. Particularly since "scope" doesn't appear in
the proposed smf_template(5).
2133: Would it be counterintuitive to reverse the sense of this table?
That is, I suspect it would be clearer to list which scopes should
be applied for which targets, though it's possible that I just don't
understand how the function will be used very well yet.
pg_target_check(): Would this function make sense to call if
pg_pattern was not just returned from iter? I suspect that it would
be a cleaner interface to accept the tmpl_scope_t instead of the
pg_iter_t, to reduce the assumptions the function makes about the
caller.
2362: Can't you just use strlen(pfx->pp_prefix) here? It seems that
would obviate prop_prefix_t.
tmpl_scan_general(): This seems a lot like gather_pattern(). Could
they be combined someday?
2623: "server" -> "service".
2770: What should a user do if he sees this message?
2883: I presume you do this even if pg_patterns[i] isn't NULL because
you consider it an error for manifests to redefine patterns. If so,
please add a comment explaining this.
tmpl_required_pg_present(): Shouldn't this be static?
cmd/svc/svccfg/svccfg_xml.c
1321: Is this enough information for the user to know how to fix the
problem?
lxml_get_common_name(): This function is only used in
lxml_get_tm_common_name(). Does it make sense to have both?
1456: Too much indentation.
lxml_get_description(): This function is only used in
lxml_get_tm_description(). Does it make sense to have both?
1512,1555: It is not appropriate to die here because DTD validation
won't catch these problems.
1706: Would DTD validation catch this? If not, it's not appropiate to
die here.
1825: I don't think it's good style to break an argument name from its
type.
1913: It's not appropriate to die here because DTD won't catch this
problem.
1921: "two elements" -> "two attributes"
1925: I suspect this should be "name of prop_name".
1955: Why is this max_scf_name_len and not max_scf_value_len + 1?
1958: I think it would be better to assert that max_scf_name_len is
larger than the bytes required to represent two 64-bit numbers. But
if you insist, at least change "to long" to "too long".
2043,50,58: Is it possible for us to tell the user how many bytes too
long the name is?
2289: It's not appropriate to die if prop_pattern_name is empty
because DTD validation won't catch it.
2295: It's not appropriate to die here because DTD validation won't
catch the problem.
2302: Can we tell the user how many bytes too long the name is?
2313: How can this happen? If the user includes a property_group by
that name? If the user included duplicate prop_pattern's?
2438: This can only happen if svccfg and the DTD are out-of-sync,
right? In that case, I think you should die instead of warn.
2444,49: How would these happen?
2502: I think it would be more accurate to say that patterns with
target "all" are only allowed on the global service.
2550: Can we tell the user how many bytes too long the type is?
2559: It's not appropriate to die here because DTD validation won't
catch this problem.
2575: Can we tell the user how many bytes too long the name is?
2589-605: It's not appropriate to die here because DTD validation
won't catch this problem.
2618: It's not appropriate to die here because DTD validation won't
catch this problem. Plus, verify_pg_pattern_attributes() already
printed a more precise error message, so this one isn't useful.
3016: It's not appropriate to die if sc_bundle_name is empty because
DTD validation won't catch that problem.
lib/libscf/inc/libscf.h
scf_values_t: These member names don't match the ARC materials.
scf_tmpl_visibility_to_string(): The type of this argument doesn't
match the ARC materials.
11214: Shouldn't this %d be %lld?
lib/libscf/inc/libscf_priv.h
_read_tmpl_prop_type_as_string(), _read_single_astring_from_pg():
Please include "scf" in the names of these functions so it's obvious
that they're defined in libscf.
I'll continue reviewing the rest in the new webrev.
David
