If a user specified --privileged --device xxx:xxx, then xxx will anyway be available, so specifying --device xxx:xxx is redundant, and it should not cause an error. One option is to just ignore it, the other is to have a warning, something like this maybe:

 log.Warnf("Specifying a device %s is redundant for privileged mode", deviceMapping.PathOnHost)

Was thinking about that; --device allows setting r (read), w (write), and m (mknod) options; what options are used if the device passed through --device overlaps with a device on the host?

This is a valid concern which I totally overlooked. I think in case restrictive permissions (anything other than the most permissive rwm) are given, the code should return an error as it can't enforce the required permissions (e.g. read-only).

What do you think?

In case of a conflict, yes. Perhaps a warning is ok, after all, --privileged is expected to remove any restrictions, so perhaps lifting the restrictions that were specified to be ignored is ok.

@wenlxie what would your expectations be?

If a user specified --privileged --device xxx:xxx, then xxx will anyway be available, so specifying --device xxx:xxx is redundant, and it should not cause an error. One option is to just ignore it, the other is to have a warning, something like this maybe:

Thanks for the comments, will change the code.

Was thinking about that; --device allows setting r (read), w (write), and m (mknod) options; what options are used if the device passed through --device overlaps with a device on the host?

@thaJeztah @kolyshkin
For this case, I'd like to ignore the restrictions and respect the --privileged option.

One problem, though, is that the user (say, docker cli user) won't see it -- this is a warning in daemon logs. Or will they? Frankly I don't remember

@kolyshkin It will be at the daemon logs and user will not see it.

Extra space after Specify
Also should be Specifying.
These could all use some extra context on the log messages... e.g. logrus.WithField("container", c.ID)

Extra space after Specify, and should be Specifying

return errdefs.InvalidParameter(errors.New(errorMsg))

Additionally this should be validated on container create.

Can we clean up some of the grammar here?
Suggestion:

container device path must be different from any host device path for privileged mode containers

Done

Same comment as above re: grammer

Done

This might be cleaner like:

if _, err := os.Stat(...); err != nil {
  if os.IsNotExist(err) {
    continue
  }
  return nil, errors.Wrap(err, "error stating device path in container")
}
return nil, errors.Errorf(...)

Looks better, tks

I'd prefer this warning to look more like a warning; it's not really "redundant", but will be ignored (so you'd end up with different permissions than you configured);

Something like:

running in privileged mode: custom %s permissions for device %s are ignored

or

custom %s permissions for device %s are ignored in privileged mode

Done

Same question as above: in what situation do we end here?

The error message seems to be if deviceMapping.PathOnHost == deviceMapping.PathInContainer (but that's already handled above, and prints a warning ? (perhaps I'm looking wrong, 😅)

Change the logic to make it more clear

Looking at this again, I'm wondering if it's worth logging this, or if it's ok to just skip this warning: if there are no custom options (permissions) set, then the end result is exactly the same (with or without this device), so perhaps it's ok to just ignore this without logging a warning

@cpuguy83 wdyt? ^^

In what situation do we end here? (looks like we only get here if deviceMapping.PathOnHost != deviceMapping.PathInContainer and if deviceMapping.PathInContainer exists.

But the error says (if I interpret correctly); it's not allowed to add a device if deviceMapping.PathOnHost == deviceMapping.PathInContainer ?

@thaJeztah deviceMapping.PathOnHost == deviceMapping.PathInContainer has a continue

Trying to go through the logic:

1. --device /dev/vdc:/dev/vdc, means: deviceMapping.PathOnHost == deviceMapping.PathInContainer

• continue (no error)

2. --device /dev/vdc:/some/path, and /some/path doesn't exist in the container:

• continue (no error)

3. --device /dev/vdc:/some/path, and /some/path cannot be read in the container:

• error: error stating device path in container

4. --device /dev/vdc:/some/path, and exists in the container:

• error: container device path: /some/path must be different from any host device path for privileged mode containers

So for 4., the device target in the container is not allowed to overlap with an existing path in the container;

• Is this different for non-privileged containers? (I guess not; but we don't seem to be validating that)
• If deviceMapping.PathOnHost == deviceMapping.PathInContainer, should we still validate if PathInContainer is not an existing path in the container? (could there be situations where a device exists on the host at /foo/bar, but the container has a directory (or file) at that location?)
• The error message confuses me, because (IIUC) it's actually about the container path (not the host path), correct?

(again, apologies if I'm misunderstanding 😄)

@thaJeztah
For step 4:
The device target in the container is not allowed to overlap with an existing path on the host but not in the container.
For privileged mode containers , some hostpath maybe mapped into the container, so I do this check here. I admit it is a bit excessive, maybe just check the /dev/ path is enough? What do you think?

Same comment here re: error message wording

Done

One problem, though, is that the user (say, docker cli user) won't see it -- this is a warning in daemon logs. Or will they? Frankly I don't remember

s/contianer/container/

Done

Looks like this err is not used other than inside the if below, so to prevent confusion, it may be better to just;

if _, err := os.Stat(deviceMapping.PathInContainer); os.IsNotExist(err) {

Can you change error to something else (e.g., err2)? error is also the "type" for an error, so the name is confusing.

Was looking if we could get rid old all these nested ifs (for example, but extracting the "non-privileged" and "privileged" branches to separate functions), but may be best to leave that refactoring for another PR