I'd like to try to resolve the remainders of your comments here. If this doesn't answer satisfactorily, let's talk in person Wednesday and close on these issues.
David Bustos wrote: > Quoth Liane Praza on Thu, Oct 09, 2008 at 08:49:01PM -0700: >>>>>> http://cr.opensolaris.org/~lianep/webrev-20080822/ >>>>> lib/libscf/common/scf_tmpl.c > ... >>>>> 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. >>> Telling the caller the template is invalid could be valuable if there's >>> no supported way to end up with that property having no values. Is that >>> possible? Or is it legitimate to specify that no values are legal for >>> a property? Or are you anticipating a future change wherein it may be >>> possible to do that in a supported way? >> No specific legitimate cases, but given that there's no actual damage or >> bad information here, I don't think there's value in telling the caller >> the template is invalid. It's a fetch-a-rock game for the user. >> >> I continue to disagree philosophically on this one. Are you insisting? > > I don't think I have enough information to know whether I should insist > or not. I believe it comes down to whether it's possible for > a developer to supply a manifest which specifies that there are no valid > values for a property, and if so, whether it's legitimate to do so. > > If it's not possible, then we can interpret the value-less property as > either a template which has undergone an unsupported modification, and > is therefore invalid, or we can interpret it the same as if no > constraints were specified, which affords some leniency for direct > modification of the properties or future changes to the template import > implementation. > > If it is possible and legitimate, then the caller needs to be able to > distinguish between the the absence of the property and the existance of > the property with no values, so we either need to return different error > codes, or return success and allow the client to test value_count. > > If it's possible, but we can't conceive of a case where a developer > would want to do this, then it's a judgement call between the above > options. It is not possible today. I've articulated why I made the judgement call I did. >>>>> 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. >>> Perhaps I'm missing something. Doesn't this condition mean that the >>> property contains an invalid number? Is there a supported way to end up >>> in that state? >>> >>> Also, how do you forsee a client behaving differently between >>> _CONSTRAINT_VIOLATED and _TEMPLATE_INVALID? >> The general point is that CONSTRAINT_VIOLATED means you probably called >> the wrong function for this min,max pair. TEMPLATE_INVALID means the >> template is wrong no matter which one you call. > > Ah, yes, the developer could be calling a count function on an int > property. If min > max, though, doesn't that necessarily mean the > template is invalid? I.e., that the developer delivering the manifest > made an error, rather than the developer calling the function? And for > the int functions, if the value doesn't fit in an int64_t, doesn't that > also mean that the developer delivering the manifest made an error, > rather than the developer calling the function? But in the count representation, -1 > 1, which isn't true in the int representation. Template isn't invalid. >>>>> 89: What would it take to wrap this line in 80 columns? >>>> I haven't come up with a satisfactory way yet. :/ >>> Well is it legitimate to have newlines in prop_pattern/description/ >>> loctext's? >> Yes, but then the newline is then encoded in the string. >> >>> If not, you could just break the line and have svccfg join >>> them together. Otherwise, couldn't you introduce a way to escape >>> newlines? It's ok if these are too distasteful -- I just want to >>> understand the alternatives. >> This is an existing problem that I'd prefer not to tack onto this >> project, but if you insist, we could explicitly choose to strip newlines >> from loctexts on import, though I worry about the reduction in later >> flexibility to fix essentially code formatting. > > Ok. To be clear, you're saying that it's not legitimate to have > newlines in the strings now, but you suspect that it may be in the > future? Yes. (There may be potential uses today, but I can't articulate them at this point and we aren't taking advantage of them.) > >>>>> 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. >>> Are you saying that it's ok for restarters to bypass librestart? >> I'm saying not all of the properties are managed by librestart. > > I don't understand how that means that these descriptions shouldn't be > in global.xml. This way, if I write a new restarter that uses > librestart, won't I have to duplicate all of this template information? > Or is that necessary because there's no way for me to specify templates > for properties in addition to those defined in global.xml? Yes. The latter. The design document which you saw early versions of articulated the tradeoffs in overrides, and we chose to reduce complexity at the risk of slightly less flexibility. >>>>> 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.) >>> By "It isn't in startd right now", do you mean that startd will not >>> interpret "signal,core" in the same way as "core,signal"? >> My apologies, I was in error. startd does accept both. > > Ok. So another entry is warranted, right? Yes. >>> And I don't understand how your response answers my first question; >>> please forgive me! >> Sorry. No, it currently does not. > > Is an RFE justified? And maybe a comment explaining we have to list > every combination because of this? OK. I'll make that happen. >>>>> - Did you consider separate patterns for the start, stop, and >>>>> refresh methods? >>>> Yes, but I didn't feel like there was enough benefit. >>> Wouldn't it allow us to declare that start and stop methods are >>> required? And won't the proposed change validate methods that we know >>> don't do anything, like "restart"? >> True. Are you requesting a change? > > Yes. Though I don't think it will cause enough of a problem that you > have to do it before integration. A follow-up bug would suffice. I'd prefer to do it before integration and will work on it. >>>>> cmd/cmd-inet/usr.lib/inetd/inetd.xml >>>>> 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. >>> Won't this cause user interfaces (such as "visual panels") to allow >>> negative values to be specified, which will be interpreted as enormous >>> timeouts? Maybe I'm missing something. >> Perhaps, but I don't see how that risk outweighs against breaking all >> inetd manifests in the wild. > > I suspect our definitions of "broken" differ. Would that cause the > manifests to be unimportable, or cause inetd not to behave properly, or > just cause svccfg to claim that they're invalid? If the last, then it > seems that this is a tradeoff between correct user interfaces and false > claims of invalidity. I believe you estimate the latter to be alarming > to users, and I can agree with that. And I suppose the difference > between an integer control and a count control would be pretty small. > So I'll agree with your judgement. OK. > > Would it be legitimate to say that there is room for enhancement in the > template framework, to allow a property to be one of a few types? A property type can be wildcarded. There's potential for the enhancement you describe, but I'm not yet sure how valuable it would be. The big thing is that inetd would need to be changed to accept either type. New inetd manifests which specified the property as type 'count' couldn't be used at all with old inetd. >>>>> 494: How many RPC versions are there? >>>> Depends on the service. Not sure what you're trying to get at. >>> Well if we know what the legitimate versions are, we can specify >>> constraints, can't we? >> Clearly I'm missing something. Where am I specifying constraints on RPC >> versions? > > Nowhere, which is the point. It seems to me that you could specify > a constraint on RPC versions if we know that they are limited to > a certain range. If you don't know, though, we can file an RFE for the > networking team to make this more precise later. Ah. After a search through the code and manpages, I don't currently see anywhere aside from its type that it's limited. liane
