Most of these comments were simply addressed. I've omitted those from this mail, and included only comments which were not addressed or required further followup. Please let me know if you have issues with any of these and we'll try to reach resolution.
David Bustos wrote: > Quoth Liane Praza on Fri, Aug 22, 2008 at 12:34:53PM -0700: >> http://cr.opensolaris.org/~lianep/webrev-20080822/ > > lib/libscf/common/scf_tmpl.c > 2888: Shouldn't this fail if flags contains bits other than > SCF_PROP_TMPL_FLAG_REQUIRED? Traditionally, we haven't done this type of checking. > 3479: Why isn't this SCF_ERROR_TEMPLATE_INVALID? Because it's defined not to be. Seemed like the most flexible definition and is carried throughout its companion functions in the manpage and the function comments. > 3797,3817: If the string representation of {val_min,val_max} be > negative, wouldn't it have to be of some type other than count, in > which case scf_value_get_count() would fail? It isn't documented to... > 3972: It's not clear to me why it's better to return > SCF_ERROR_NOT_FOUND in this case than SCF_ERROR_TEMPLATE_INVALID, or > just let the caller decide that zero values is bad. Is there some > reasoning for this? Because, generally, for a property with no values there's no useful result to be returned. The value requested wasn't found. I'm not sure why telling the caller the template is invalid is more valuable here, and I hate having to have each caller include the logic to determine that there are zero values. This seemed like the most friendly and error-resistant calling convention. (I could generally question our choice to make properties with no values a valid repository state, but am not quite willing to go that far with this change. I control the semantics here, so can say that's neither a state I wish to error on nor return to the consumer.) > 4162,68,72: Is there a reason to return SCF_ERROR_CONSTRAINT_VIOLATED > instead of SCF_ERROR_TEMPLATE_INVALID? Because the template isn't necessarily invalid. > 4812: Will this work with xgettext(1)? Or is that not necessary? Why wouldn't it? I guess I'm not clear on what you're asking. > 6202: It seems that all of the callers of _add_error() know the type, > so why don't they just call the appropriate _add_tmpl_*() function > directly? Keeps all the logic in one place rather than spread all over, I guess. If you feel the other paradigm would be tangibly better, we can change it. > 7297: Shouldn't you fail if flags contains invalid flags? Again, this isn't typical checking. > lib/libscf/inc/libscf_priv.h: ok > > cmd/svc/svcs/svcs.c > 2175: Will we print property groups which only contain hidden > properties? Yes, as I'm sure you've already noticed, we will currently print the name and type of the property group. I suppose I could delay the printing if you think it's important not to. > cmd/svc/milestone/global.xml > 30: I wonder if we should omit the "Make customizations..." directive > here. Why? With Profiles, that will be exactly the way customizations are possible. I'm also not sure why we would change this one and not all of the manifests which use this block comment. > 78: I think this file would be easier to interpret if you included > a comment here saying who creates general property groups and why, > and what components read general property groups, since this is > essentially a codification of their reading rules. (svc.startd and > librestart in this case.) Similarly for 111, 249, 325, and 492. > Though I suppose for 249 and 325 you can just say that the templates > describe a repository representation which is not stable / not > public. I've chosen to augment the public descriptions rather than add comments seen only by developers and the morbidly curious. This is true for all of your comments of this form, so I won't repeat those comments and my response. > 89: What would it take to wrap this line in 80 columns? I haven't come up with a satisfactory way yet. :/ > cmd/svc/milestone/make-console-login-xml > 233: I think this should be qualified with a time. I don't really understand. I've rephrased using some of the manpage wording to try to make things clearer, but there's still no statement about the time. > cmd/svc/milestone/restarter.xml > 80: Isn't restarter created by librestart? And aren't restarters > required to use librestart? If so, it seems to me that it should be > in global.xml. In any case, I think you should add a comment > describing what creates restarter property groups and what readers > these rules correspond to. Not all restarters use only these properties, or are required to use all of them. The restarter property group is strictly owned by the restarter, even though it is required to set a very small set of the properties. > 446: I take it that the template validation code doesn't know how to > validate values separated by the declared internal separators? > Should "signal,core" also be included as a valid choice? It isn't in startd right now. (Which should be fixed -- we should fix it to actually accept a value list, but... that's a separate bug that I'm not addressing now.) > - Did you consider separate patterns for the start, stop, and > refresh methods? Yes, but I didn't feel like there was enough benefit. > cmd/cmd-inet/usr.lib/inetd/inetd.xml > 60: Is this change necessary? No, but it makes the formatting consistent with the rest of the file. > > 216,259,343,368,411,440,486,495: These should be counts, shouldn't > they? Is there a bug filed for that? Whether or not it was a mistake originally (it was), I don't think breaking all inetd manifests is worth it to change this now or in the future. I'm declining to file a bug. > 494: How many RPC versions are there? Depends on the service. Not sure what you're trying to get at. > 916: Why no pattern for contracts? Because they're not stored in the repository anymore. liane
