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. ... > >>> 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? ... > >>>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. > > > >I meant global.xml specifically, not all manifests. But maybe I'm > >mistaken. Doesn't global.xml correspond to the reading rules of > >svc.startd & librestart, which we control? If so, it seems that there's > >no legitimate reason for a user to customize them. Do you have some use > >case in mind? > > An admin may eventually wish to override with a different description, > or perhaps even additional constraint for their site. I believe we > talked about these use cases early in the project. I suppose. > >>> 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. > > > >Sorry, which public descriptions are you talking about? > > The template "description" for the property group. Ok. > >>> 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? > >>>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? > >>> 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? > >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? > >>> - 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. > >>>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. 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? > >>> 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. Quoth Liane Praza on Fri, Oct 10, 2008 at 09:31:11AM -0700: > >>>>lib/libscf/common/scf_tmpl.c > > > >>>> 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. > >>Ok, maybe I don't understand. Is there a way to create a property > >>template through the manifest which does not have an > >>SCF_PROPERTY_TM_TYPE property? > > > >Sorry, thought I was looking at pg_type. I'll fix. > > Wait, no I won't. (Thanks to Antonello for noticing it's > inappropriate.) The property type is absolutely not required in the > prop pattern definition. That's why it's NOT_FOUND, which was my > original point. Ok, sorry, I asked the wrong question. I should have asked "Is there a way to create a property template through the manifest which has an SCF_PROPERTY_TM_TYPE with a single empty value?" And then I think the same decision process applies as for line 3972 above. David
