redefine error code as int64

[Mashimiao]

Currently, error code is string type and duplicate with comment.
I think that's not good, we should change it.

Signed-off-by: Ma Shimiao mashimiao.fnst@cn.fujitsu.com

[Mashimiao]

ping @liangchenye How do you think about it?

[liangchenye]

I'm afraid if will be a huge code.

       DefaultFilesystems Code = 0xc << iota
  	// NSPathAbs represents "This value MUST be an absolute path in the runtime mount namespace."
       NSPathAbs

I did a test, the 50th number is 27021597764222976. I'm not sure one day it will be out of range.

[Mashimiao]

I changed them to 0x**** + iota. I think out of range will not be a problem any more.

[liangchenye]

int is better than string in defining Code. But if we use int, when we add/update a *.go, we need to worry about whether there will be a collision. I just want to avoid this and decoupling different *.go files.

Another advantage is, if we want to tell the user what the code really mean, we just need to print it.

[Mashimiao]

We get rfc code from spec documents, and classify them based on spec documents, so I think collision is hard to meet...
And in struct specerror.Error, we have Err which contains error message, printing Code seems not so necessary and we haven't printed Code at any place currently.

[liangchenye]

I'm fine with this.
But I don't like 0xc << iota.
Can we make them a continuous codes in one .go file?

Just like http code, grouped by 2**, 4** .
In each group, the code is continuously.

[Mashimiao]

I have changed it to Code = 0xc001 + iota, each group will be continuous.

[wking]

On Thu, Oct 26, 2017 at 08:55:34AM +0000, 梁辰晔 (Liang Chenye) wrote: `int` is better than `string` in defining `Code`…

I prefer the string forms, because:

Another advantage is, if we want to tell the user what the code really mean, we just need to print it.

With this PR, the violated requirement only occurs in Go comments: $ git grep 'The value SHOULD be the conventional' origin/pr/501 origin/pr/501:specerror/config.go: // RootPathOnPosixConvention represents "The value SHOULD be the conventional `rootfs`." If we stick with strings, we can drop our current rephrasings: $ git grep 'path name should be' origin/master origin/master:validate/validate.go: specerror.NewError(specerror.RootPathOnPosixConvention, fmt.Errorf("path name should be the conventional 'rootfs'"), rspec.Version)) and just have: specerror.NewError(specerror.RootPathOnPosixConvention, rspec.Version) I think that would be easier to maintain going forward than using integer codes and creating our own spec-rephrasings. Is there a reason to prefer integers over strings? The only ones I can think of are: * Stability, but I don't see a benefit to requiring stability between runtime-tools releases. Folks consuming via Go will be compiling runtime-tools into their program and using the stable error values (e.g. RootPathOnPosixConvention), the internal representation of that error (int or string) can be opaque. * Speed, because integers may compare faster than strings (I'm not sure how Go does this, short strings may be singletons, like they are in Python). But I doubt folks will be comparing errors much outside of our test suite; they'll probably be more interested in whether there are any level+ errors. So what's the motivations for using ints that makes it worth maintaining local spec-rephrasings?

[Mashimiao]

On 10/27/2017 12:46 AM, W. Trevor King wrote: On Thu, Oct 26, 2017 at 08:55:34AM +0000, 梁辰晔 (Liang Chenye) wrote: > `int` is better than `string` in defining `Code`… I prefer the string forms, because: > Another advantage is, if we want to tell the user what the code > really mean, we just need to print it. With this PR, the violated requirement only occurs in Go comments: $ git grep 'The value SHOULD be the conventional' origin/pr/501 origin/pr/501:specerror/config.go: // RootPathOnPosixConvention represents "The value SHOULD be the conventional `rootfs`." If we stick with strings, we can drop our current rephrasings:

No, absolutely not. I can't agree with you. We need the rephrasings. There are too many strings in specerror/*.go that not suitable for error message.

$ git grep 'path name should be' origin/master origin/master:validate/validate.go: specerror.NewError(specerror.RootPathOnPosixConvention, fmt.Errorf("path name should be the conventional 'rootfs'"), rspec.Version)) and just have: specerror.NewError(specerror.RootPathOnPosixConvention, rspec.Version)

As I said above, is this really suitable for us? If error occurs, you print "The value SHOULD be the conventional `rootfs`." So, the user must ask which value? obviously, the string can't accurately tell user which value and why is invalid. But, our rephrasings can. In my opinion, we need Code in specerror/*.go, because we can take advantage of them to make sure if we validate all spec requires. There is no need we must use spec strings to print as error message. Why, as I said, because they are not so suitable.

…

I think that would be easier to maintain going forward than using integer codes and creating our own spec-rephrasings.

[wking]

