(Handling the 'pending's from Antonello's previous response.) Antonello Cruz wrote: > 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.
>> lib/libscf/common/lowlevel.c >> scf_encode32(), scf_decode32(): Is there a reason for the placement >> between _scf_request_backup() and _scf_repository_switch()? This was missed in the previous responses. We'll move it. >> 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 -- And the check after 1129 is bogus anyways. Changing to asserts, as these are "shouldn't happen" cases. > >> >> lib/libscf/common/scf_tmpl.c > >> 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 -- Will do. >> 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 -- No need to take h as an argument. Removed that, and now the assert is valid. >> 1043: As at 968. > -- Pending -- Comment added. >> _walk_template_instances(): Please specify what the output of this >> function is. > -- Pending -- Comment added. >> 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 -- Comment above walk_template_instances() describes. >> 2798: Did you consider designating an error code for when the property >> group exists but has the wrong type? > -- Pending -- I'm not sure that callers would handle it differently than a simple not found. liane
