Looks like this line is not needed at al if the MkdirAllAndChown() above doesn't use path.Dir(dir), but perhaps I misunderstood the logic, and it's there for a specific reason

(copy/paste from slack for everyone reading this)

This seems correct to me. First call basically ensures that /var/lib/docker/overlay/ exists and is a directory (and it needs to do it recursively this is why it's MkdirAll). Second call just creates directory under it and there's no need for recursion. Surely these two can be combined, maybe the reason was clearer error diagnostics in case something is wrong. Maybe there's something else I'm missing here, but I'd leave it as is, no point in optimization.

Same here

This is new; previously any error that was not an os.IsNotExist() was ignored; I think this is correct, but double check if there was a reason we ignored those errors

I would guess it is because the Stat is not really important. It's just a check to see if the operation can be skipped, a failure is acceptable here. If the path is invalid then the mkdir() below will error out with a more appropriate error message.

Ah, yes, you may be right; returning this error won't add much. Let me revert this part

I would guess it is because the Stat is not really important. It's just a check to see if the operation can be skipped, a failure is acceptable here. If the path is invalid then the mkdir() below will error out with a more appropriate error message.

small nitpick (optional): declaring and assigning zeroIDPair = idtools.IDPair{UID: 0, GID: 0} and passing it in all the MkdirAllAndChown.

I initially started doing that, but decided that it was only used in three locations here, and found this more readable (without having to look what zeroIDPair actually was).

For the other locations; I was thinking of adding a idtools.GetRootIDPair(d.uidMaps, d.gidMaps) utility, but kept that for a possible follow-up

Perhaps for the sake of readability/simplicity it makes sense to do something like

owner :=  idtools.IDPair{UID: rootUID, GID: rootGID}

and then use it

if err := idtools.MkdirAllAndChown(root, 0700, owner); err != nil {

otherwise it looks like excessive typing or autogenerated code (which might actually be true -- I bet @thaJeztah used good ol' sed or other form of find/replace 👍 )

Same for all similar places.

See my earlier comment; re: using a variable or not #35541 (comment)