Quoth Liane Praza on Fri, Aug 22, 2008 at 12:34:53PM -0700:
> Full webrev and incremental from the 0815 review are here. The old
> version is preserved at its original location.
> http://cr.opensolaris.org/~lianep/webrev-20080822/
> http://cr.opensolaris.org/~lianep/webrev-20080822-in/
I wasn't able to finish the rest, but here's what I have before I go on
vacation.
cmd/svc/dtd/service_bundle.dtd.1
681: Rather than "Identifies a possible property value," I think it
would be clearer to someone who is first learning about templates to
say "Describes a legal value for a property," or similar.
687: Rather than "The literal property value referenced...", I think
it would be clearer to someone who is first learning about templates
to say "A string representation of the value," or similar.
700: Rather than saying, "can take on", I think it would be clearer to
say something like "... descriptions for legal values of
a property". Or maybe "allowable" instead of "legal". The point
being that I think some people would infer from the current language
that SMF will not allow unmentioned values, whereas it's the service
implementation which will accept or reject them.
805: To me, "holds a set of property descriptions" means the elements
can describe multiple properties. I believe that isn't the case...
that each prop_pattern describes a single property. If that's
correct, please adjust the description.
811: Please note that type may be omitted if required is false.
831: Similarly to 805 for "holds a set of property group
descriptions."
lib/libscf/Makefile.com: ok
lib/libscf/common/lowlevel.c
scf_encode32(), scf_decode32(): Is there a reason for the placement
between _scf_request_backup() and _scf_repository_switch()?
lib/libscf/common/mapfile-vers
31-32: I don't know all of the details about mapfiles, but
scf_decode32() & scf_encode32() aren't in the interface table of the
ARC case, so I thought they belong in the SUNWprivate_1.1 section.
lib/libscf/common/midlevel.c
1129,1673,1818: Since scf_limit() returns -1 when it fails, won't
these changes turn the error handler into dead code?
lib/libscf/common/scf_tmpl.c
*: I believe many of the functions in this file should be static.
Please check them.
28: Please add a comment giving an overview of the entry points of the
file, any major data structures, and how the file is organized.
253: Isn't scf_type_to_string() documented to never return NULL?
324,29: cstyle says if statements should be broken across two lines.
Though it is legal to call scf_value_destroy() and
scf_property_destroy() with NULL arguments.
316: Please break this assert()ion into two, so we can tell which
failed without the core.
366: I think you should abort() on this condition, or just assert()
it, since your invocation of scf_limit() should only fail due to
programmer error.
394: Please explain why it's preferable to define a malloc() version
of _read_single_astring_from_pg() than the alternatives. Namely,
making _read_single_astring_from_pg() accept an argument dictating
whether to use uu_zalloc() or malloc() and making all of the callers
of _read_single_astring_from_pg() use uu_free().
542: Please add some text giving an overview of what this function is
supposed to do. Note that it skips over zero-length and duplicate
values. And maybe that the duplicate-detection algorithm won't
scale very well.
569,603,659: Please break these assert()ions up.
580: Please add a comment explaining that the array may be bigger, but
it doesn't matter because we'll always allocate a new one.
611: cstyle.ms.pdf calls for a blank line after local declarations.
664: cstyle.ms.pdf says if any arm of an if statement has braces, they
all should.
765: This prototype seems to be out-of-sync with the code.
774: Please explain why a coder would want to use use_type.
791: cstyle.ms.pdf says continuation lines should never start with
a binary operator.
796-808,817-818: cstyle.ms.pdf says if any arm of an if statement has
braces, they all should.
861: I think this would be clearer as something like "For the property
group pattern represented by scf_pg_tmpl_t, return the property
group name for the property pattern for the property named prop."
Unless, of course, that's wrong.
968: SCF_ERROR_HANDLE_MISMATCH should be a valid error since inst
might not have been created from h. If we control all of the
callers, then it's ok to abort() on this, but please add a comment
explaining that we're enforcing correctness in the callers.
1018: cstyle.ms.pdf says if statements should be broken across two
lines.
1043: As at 968.
_walk_template_instances(): Please specify what the output of this
function is.
1223: I presume func() set this? Please add a comment before this
function, or before the definition of walk_template_inst_func_t, or
before the definition of pg_tmpl_walk_t, on what pw_pg means, or how
func() is expected to set pw_pg.
_get_pg(): Why does this function take an scf_propertygroup_t **? It
seems an scf_propertygroup_t * would work fine.
1537: cstyle.ms.pdf says if statements should be broken across two
lines.
1554: It seems this code relies on _lookup_pg() destroying pg in some
case. Please document that behavior in _lookup_pg()'s comment.
1559-60,79-80,1624-25,57-58,72-73: cstyle.ms.pdf says if any arm of an
if statement has braces, they all should.
1612-27: I suspect the readability of this code could be improved by
inverting some of these if statements. Namely,
name = _read_single_astring_from_pg(pg,
SCF_PROPERTY_TM_NAME);
if (name == NULL)
continue;
type = _read_single_astring_from_pg(pg,
SCF_PROPERTY_TM_TYPE);
if (type == NULL) {
uu_free(name);
continue;
}
...
1679,83: cstyle.ms.pdf says if statements should be broken across two
lines.
1772: I think I've always seen ARGSUSED before the type, not between
the type and the function name.
1797-8,1806-8,2013: cstyle.ms.pdf says if statements should be broken
across two lines.
1941-8: cstyle.ms.pdf says if any arm of an if statement has braces,
they all should.
2052: I think I've always seen ARGSUSED before the type, not between
the type and the function name.
2071-2,72-4: cstyle.ms.pdf says if statements should be broken across
two lines.
2124-5: If I'm reading this code correctly, we'll enter the same path
for when snapshot is "running" and SCF_PG_TMP_FLAG_CURRENT is set.
Am I mistaken in reading the scf_tmpl_get_by_pg(3SCF) manpage as
saying that the current flag should not use the "running" snapshot?
2130-2: cstyle.ms.pdf says if any arm of an if statement has braces,
they all should.
_scf_tmpl_pg(): Shouldn't the name of this function indicate that it
will retrieve the value of a property?
2427: I think it would be clearer to say something like "When t is
uninitialized or reset, sets t to the first property group template
in fmri. On subsequent calls, sets t to the next property group
template in fmri."
2529-32,2663-4,65-6: cstyle.ms.pdf says if any arm of an if statement
has braces, they all should.
2561,2654: cstyle.ms.pdf says continuation lines should never start
with a binary operator.
2672-5: cstyle.ms.pdf says if statements should be broken across two
lines.
2755: I think I've always seen ARGSUSED before the type, not between
the type and the function name.
2752: I don't understand why this function would fail because
a property has no value.
2798: Did you consider designating an error code for when the property
group exists but has the wrong type?
4505: Please add a comment explaining why a default case isn't
appropriate.
lib/libscf/inc/libscf.h
struct scf_values: Please add a comment which explains that there is
no explicit initializer for this structure. Instead users are
expected to provide the appropriate amount of memory, and functions
which set the structure assume that it is either uninitialized or
destroyed.
scf_tmpl_error_t: Are the structures of this member public? If not,
shouldn't they be in libscf_priv.h or scf_tmpl.c?
311-21: Do these need to be public? Can they be in libscf_priv.h?
SCF_ENCODE32_PAD: Is this public? Should it be in libscf_priv.h?
scf_decode32() & scf_encode32(): These aren't listed in the ARC case,
so I presume they should be in libscf_priv.h.
David
