search

containerd / continuity

A transport-agnostic, filesystem metadata manifest system
https://containerd.io
Apache License 2.0
139 stars 67 forks source link

Should os.ModeCharDevice be taken into account? #139

Open thaJeztah opened 5 years ago

thaJeztah commented 5 years ago

Relates to https://github.com/moby/moby/pull/38758 / https://github.com/moby/moby/pull/38404#issuecomment-465151750 / https://github.com/golang/go/commit/a2a3dd00c934fa15ad880ee5fe1f64308cbc73a7

Due to a change in Go 1.12, some file modes are no longer being returned as os. ModeDevice, but as os.ModeCharDevice. This caused pulling images to break on docker (https://github.com/moby/moby/pull/38404#issuecomment-465151750).

I'm not very familiar with this repository, but a quick search showed some similar uses in this package, so I suspect some of those will have to be updated accordingly.

https://github.com/containerd/continuity/blob/004b46473808b3e7a4a3049c20e4376c91eb966d/fs/copy.go#L137-L140

https://github.com/containerd/continuity/blob/c2ac4ecc959316e616c37fd95143e972811bd12e/resource.go#L552-L553

https://github.com/containerd/continuity/blob/c2ac4ecc959316e616c37fd95143e972811bd12e/resource.go#L434-L437

https://github.com/containerd/continuity/blob/c2ac4ecc959316e616c37fd95143e972811bd12e/context.go#L501-L502

https://github.com/containerd/continuity/blob/c2ac4ecc959316e616c37fd95143e972811bd12e/context.go#L192

https://github.com/containerd/continuity/blob/c2ac4ecc959316e616c37fd95143e972811bd12e/devices/devices_unix.go#L46

thaJeztah commented 5 years ago

ping @cpuguy83 @AkihiroSuda @tonistiigi @dmcgowan PTAL

thaJeztah commented 5 years ago

Ok, so the approach here is more defensive than the variation in Moby; os.ModeCharDevice will always be set in combination with os.ModeDevice.

The code in this repository checks if os.ModeDevice (irregardless of it being a os.ModeCharDevice or not), whereas the Moby code checks for an exact match, expecting only os.ModeDevice to be set, therefore causing it to break with Go 1.12.