Do not wrap errors from filepath / os as they are already wrapped.

[manugupt1]

While trying to import MountedFast into k8s to detect mount points
using openat2, several tests failed because they rely on
os.IsErrNotExist / os.IsPermission which do not actually test the
underlying error.

For a non-existent path, if we return the bare error, this problem
is solved. I tested for the following cases to ensure that
backward compatibility is not broken:

fmt.Println(os.IsNotExist(err)) returns false
fmt.Println(os.IsNotExist(&os.PathError{
		Op:   "lstat",
		Path: "somepath",
		Err:  err,
})) returns false

fmt.Println(os.IsNotExist(fmt.Errorf("unable: %w", err))) returns false
fmt.Println(errors.Is(fmt.Errorf("unable: %w", err), os.ErrNotExist))
returns true.
```

cc @kolyshkin @thaJeztah 

[thaJeztah]

Hmm bit on the fence on this one; we'll lose some context on the errors, and os.IsNotExist() generally should only be used when directly interacting with the os package; https://pkg.go.dev/os#IsNotExist

This function predates errors.Is. It only supports errors returned by the os package. New code should use errors.Is(err, fs.ErrNotExist).

[manugupt1]

Hmm bit on the fence on this one; we'll lose some context on the errors, and os.IsNotExist() generally should only be used when directly interacting with the os package; https://pkg.go.dev/os#IsNotExist

This function predates errors.Is. It only supports errors returned by the os package. New code should use errors.Is(err, fs.ErrNotExist).

Yep! This is because there is a lot of old code that relies os.ErrNotExist in k8s (see: kubernetes/kubernetes@49f76c3) and it is proving incredibly hard to unwrap them and maintain compatibility. os.IsNotExist is not the only error; there is os.IsPermission that it relies on.

[kolyshkin]

This part of patch might actually make sense (since the errors from os.Stat and filepath are probably already wrapped).

[kolyshkin]

Also, os.IsNotExist() does not work for errors wrapped using %w (see https://go.dev/play/p/RcpXLGqwO9l), and since

• both filepath.EvalSymlinks and os.Stat already contain path in their error message;
• filepath.Abs can only return an error from os.Getwd() which is already descriptive enough,
  this part of patch does makes sense.

[thaJeztah]

Also, os.IsNotExist() does not work for errors wrapped using %w (see https://go.dev/play/p/RcpXLGqwO9l), and since

The reverse is also true for some errors unfortunately (and based on the discussion in golang/go#39155, I doubt that'll change)

[kolyshkin]

@manugupt1 I do not understand the need for change; os.IsNotExist() should work just fine.

Please see the following example that works as expected: https://go.dev/play/p/kAy9rCXZaW4

If something is not working like you think it should, can you please provide the example similar to mine?

[kolyshkin]

@manugupt1 I do not understand the need for change

I stand corrected -- part of this PR actually makes sense (see https://github.com/moby/sys/pull/109#discussion_r843415781https://github.com/moby/sys/pull/109#discussion_r843415781)

[manugupt1]

@manugupt1 I do not understand the need for change; os.IsNotExist() should work just fine.

Please see the following example that works as expected: https://go.dev/play/p/kAy9rCXZaW4

If something is not working like you think it should, can you please provide the example similar to mine?

Hi Kir,
I had the following examples when I was testing: Wrapping with &os.PathError{err: Err} also returns false

https://go.dev/play/p/n156HDTShUu

[kolyshkin]

On Tue, Apr 5, 2022, 21:44 Manu Gupta ***@***.***> wrote: I had the following examples when I was testing: Wrapping with &os.PathError{err: Err} also returns false https://go.dev/play/p/n156HDTShUu

Well, the error from os.Lstat is already an os.PathError, and it is obviously a bug to wrap it in another os.PathError, so the example code is wrong. The code in this repo only wraps errors from unix into os.PathError, as errors from unix are bare errnos.

…

[kolyshkin]

@manugupt1 can you adopt the commit accordingly (i.e. only leave the part that patches normalizePath, and drop the part that wraps bare unix errors into os.PathError)?

[kolyshkin]

@manugupt1 can you adopt the commit accordingly (i.e. only leave the part that patches normalizePath, and drop the part that wraps bare unix errors into os.PathError)?

and fix the commit description accordingly (saying that errors from os or filepath do not need to be wrapped).

[kolyshkin]

LGTM, thanks!

[thaJeztah]

LGTM, thanks!