This should rather return an error and cmd/dockerd/daemon.go should catch that err.

Yes I think that would be better; I also think we can drop the invalid registry mirror configurations and invalid insecure registry configurations prefix here; the error itself looks pretty self-describing already;

invalid mirror: unsupported scheme "example.com" in "example.com:5000

When handling in the daemon, we could have a prefix added, similar as what's used for reloading the configuration, e.g.:

Error configuring the daemon: 

Yes I think that would be better; I also think we can drop the invalid registry mirror configurations and invalid insecure registry configurations prefix here; the error itself looks pretty self-describing already;

invalid mirror: unsupported scheme "example.com" in "example.com:5000

When handling in the daemon, we could have a prefix added, similar as what's used for reloading the configuration, e.g.:

Error configuring the daemon: 

looks like you forgot to point to testCase.err here, janky:

registry/config_test.go:196: missing argument for Errorf("%s"): format reads arg 1, have only 0 args

How about adding a second (error) return value to NewService()? This would leave validation to the service and the errors when loading mirrors and registry, etc. wouldn't be ignored anymore (see a similar change in bd3cf1e). Also, notice that the change in the current form is likely to break what @stevvooe suggests in #34319, namely to add private-registry-mirror semantics to --registry-mirror.

tl;dr To me it seems like redundant checks, so I suggest to add a second error return value to NewService() and leave validation there.

I thought about it and the main issues were:

• I will have to update other parts in the code
• There is overhead. Loads functions are called in newServiceConfig and will be called again in NewService.

I thought maybe it would be a good idea to change newServiceConfig to return error but the main reason I stopped is because it broke a lot of tests and I wasn't sure I should go that path.

WDYT? 💭

Ah yes, that makes more sense. newServiceConfig should be the right place!

@vrothberg 👍
@AkihiroSuda @thaJeztah can you please give feedback.

But we still need to pass an error to daemon start, which implies adding an error return to newService (as it calls newServiceConfig). Doesn't it?

Yes, actually we do otherwise there is no way to propagate that error.

In a second thought we can create a new function called NewServiceWithError and use it in daemon/dockerd.go
In that case we break less parts and still be able to make it work.

I think that a function that parses or validates user input should return an error, so I prefer to do that for newService. But that's just my 2 cents :-)