Quoth Antonello Cruz on Wed, Sep 03, 2008 at 11:40:16AM -0700:
> 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/
...
> > lib/libscf/common/scf_tmpl.c
...
> > 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.
Ok, but please include a comment explaining why that's preferable to
making all of the callers use the same freeing function.
...
> > 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."
So specifying SCF_PG_TMPL_FLAG_CURRENT is equivalent to passing NULL for
the snapshot?
...
> > 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);
> }
Yes, that's better.
...
> > 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
> */"
Hmm, when I read "explicit initializer" again, I expected a static
initializer. Probably better to specify "initializing fuction" or
"constructor". I suspect you're missing a subject in the second
sentence, because I don't think consumers are supposed to provide
functions which set and populate the structure. Perhaps my suggestion
wasn't clear in describing the functions which set the structure.
David
