David, Thanks for your comments. I'll work my way through and send you my responses and a new webrev as soon as possible.
tom David Bustos writes: > Quoth Liane Praza on Fri, Aug 15, 2008 at 03:41:37PM -0700: > > Tom, Antonello, and I would like to invite reviews of the code for > > extending the existing SMF templates to incorporate general service > > property metadata. > > > > We previously published and discussed ARC materials on this alias, so > > the interfaces and goals of the project shouldn't be much of a surprise. > > > > I'm re-posting our manpage diffs and interface information here for > > reference, though it is not under review: > > http://cr.opensolaris.org/~lianep/templates-0815/ > > While reading about the interfaces, I came across some text which > I think many users will find unclear. And during the review, I had some > questions. Since this documentation is not under review, however, feel > free to ignore. > > smf_template(5) > "Templates may be defined for a service to describe metadata about > the service in general...": I think this would be more useful to > users if it were more concrete. Specifically by describing when > users might want to produce or consume templates. Perhaps saying > that service developers use templates to describe what > configuration values are valid, administrators can use templates > to validate configuration, and tool developers can use templates > to provide more helpful configuration user interfaces. > > "svcs -lv...": Perhaps you should specify that the output of svcs > -lv and svccfg describe are human-readable, and for programmatic > or stable access the libscf interfaces should be used. > > "A template is created...": This paragraph confuses me because the > first sentence says what creates a template, but the rest > describes, I think, how service developers should write templates > in their manifests. Though maybe it's trying to describe which > templates should be used for which properties. I suppose it turns > on the definition of "templated". > > "Templates defined globally...": I think you should clarify when > a template is considered to re-define another template. Do they > have individual names? Or is it based on their matching criteria? > It seems this information is most relevent to template authors, > except for when template consumers need to understand that > configuration validation can fail due to miscoordination between > different template authors. So I'm not sure that the "Template > Composition" section is the best place for this. > > "Future work...": Is future work appropriate for a manpage? > > "Capital letters...": I suspect this should be qualified with "in > English locales". > > "If name or type is omitted, it acts like a wildcard.": I think you > mean that the template acts as though it has a wildcard for that > field. Though saying that the template can act in such a way > might be considered bad diction. > > "This property lets us declare which.": First person doesn't seem > appropriate for a manpage. > > "internal_separators tells us...": Can this be deprecated now that > SMF Property Value Ordering has been integrated? > > svccfg(1M): > "... if invoked with the -s option...": -s is missing from the > synopsis. > > scf_tmpl_pg_name(3SCF): > "this funtion will return a string containing SCF_TMPL_WILDCARD": > SCF_TMPL_WILDCARD doesn't seem to be in the interface table of the > ARC case. Is it appropriate to mention it here? > > "The caller is responsible for freeing the *out buffer on success.": > I presume you mean the caller should use free(3C). Though in the > past, the library was required to provide its own freeing function > in case malloc() or free() were interposed, or the function used > an allocator in a library other than libc. Is that no longer > necessary due to direct bindings? > > "The scf_tmpl_pg_target() function will retrieve the property > group's target as currently templated...": I don't understand what > you mean by a property group's target. Do you mean the template's > target? > > scf_tmpl_prop_name(3SCF): > scf_tmpl_prop_internal_seps(): I don't understand how this function > will fill the scf_values_t. Will there always be a single element > for the entire string of separators, or will there be a separate > element for each character of the separators string? > > scf_tmpl_validate_fmri(3SCF): > "The template validation functions offer a way to validate an > FMRI...": Really you're validating the configuration data associated > with an instance, eh? > > "By default, the validation functions look up composed data...": Is > this referring to the data to be validated, or the data to > validate against? > > > The code review, parented to the onnv_95 snapshot is here: > > http://cr.opensolaris.org/~lianep/webrev-20080815/ > > So far I've reviewed svccfg's code. > > cmd/svc/svccfg/Makefile: ok > > cmd/svc/svccfg/svccfg.h > 184: Please add a comment explaining that sc_pgroup_composed is > created & destroyed by compose_props() & composed_pg_destroy(). > > cmd/svc/svccfg/svccfg.l: ok > > cmd/svc/svccfg/svccfg.y > 133: This doesn't implement the -s option documented in > svccfg_1m.diff. Which is correct? > > 143: I'm not sure this should be flagged as a syntax error. It seems > more like a semantic error to me (and therefore should use > semerr()), since the syntax of the command is correct, but the > command is wrong due to the context. > > 420: cstyle calls for NULL instead of 0 for pointers. > > cmd/svc/svccfg/svccfg_engine.c > 584: lxml_get_bundle_file() seems to call semerr() before returning > any errors. So it seems that this error is superfluous. Am > I missing something? > > engine_import(): svccfg_1m.diff says -V is the default in interactive > mode, but I don't see where that is implemented. > > 599: This error isn't very informative. Are there cases where > tmpl_errors_print() doesn't print an informative error message? > > 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? > > cmd/svc/svccfg/svccfg_help.c > 33: Shouldn't this be updated? > > 102-3: This is the only place "settings" is used. Can we use > "properties" instead? > > cmd/svc/svccfg/svccfg_internal.c > internal_pgroup_create_unique(): I think > internal_pgroup_create_strict() would be a more intuitive name for > this function, though I suspect internal_pgroup_create() with > a comment would be sufficient. > > load_instance(): > - Please note in the comment that the property and property group > patterns of the composed instance are loaded also. > - Shouldn't inst be scf_instance_destroy()ed before returning? > - I think you should return ENOENT for a lot of the > SCF_ERROR_DELETED failures, since the caller shouldn't care > whether the instance didn't exist in the beginning, or was deleted > halfway through. > > 1186: I think you should note that fmri will be referenced by > *inst_ptr (rather than being copied) in the function comment. > > 1205: This should be "scf_pg_get_type", since that's the function that > returned the bad error. And the following break is superfluous > since bad_error() never returns. > > 1222: This should be bad_error("load_pg", rc). And the following > break is superfluous since bad_error() never returns. > > cmd/svc/svccfg/svccfg_libscf.c > 66: I think it would help future modifiers of this code to add > a comment explaining when these macros are used. > > 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. > > lscf_validate_fmri(): Please add a comment explaining what this > function is supposed to do. > > 9428: Shouldn't inst_sz be of type size_t? > > 9439: Is there a reason this isn't semerr(gettext("..."))? > > 9444,59: I don't understand why these messages end with colons and > newlines before exiting. But they doesn't seem helpful anyway. > I think it would be more helpful to inform the user that either > there is a defect in libscf, or memory was corrupted. > > 9445: This should be superfluous since uu_die() should never return. > > 9448-54: It doesn't seem wise to me to use lscf_select() in this way. > I suppose because lscf_select() is an entry point, so someone who > modifies it should be entitled to the assumption that it is not > being called from another entry point. I think it would be better > to factor lscf_select() into an entry point and an > argument-accepting utility, which is invoked here. I suppose it > doesn't need to be changed before you integrate, but at the very > least, a comment should be added before lscf_select() saying that > it's being used in this way. > > 9460: This should be superfluous since uu_die() should never return. > > 9478: By my reading of > > http://cr.opensolaris.org/~lianep/templates-0815/scf_tmpl_validate_fmri.3SCF.new > , > scf_tmpl_validate_fmri() doesn't accept SCF_PG_TMPL_FLAG_CURRENT. > Do you mean SCF_TMPL_VALIDATE_FLAG_CURRENT? > > 9478: Since the only statement in this if statement tests ret, this > test seems redundant. Couldn't we improve readability by > eliminating it? > > ret = scf_tmpl_validate_fmri(...); > if (ret == -1) { > ... > } else if (ret == 1 && errs != NULL) { > ... > } > > if (errs != NULL) > ... > > 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. > > lscf_validate_file(): Shouldn't this be static? > > 9539: I believe this (char *) cast could be eliminated by making str > const char *. > > list_pg_tmpl(): I think it would help future modifiers of this code to > add a comment which explains what list_pg_tmpl() should do, and > particularly what how the templates argument affects behavior. > > 10894: scf_value_get_astring() returns the length of the string, > rather than SCF_SUCCESS, on success. I think you mean "== -1". > > 10921,30,11162: Won't we print this if scf_tmpl_pg_name() fails? Why > don't you print an error? Do you mean to detect the wildcard? If > so, the ARC materials say you should test for "*". > > 10935,11180: Shouldn't "true" & "false" be in gettext()? They are in > svccfg_tmpl.c. > > 10995: I don't understand why you'd use quote_and_print() if the > string contains a space, tab, newline, or parentheses, since > quote_and_print() doesn't quote any of those characters. > > 11018,64: Is there a reason not to use for (i = 0; > i < values.value_count; ++i) here? > > list_prop_tmpl(): Please add a comment explaining how prop and > templates arguments affect the behavior of this function. > > 11198: What's the difference between this line and the other side of > the if? > > get_template_pg(): This function doesn't seem to be template-specific. > Can you change the name? And move it to an area for utility > functions, either the top or the bottom of the file. > > 11252: Why die on SCF_ERROR_DELETED? > > 11270: This break should be superfluous because uu_die() shouldn't > return. > > 11294: Why do we continue if the repository connection was broken? > > 11304: Should there be a new macro for indentations twice as long as > TMPL_INDENT? > > 11342: cstyle says labels must be on their own lines. > > 11368,97: Why are these hardcoded for the "C" locale? > > 11370: Why die if pg was deleted? Why not act as though it never > existed? > > 11383,410: Why goto done if scf_value_get_ustring() fails with > SCF_ERROR_NOT_FOUND? It's not documented to do so, so I think you > should inform the user that libscf was defective, or memory was > corrupted, and maybe abort. > > 11428: Why die if the current selection was deleted? Why not issue an > error and return? > > 11440: val & iter are never destroyed. > > 11444: Is this legal? I've only seen ARGSUSED before the function. > > 11493: Won't prop_name always be NULL here? Why not just use NULL? > > 13662: Can you add a comment explaining that we have to set optind to > 0 because the arguments start at argv[0] rather than argv[1]? > > 13685: This line seems superfluous since argc isn't used again. > > cmd/svc/svccfg/svccfg_main.c: ok > > cmd/svc/svccfg/svccfg_tmpl.c > 33: Could you mention the entry points and main data structures of > this file? And maybe the main sections of code in the file? E.g., > the im_tmpl_error_print() code could be called out as a significant > block. > > 75-294: There are a lot of types here. Can you add a comment which > gives an overview of which types are used in what parts of the code? > This could go into the file comment. Or maybe put in a few comments > that divide the types into sections. > > 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? > > 431: Shouldn't you pass pg or cpg as the parent? > > 511: Why not pass inst as the parent here? > > build_composed_property_groups(): This function is only called at the > end of build_composed_instance(). Does it make sense as a separate > function? > > 620: Please add a comment explaining that we don't need to free the > strings because they should point to buffers owned by property_t's. > > 626: "inialize and" should probably be "initialize an". > > 778: Isn't this an error that we should catch with an assertion? > > 809: Couldn't v be NULL? > > find_count_value(),find_tmpl_property(): I didn't think properties > can have values of different types. In that case, it should suffice > to just use the first value on the list. > > find_tmpl_property(): Doesn't this do the same thing as > find_astring_value_in_pg()? > > find_restarter(): Why doesn't this function use sc_composed? > > get_ranges(): If count is a count of elements, it should be an > unsigned integer rather than a size_t. > > 1005: This won't detect garbage after the number. For that you should > test that *endptr2 == '\0'. > > scf_terrors_create(), scf_terrors_destroy(): Would you mind renaming > these functions so they don't begin with scf_? Otherwise it's easy > to think they're defined in libscf rather than svccfg. > > 1041: Shouldn't this say "Given a property group and ..."? > > 1043: I find "template prop_pattern information" difficult to > understand. Do you mean just "template information"? > > 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? > > im_tmpl_error_create(): This function is only called by > tmpl_errors_add_im(). Is it worth being on its own? > > im_tmpl_error_destroy(): This function is only called by > tmpl_errors_destroy(). Is it worth being on its own? > > 1140: Would you mind describing the overall architecture here? > Specifically that im_tmpl_error_t's are printed by > im_tmpl_error_print(), which dispatches out to the im_perror_*() > functions, which use im_perror_item() as a utility. And maybe the > general form of how the output will look. > > 1215: Are you sure the strings won't have embedded quotes? If so, > please add a comment. > > 1390: I believe you're missing a space between "attribute" & "is". > > LLDIGITS: Isn't sizeof (uint64_t) always 8? Do you mean > sizeof (long long)? Can you add to the comment that this is based > on the idea that 10 digits are required to represent 32 bits? > Though if we're going to print numbers that large, I suspect we > should use thousands separators. For the users. > > 1464: The lltostr(3C) manpage I read says the function is undefined > for negative values. Don't you have to pass the absolute value, and > prepend a negative yourself? Maybe it would be easier to use > snprintf(). > > add_scf_error(): This function is long enough to justify a block > comment explaining what it should do, I think. > > 1548: Please break this assertion up into the two sub-conditions: > > assert(einfo != NULL); > assert(einfo->ei_type == EIT_CARDINALITY); > > This allows us to tell which condition failed from just the error > message. > > 1682: All values of a property should have the same type, so searching > for one of type boolean is unnecessary. You should just use the > first value on the list. > > load_general_templates(): Restarters are attributes of instances, not > services. I suspect this means you need to move the sc_restarter > field of entity_t from sc_service to sc_instance. > > 1727: You should not give up on the restarter because the the > service instance in question is a restarter -- restarters can be > delegated to restarters, too. > > 1832: Why not use composition here? > > 1980: Since this iterator is intended to iterate over multiple > instances, this comment should indicate such. > > pg_iter_next(): I think the name of this function is too generic. > I recommend something like next_possible_pattern_pg() or similar. > > 2132: By scope, do you mean the entity which specifies the pattern? > If so, I suspect 'origin' or the like would be easier for future > coders to understand. Particularly since "scope" doesn't appear in > the proposed smf_template(5). > > 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. > > pg_target_check(): Would this function make sense to call if > pg_pattern was not just returned from iter? I suspect that it would > be a cleaner interface to accept the tmpl_scope_t instead of the > pg_iter_t, to reduce the assumptions the function makes about the > caller. > > 2362: Can't you just use strlen(pfx->pp_prefix) here? It seems that > would obviate prop_prefix_t. > > tmpl_scan_general(): This seems a lot like gather_pattern(). Could > they be combined someday? > > 2623: "server" -> "service". > > 2770: What should a user do if he sees this message? > > 2883: I presume you do this even if pg_patterns[i] isn't NULL because > you consider it an error for manifests to redefine patterns. If so, > please add a comment explaining this. > > tmpl_required_pg_present(): Shouldn't this be static? > > cmd/svc/svccfg/svccfg_xml.c > 1321: Is this enough information for the user to know how to fix the > problem? > > lxml_get_common_name(): This function is only used in > lxml_get_tm_common_name(). Does it make sense to have both? > > 1456: Too much indentation. > > lxml_get_description(): This function is only used in > lxml_get_tm_description(). Does it make sense to have both? > > 1512,1555: It is not appropriate to die here because DTD validation > won't catch these problems. > > 1706: Would DTD validation catch this? If not, it's not appropiate to > die here. > > 1825: I don't think it's good style to break an argument name from its > type. > > 1913: It's not appropriate to die here because DTD won't catch this > problem. > > 1921: "two elements" -> "two attributes" > > 1925: I suspect this should be "name of prop_name". > > 1955: Why is this max_scf_name_len and not max_scf_value_len + 1? > > 1958: I think it would be better to assert that max_scf_name_len is > larger than the bytes required to represent two 64-bit numbers. But > if you insist, at least change "to long" to "too long". > > 2043,50,58: Is it possible for us to tell the user how many bytes too > long the name is? > > 2289: It's not appropriate to die if prop_pattern_name is empty > because DTD validation won't catch it. > > 2295: It's not appropriate to die here because DTD validation won't > catch the problem. > > 2302: Can we tell the user how many bytes too long the name is? > > 2313: How can this happen? If the user includes a property_group by > that name? If the user included duplicate prop_pattern's? > > 2438: This can only happen if svccfg and the DTD are out-of-sync, > right? In that case, I think you should die instead of warn. > > 2444,49: How would these happen? > > 2502: I think it would be more accurate to say that patterns with > target "all" are only allowed on the global service. > > 2550: Can we tell the user how many bytes too long the type is? > > 2559: It's not appropriate to die here because DTD validation won't > catch this problem. > > 2575: Can we tell the user how many bytes too long the name is? > > 2589-605: It's not appropriate to die here because DTD validation > won't catch this problem. > > 2618: It's not appropriate to die here because DTD validation won't > catch this problem. Plus, verify_pg_pattern_attributes() already > printed a more precise error message, so this one isn't useful. > > 3016: It's not appropriate to die if sc_bundle_name is empty because > DTD validation won't catch that problem. > > lib/libscf/inc/libscf.h > scf_values_t: These member names don't match the ARC materials. > > scf_tmpl_visibility_to_string(): The type of this argument doesn't > match the ARC materials. > > 11214: Shouldn't this %d be %lld? > > lib/libscf/inc/libscf_priv.h > _read_tmpl_prop_type_as_string(), _read_single_astring_from_pg(): > Please include "scf" in the names of these functions so it's obvious > that they're defined in libscf. > > > I'll continue reviewing the rest in the new webrev. > > > David > _______________________________________________ > smf-discuss mailing list > smf-discuss at opensolaris.org
