David, Thanks again for your comments. I've responded to most of the comments inline below. You pose a few questions that I cannot answer. These relate to code that Liane wrote, and I'd prefer to let her answer them when she returns. I've marked these questions with "-- Liane --" so that we can easily search for them.
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. > Antonello has already responded to the documentation comments. [SNIP] > > 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(). OK > > 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? svccfg_1m.diff was referring to the [-s FMRI] in: /usr/sbin/svccfg [-v] [-s FMRI] subcommand [args]... svccfg_1m.diff has been updated and the reference to -s FMRI was removed. > > 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. We'll fix these. > > 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? You are correct. I'll remove this line. > > engine_import(): svccfg_1m.diff says -V is the default in interactive > mode, but I don't see where that is implemented. We had an internal bug tracking this, and it has since been fixed. It will show up in the next webrev. > > 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. > > 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. > > 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? yes for both of the above. > > 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. will fix the above. > > 1186: I think you should note that fmri will be referenced by > *inst_ptr (rather than being copied) in the function comment. In looking at svccfg_internal.c I see that the other places where sc_fmri is used that it is set to allocated memory. We should probably be consistent here, and copy the fmri. We'll also fix internal_service_free() and internal_instance_free() to free the allocated memory. > > 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. will fix the above. > > 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. > will do > > 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. > > 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) > ... > will fix all the above > 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. > > 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". > will fix the above > 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 "*". You're correct. The code does not correctly check for an error return, and it does not correctly check for the wild card. We'll correct these. > > 10935,11180: Shouldn't "true" & "false" be in gettext()? They are in > svccfg_tmpl.c. No. "true" and "false" are the only legal values for the required attribute. svccfg_tmpl.c is wrong, so we'll fix it. > > 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. -- Liane -- > > 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. > will fix the above > 11198: What's the difference between this line and the other side of > the if? -- Liane -- > > 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. > will fix the above > 11252: Why die on SCF_ERROR_DELETED? -- Liane -- > > 11270: This break should be superfluous because uu_die() shouldn't > return. > will fix the above > 11294: Why do we continue if the repository connection was broken? -- Liane -- > > 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. will fix the above > > 11368,97: Why are these hardcoded for the "C" locale? -- Liane -- > > 11370: Why die if pg was deleted? Why not act as though it never > existed? -- Liane -- > > 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. -- Liane -- > > 11428: Why die if the current selection was deleted? Why not issue an > error and return? -- Liane -- > > 11440: val & iter are never destroyed. will fix the above > > 11444: Is this legal? I've only seen ARGSUSED before the function. Will remove it. Both arguments are used in 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. will fix the above > > 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. Sure. > > 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. > Will do. > 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. > > 431: Shouldn't you pass pg or cpg as the parent? > > 511: Why not pass inst as the parent here? > will fix the above > 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. > > 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. > will fix the above > find_tmpl_property(): Doesn't this do the same thing as > find_astring_value_in_pg()? Yes. Will get rid of one of them. > > 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. > > 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 ..."? > will fix the above > 1043: I find "template prop_pattern information" difficult to > understand. Do you mean just "template information"? Each prop_pattern is represented by a separate property group in the repository. I'll work on a clearer way to express this in the 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. > > 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". will fix the above > > 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(). I like the idea of snprintf. I can get it to tell me how many digits it is going to print, so I don't need LLDIGITS either. > > 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. > will fix the above > 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. You're right. We had a project bug on this which was fixed after we sent out the webrev. You should see the change in the next webrev that we send out. > > 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. OK. > > 1832: Why not use composition here? good idea. > > 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). will fix the above > > 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. > > 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. will fix the above > > tmpl_scan_general(): This seems a lot like gather_pattern(). Could > they be combined someday? There are some similarities. I'll create a project bug to see if they can either be combined or perhaps extract some common code into a single function. > > 2623: "server" -> "service". ok > > 2770: What should a user do if he sees this message? I'll remove the message. It's unlikely that the error message will ever be larger than 4096 characters. > > 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? will fix the above > > cmd/svc/svccfg/svccfg_xml.c > 1321: Is this enough information for the user to know how to fix the > problem? No. I'll improve the message. > > lxml_get_common_name(): This function is only used in > lxml_get_tm_common_name(). Does it make sense to have both? No. When I originally wrote the function I was anticipating more uses for it. I'll combine the function. > > 1456: Too much indentation. OK > > lxml_get_description(): This function is only used in > lxml_get_tm_description(). Does it make sense to have both? I'll combine them. > > 1512,1555: It is not appropriate to die here because DTD validation > won't catch these problems. will call semerr() instead > > 1706: Would DTD validation catch this? If not, it's not appropiate to > die here. will call semerr() instead > > 1825: I don't think it's good style to break an argument name from its > type. You're absolutely right. I'll fix that. > > 1913: It's not appropriate to die here because DTD won't catch this > problem. will call semerr() instead > > 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? will fix the above > > 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. will call semerr() for both the above > > 2302: Can we tell the user how many bytes too long the name is? yes > > 2313: How can this happen? If the user includes a property_group by > that name? If the user included duplicate prop_pattern's? You are correct on both counts, although I think that the error message can be improved in this case. > > 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. Correct. I'll change it to uu_die(). > > 2444,49: How would these happen? They can't. I'll assert() here. > > 2502: I think it would be more accurate to say that patterns with > target "all" are only allowed on the global service. agreed > > 2550: Can we tell the user how many bytes too long the type is? will do > > 2559: It's not appropriate to die here because DTD validation won't > catch this problem. will call semerr() instead > > 2575: Can we tell the user how many bytes too long the name is? will do > > 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. will fix the above > > 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. Antonello has corrected the documentation. > > usr/src/cmd/svc/svccfg/svccfg_libscf.c > 11214: Shouldn't this %d be %lld? Good catch. Same is true for 11207. To be precise they should probably be %llu, since both min and max are uint64_t. > > 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. ok > > > I'll continue reviewing the rest in the new webrev. > > > David > _______________________________________________ > smf-discuss mailing list > smf-discuss at opensolaris.org
