David, Please find my answers inline below. I tagged with -- Pending -- the questions that I think will be better addressed by Tom or Liane. Thanks for your review and comments.
Antonello David Bustos wrote: > 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." -- Pending -- > > 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. Moved to 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? -- Pending -- > > lib/libscf/common/scf_tmpl.c > *: I believe many of the functions in this file should be static. > Please check them. Done. > > 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. -- Pending -- > > 253: Isn't scf_type_to_string() documented to never return NULL? Yes, fixed. > > 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. Changed to accept NULL arguments. But... Calling scf_value_destroy() and scf_property_destroy() with NULL argument is NOT documented in the man pages. Since it is unlikely this will change AND it belongs to the same project, I'll assume it is safe to accept a NULL argument here. But I'll file a bug against the man pages to make sure surprises won't happen here. > > 316: Please break this assert()ion into two, so we can tell which > failed without the core. Done. > > 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. Thanks, you found a bug. I was returning NULL without setting scf_error(). I chose to assert(). > > 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(). Yep! refactoring to use a flag. > > 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. Comment added: "char **_append_astring_values() reads the values from the property prop_name in pg and append to an existing scf_values_t *vals. vals may be empty, but MUST exists." duplication-detection will be refactored. > > 569,603,659: Please break these assert()ions up. Done. > > 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. Commented: "the array may be bigger, but it is irrelevant since we will always re-allocate a new one" > > 611: cstyle.ms.pdf calls for a blank line after local declarations. Done. > > 664: cstyle.ms.pdf says if any arm of an if statement has braces, they > all should. Done. > > 765: This prototype seems to be out-of-sync with the code. Fixed > > 774: Please explain why a coder would want to use use_type. Added comment: "If use_type is set and pg is not NULL, a property group name for a property group template that has type defined is returned, even if no type is provided." > > 791: cstyle.ms.pdf says continuation lines should never start with > a binary operator. Done. > > 796-808,817-818: cstyle.ms.pdf says if any arm of an if statement has > braces, they all should. Done. > > 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. I agree the current wording is poor. I am re-wording as: "Returns the name of the property template prop in the property group template t. Returns NULL on failure and sets scf_error() to:" > > 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. -- Pending -- > > 1018: cstyle.ms.pdf says if statements should be broken across two > lines. Done. > > 1043: As at 968. -- Pending -- > > _walk_template_instances(): Please specify what the output of this > function is. -- Pending -- > > 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. -- Pending -- > > _get_pg(): Why does this function take an scf_propertygroup_t **? It > seems an scf_propertygroup_t * would work fine. You're right. Fixed > > 1537: cstyle.ms.pdf says if statements should be broken across two > lines. Done. > > 1554: It seems this code relies on _lookup_pg() destroying pg in some > case. Please document that behavior in _lookup_pg()'s comment. Added comment in _lookup_pg() "On error, destroy pg and set it to NULL." > > 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. Done. > > 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; > } > > ... Done. > > 1679,83: cstyle.ms.pdf says if statements should be broken across two > lines. Done. > > 1772: I think I've always seen ARGSUSED before the type, not between > the type and the function name. Since all arguments are used in this function, I removed ARGSUSED > > 1797-8,1806-8,2013: cstyle.ms.pdf says if statements should be broken > across two lines. Done. > > 1941-8: cstyle.ms.pdf says if any arm of an if statement has braces, > they all should. Done. > > 2052: I think I've always seen ARGSUSED before the type, not between > the type and the function name. Since all arguments are used in this function, I removed ARGSUSED > > 2071-2,72-4: cstyle.ms.pdf says if statements should be broken across > two lines. Done. > > 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? You may have been misled by the documentation. "current configuration" *is* the "running". I re-phrased to: "If flags includes SCF_PG_TMPL_FLAG_CURRENT, the snapshot argument is ignored and the "running" snapshot is used." > > 2130-2: cstyle.ms.pdf says if any arm of an if statement has braces, > they all should. Done. > > _scf_tmpl_pg(): Shouldn't the name of this function indicate that it > will retrieve the value of a property? I added: "Retrieves name or type of a template pg." to the function's comment block. and renamed the function to: _scf_tmpl_prop_value() > > 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." Re-phrased to function's block comment to: "Iterates through the property group templates for the fmri given. 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 frmi. Returns 0 on success, 1 when no property group templates are left to iterate, -1 on error. The flags argument may include SCF_PG_TMPL_FLAG_REQUIRED, SCF_PG_TMPL_FLAG_CURRENT, and/or SCF_PG_TMPL_FLAG_EXACT." > > 2529-32,2663-4,65-6: cstyle.ms.pdf says if any arm of an if statement > has braces, they all should. Done. > > 2561,2654: cstyle.ms.pdf says continuation lines should never start > with a binary operator. I actually chose to improve readability by breaking down the assignment and test: pg_tmpl->pt_iter = _get_next_iterator(h, pg_tmpl, snapshot, (flags & SCF_PG_TMPL_FLAG_EXACT) ? 1 : 0); if (pg_tmpl->pt_iter == NULL) { if (scf_error() == SCF_ERROR_NOT_FOUND) return (1); else return (-1); } > > 2672-5: cstyle.ms.pdf says if statements should be broken across two > lines. Done. > > 2755: I think I've always seen ARGSUSED before the type, not between > the type and the function name. Moved and placed before the type. However, I think we should either remove the flag or give it a use and document it. > > 2752: I don't understand why this function would fail because > a property has no value. Comment fixed to match man page reason for SCF_ERROR_NOT_FOUND > > 2798: Did you consider designating an error code for when the property > group exists but has the wrong type? -- Pending -- > > 4505: Please add a comment explaining why a default case isn't > appropriate. Added a case for SCF_TYPE_TIME that was missing and included a default case to abort. > > 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. Placed the comment: "/* * There is no explicit initializer for this structure. * Consumers should provide the appropriate amount of memory, and * functions which set and populate the structure. * scf_values_destroy(3SCF) will free the structure members using * uu_free(3uutil). Any new function that populates this structure has * to allocate memory with uu_zalloc(3uutil) or provide its own * destructor */" > > 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? Moved structure definition to libscf_priv.h, left typedef in libscf.h > > 311-21: Do these need to be public? Can they be in libscf_priv.h? Moved to libscf_priv.h > > SCF_ENCODE32_PAD: Is this public? Should it be in libscf_priv.h? Moved to 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. Moved to libscf_priv.h
