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/ ... > > cmd/svc/svccfg/svccfg_engine.c ... > > 599: This error isn't very informative. Are there cases where > > tmpl_errors_print() doesn't print an informative error message? > > No. tmpl_errors_print() gives pretty detailed information.
I suppose it doesn't know that the errors are related to the import operation, though. And it doesn't call semerr(). I suppose you should keep it. > > 608: I believe lscf_bundle_import() prints an informative error > > message before returning an error code. So it seems that this error > > is superfluous. Am I missing something? > > lscf_bundle_import() does give useful messages. This removes the ambiguity > as to whether or not the import succeeded and set the appropriate svccfg > exit code. The project team set a goal of always announcing when the > import failed. Ok. ... > > 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. ... > > 9486: len should be const. And shouldn't it be size_t? And there > > should probably be a comment explaining its value, even if it's > > arbitrary. > > len cannot be const because we may need to increase the size of the buffer > in line 9496, but I agree with the rest of your statement. Right. ... > > 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. ... > > 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(). ... > > find_restarter(): Why doesn't this function use sc_composed? > > This is called before build_composed_instance(). I'll check to see if I > can reorder the calling sequence. Ok. If you can't, please add a comment. ... > > 1067: I believe you should allocate a buffer of limit + 1 bytes, not > > limit. > > > > 1073: Couldn't the string be too long if prop_name is too long, which > > is more of a problem with the manifest rather than the template? Or > > is it impossible to tell which is too long? > > It is the total of the pg_pattern name and the prop_pattern name that is > too long. It hard to cast blame on one or the other. I suppose. ... > > 2133: Would it be counterintuitive to reverse the sense of this table? > > That is, I suspect it would be clearer to list which scopes should > > be applied for which targets, though it's possible that I just don't > > understand how the function will be used very well yet. > > Perhaps. For me it was easier to think of which cases were special, > i.e. those cases where the template did not apply. Either way, I think > that the comment should be considerably enhanced. The comment should give > a better explanation for the purpose of the function and explain why each > entry is in the table. While I'm doing this, I'll look at reversing the > table. Ok, thanks. David
