David Bustos writes:
> Quoth Tom Whitten on Thu, Aug 28, 2008 at 10:29:27AM -0600:
> > > > The code review, parented to the onnv_95 snapshot is here:
> > > > http://cr.opensolaris.org/~lianep/webrev-20080815/
> ...
[SNIP]
> > > cmd/svc/svccfg/svccfg_libscf.c
> ...
> > > 5810: Why are you changing this? From what I can tell, imp_tsname is
> > > allocated to be max_scf_name_len + 1 bytes long, at 6754.
> >
> > Because of bug 6681151. This will prevent svc.startd from core dumping
> > if a service name of 115 characters long is imported. When the bug is
> > fixed we can revert this change.
>
> Add a comment. I think you should include the bug ID.
OK
[SNIP]
> > > cmd/svc/svccfg/svccfg_tmpl.c
> ...
> > > composed_pg_destroy(): If cpg_instance_pg->sc_pgroup_composed is
> > > destroyed by composed_pg_destroy(), why isn't it in composed_pg_t?
> > > Are there places were you have access to the instance pgroup_t but
> > > not the composed_pg_t?
> >
> > Yes in property_find() and next_property(). As I am working the other
> > issues in this review, I'll keep my eyes open for ways to restructure this.
>
> Ok. If you don't find a way, please add a comment.
will do.
>
> ...
> > > build_composed_property_groups(): This function is only called at the
> > > end of build_composed_instance(). Does it make sense as a separate
> > > function?
> >
> > We could do that. When I was writing the code, I thought of them as two
> > separate operations, so I wrote them as two separate functions.
>
> I don't think the split saves a lot of complexity. But it's not fatally
> confusing, either. If you decide to keep it, please add a comment
> before build_composed_property_groups() that it is a utility for
> build_composed_instance().
OK.
>
[SNIP]
With this, I think that we are in agreement on all points of your svccfg
code comments.
Antonello and I are working on coding all the fixes.
tom