On Thu, Oct 26, 2017 at 06:28:46PM -0700, Ma Shimiao wrote: There are too many strings in specerror/*.go that not suitable for error message.

Ah, good point.

If error occurs, you print "The value SHOULD be the conventional `rootfs`." So, the user must ask which value? obviously, the string can't accurately tell user which value and why is invalid.

We may be able to extract that information from the Markdown in most cases. But, yeah, until that happens, we want to continue using our local rephrasing. Still, sticking with strings keeps this door open in case we get good enough spec wording in enough cases. Changing back to integers closes that door, and I'm not clear on the upside.

In my opinion, we need Code in specerror/*.go, because we can take advantage of them to make sure if we validate all spec requires.

So integers give you easier enumeration? We auto-generate these values, can't we just auto-generate an array holding all of them as well?

[Mashimiao]

On 10/27/2017 12:41 PM, W. Trevor King wrote: We may be able to extract that information from the Markdown in most cases. But, yeah, until that happens, we want to continue using our local rephrasing. Still, sticking with strings keeps this door open in case we get good enough spec wording in enough cases. Changing back to integers closes that door, and I'm not clear on the upside.

At first, in my opinion, letting strings in spec to be suitable for error message, that's nearly impossible. spec is not born for that. If we agree on that, why do we insist on this? And why do we must have same strings with spec? Second, Users need accurate error message, and we can supply that by referring to spec. So, closing that door does not have any serious problems, maybe we just don't really need that door.

> In my opinion, we need Code in specerror/*.go, because we can take > advantage of them to make sure if we validate all spec requires. So integers give you easier enumeration? We auto-generate these

About upside, if as I said keeping Code as strings is not so necessary, I think letting them to be integer is more concise. at least will not be duplicate with comments, which looks a little .hmm... stupid... And we can take advantage of `iota` for enumeration.

values, can't we just auto-generate an array holding all of them as well?

I think auto-generate an array is a good opinion. Maybe we can improve the auto-generate tool later and put it into runtime-tools? @liangchenye

…

[wking]

On Thu, Oct 26, 2017 at 10:54:30PM -0700, Ma Shimiao wrote: On 10/27/2017 12:41 PM, W. Trevor King wrote: > We may be able to extract that information from the Markdown in > most cases. But, yeah, until that happens, we want to continue > using our local rephrasing. Still, sticking with strings keeps > this door open in case we get good enough spec wording in enough > cases. Changing back to integers closes that door, and I'm not > clear on the upside. At first, in my opinion, letting strings in spec to be suitable for error message, that's nearly impossible. spec is not born for that. If we agree on that…

We don't agree on that ;). Looking through the current error strings, these strike me as useful error messages: * The value of `cgroupsPath` MUST be either an absolute path or a relative path. * `names` MUST contain at least one entry. * The values MUST be absolute paths in the container namespace. * `layerFolders` MUST contain at least one entry. * For Hyper-V Containers, this field MUST NOT be set. * On Windows, `path` MUST be a volume GUID path. * The value SHOULD be the conventional `rootfs`. * A directory MUST exist at the path declared by the field. * If set, `timeout` MUST be greater than zero. * … Although again, the full path to the broken property (e.g. linux.cgroupsPath) and the value of the broken property (the empty string?) would be useful additional context. I do agree that in some cases we will want our own rephrased error, but I don't agree that we will *always* want our own rephrased error.

Second, Users need accurate error message, and we can supply that by referring to spec.

The spec doesn't currently provide requirement-level linking (e.g. there are currently several root errors [1] all linking to [2]). Having access to the exact clause we consider violated seems useful, especially in the absence of per-condition links.

>> In my opinion, we need Code in specerror/*.go, because we can take >> advantage of them to make sure if we validate all spec requires. > > So integers give you easier enumeration? We auto-generate these About upside, if as I said keeping Code as strings is not so necessary, I think letting them to be integer is more concise. at least will not be duplicate with comments, which looks a little .hmm... stupid...

Ah, I see, you're concerned about two instances of “This value MUST be an absolute path in the runtime mount namespace.” or similar. I don't see any of those yet: $ git describe --tags v0.2.0-48-g20af327 $ git grep represents specerror/*.go | sed -n 's/.*represents "//p' | sort | uniq -c | sort -n | tail -n3 1 maskedPaths (array of strings, OPTIONAL) will mask over the provided paths inside the container so that they cannot be read. The values MUST be absolute paths in the container namespace." 1 readonlyPaths (array of strings, OPTIONAL) will set the provided paths as readonly inside the container. The values MUST be absolute paths in the container namespace." 1 valid values are defined in the ... man page" but I agree that they would be unfortunate. In that case, I'm fine switching to integers with iota to avoid collisions. And I can always try to argue for bringing back the extracted spec strings if/when I have time to work up code that exposes them to the user. So now that I'm clear on the upside (and sorry it took me this long to see it), I'm +1 for this PR. [1]: https://github.com/opencontainers/runtime-tools/blob/20af327e2ea540f0b5a15cef80ad0d2521c778db/specerror/config.go#L146-L153 [2]: https://github.com/opencontainers/runtime-spec/blob/v1.0.0/config.md#root

[liangchenye]

LGTM

[zhouhao3]

LGTM