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
2563: I believe this is when the iteration is complete, in which case
it returns the opposite of what is documented in
scf_tmpl_get_by_pg_name(3SCF).
2764: Shouldn't you fail if flags is nonzero?
2869: Why would this fail if a property has no value?
2872: Is there a way to phrase this description in terms of the
parameters instead of local variables?
2888: Shouldn't this fail if flags contains bits other than
SCF_PROP_TMPL_FLAG_REQUIRED?
2945: cstyle says lines shouldn't begin with binary operators.
3097: What does 'pname' refer to?
3138: If _malloc_single_astring_from_pg() failed with
_CONSTRAINT_VIOLATED or _TYPE_MISMATCH, shouldn't the function fail
instead of trying the C locale?
3173,3204: It appears that these functions actually return -1 on
failure.
3180,81,3211,12: What does 'name' refer to?
3243,46: What properties does this text refer to?
3317: What does 'pname' refer to?
3322,89,98: I would treat use_malloc as a boolean rather than
explicitly comparing to 1. But it's not a big deal until someone
tries to invoke the function with use_malloc > 1, which might be
never.
3465: What property does this text refer to?
3479: Why isn't this SCF_ERROR_TEMPLATE_INVALID?
3525: I think it would be clearer to say that the property group which
represents t was deleted.
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?
3827: Shouldn't this be UINT64_MAX? That's what the manpage says.
3916: Shouldn't this function verify that the values are single
characters? Or are you leaving that for an RFE later?
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?
4005: Shouldn't this be impossible? Like a bug in strtok_r() or
memory corruption?
4017: I think the fact that this results in an error should be
described in the comment.
4077: Is there a reason this isn't in the more conventional
for (n = 0; n < vals.value_count; ++n) form?
4154: Is there a reason this isn't in the more conventional
for (i = 0; i < vals.value_count; ++i) form?
4157: cstyle says lines shouldn't start with binary operators.
4162,68,72: Is there a reason to return SCF_ERROR_CONSTRAINT_VIOLATED
instead of SCF_ERROR_TEMPLATE_INVALID?
4238: What does this comment mean?
4396: I don't understand why you're doing what you're doing. Would
you mind inserting a comment explaining what you're trying to
accomplish and how you're going about doing it?
4506: Is there a reason for not using the more conventional
for (i = 0; i < vals->value_count; ++i)?
4579: "<lang>" isn't appended as indicated in the comment at 4547.
4586,4628: It appears these functions return -1 on failure.
4703: "fileds" -> "fields".
4709: Can you add some text explaining how scf_tmpl_errors fits into
the error reporting scheme?
4722: "Tempaltes" -> "Templates".
4813: This break should be unnecessary.
4812: Will this work with xgettext(1)? Or is that not necessary?
5257: Should the parameter be called "garbage_collect_strings" or the
like?
_scf_create_errors(): Is prefix passed as something other than NULL
anywhere?
5347: Is there a reason this isn't assert(tmpl_fmri != NULL)?
5369: I don't see how this validates err. It seems to validate
_tmpl_error_items[], which is static.
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?
_value_in_constraint(): Please explain what this function is supposed
to do in its comment.
6334: Since you switch on the type anyway, this seems unnecessary and
leads to excessive indentation.
6341: Another way to do this is
r = scf_value_get_count(value, &v_count);
assert(r == 0);
6343: cstyle says lines shouldn't begin with binary operators.
6408: Shouldn't this always be true, due to 6310?
6421: Is there a reason not to use the more conventional
for (n = 0; constraints[n] != NULL; ++n) here?
6440-3: What's the difference between _OUT_OF_RANGE and
_RANGE_VIOLATION? Why do pg & prop make a difference here?
6527: "lenght" -> "length"
6535: Since the first operation on buf is always snprintf(), this
seems unnecessary.
6584: I think I've always seen ARGSUSED before the function's return
type.
6618: Shouldn't you compare max to UINT64_MAX?
7181,7201: cstyle says lines shouldn't begin with binary operators.
7297: Shouldn't you fail if flags contains invalid flags?
7281: I've only seen ARGSUSED before the function's return type.
lib/libscf/inc/libscf_priv.h: ok
cmd/svc/svcs/svcs.c
2152: I think it would be clearer to change "name" to "pg" here, and
probably to specify "type" as either "pgtype" or "proptype" in the
next few lines.
2162: This should always return SCF_PG_APP_DEFAULT, right?
2175: Will we print property groups which only contain hidden
properties?
2193: cstyle says lines should never begin with a binary operator.
2211: Shouldn't you quote values with embedded spaces? The ARC case
implies that you will.
cmd/svc/milestone/global.xml
30: I wonder if we should omit the "Make customizations..." directive
here.
47: Is there a reason the instance is enabled?
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.
89: What would it take to wrap this line in 80 columns?
91,100,108: Shouldn't this specify a cardinality of 1?
124: Should this specify a cardinality of at least 1?
128: Does "entity" refer to the entities property?
138: I think this would be clearer as something like "How the states
of the services specified by entities should be used to decide
whether this dependency is satisfied." Or maybe just "How to decide
whether this dependency is satisfied."
203,210: These should probably say "Restart if the dependency...".
257: Shouldn't this specify hidden visibilty, since users shouldn't
manipulate it directly? Similarly for the rest of the template
properties.
279: I think it would be more precise to say "If true, entities
without a property group which matches this pattern are considered
invalid."
288: This is pretty reflexive. I think it would be more informative
to say something like "The entities / service instances to which
this template / pattern should be applied."
296: Would this be more informative as "The service or instance on
which the property group resides."?
501: This should probably say "default execution context", since
method property groups can be more specific.
510: I believe processes have working directories instead of home
directories. And I think "home directory of the user" is missing.
528: I thought common names weren't supposed to begin with capitals.
And this name is pretty reflexive. What about something like
"resource pool for method process"? Similarly for 546, 560, 574,
589, 603, and 635.
620: This isn't informative. It should mention RBAC. Though maybe
common_name doesn't make sense for this property, since it isn't
really used in common communication, or make sense outside of the
property group.
cmd/svc/milestone/make-console-login-xml
142: Why is this required? It seems that the method provides
a default.
145: I thought common names weren't supposed to be capitalized.
150: I don't understand what "(not boot)" means.
190,205,225,240,256,271: Why are these required? It seems the method
allows them to be missing.
193: I thought common names weren't supposed to be capitalized. And
since this doesn't literally contain line settings, I think it
should include the word "ttydefs".
208: I thought common names weren't supposed to be capitalized. And
does it make sense to list a common name if it's going to be the
name of the property?
228,259,274: I thought common names weren't supposed to be
capitalized.
233: I think this should be qualified with a time.
252: Is an empty cardinality element any different than no cardinality
element?
cmd/svc/milestone/restarter.xml
51: Please add a comment explaining that svc.startd is started by
init, and svc.startd knows not to use these methods. So you should
probably also explain why you're including them at all.
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.
137: Shouldn't this be "type='time'"? (Hmm, did this not cause a test
failure?)
167: "Most instaces are in this state..." is easy to interpret
wrongly. It would less ambiguous to say something like "The most
common reason for service instances to be in this state..."
199: As far as I know, nothing sets restarter/state to "legacy_run".
svcs only displays it as the state for entries in smf/legacy_run.
253: Don't some components set next_state to "none"?
255: Is there a reason there is no pattern for the alt_logfile
property?
380: Please add a comment describing what creates this property group
(service manifests) and what reading component these rules describe
(svc.startd).
400: To differentiate this option from "child", you should probably
say that the service is implemented by processes in a process
contract. Or are process-contract-unaware or something. Or maybe
that it will not be considered online until the start method process
exits.
414: I think this would be more precise as "The service is implemented
by a process which should be considered online as soon as it is
started, and child processes should be ignored", or similar.
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?
459: If I were you, I'd refer to setpgrp(2) here.
475:
- Please add a comment describing what creates this property group
(service manifests) and what reading component these rules
describe (svc.startd).
- Did you consider separate patterns for the start, stop, and
refresh methods?
520: If this is true, then why isn't there a constraint?
538,550,585,603,617,631,660,677,692,709: I thought common names
weren't supposed to be capitalized.
567: I think processes are usually described as having working
directories, rather than home directories.
640,654,669: If these have single-cardinality, shouldn't they have
internal separators?
677: This common name isn't very informative. I wonder if it makes
sense to provide a common name at all, since this property just
governs which other properties to use.
cmd/cmd-inet/usr.lib/inetd/inetd.xml
60: Is this change necessary?
216,259,343,368,411,440,486,495: These should be counts, shouldn't
they? Is there a bug filed for that?
494: How many RPC versions are there?
590: Doesn't it make sense to include a constraint here?
633: I think "working directories" are usually associated with
processes, rather than "home directories".
651,669,683,697,712,726,743,758,775: I thought common names weren't
supposed to be capitalized.
707,721,736: If these have single-cardinality, shouldn't they have
internal separators?
743: This isn't very informative.
795: Shouldn't this be "type='astring'"?
805: Shouldn't this be "type='time'"?
916: Why no pattern for contracts?
lib/libuutil/common/libuutil.h: ok
lib/libuutil/common/mapfile-vers
99: It appears that the rest of these symbols are alphabetized. Is
there a reason this isn't?
lib/libuutil/common/uu_alloc.c: ok
pkgdefs/SUNWcsr/prototype_com: ok
David
