@duglin right now docker flag parsing relies on one hashmap to classify whether the incoming argument is a flag or not. That map includes both daemon and global flags. Unless we use two maps to store daemon and global args separately, I don't see a way to avoid this extra map.

N Separating IMHO would not be easy, it has dependencies everywhere.
For this error, we need some kind of data structure to look-up if the incoming argument is a daemon flag or not.

I will reorder the entries according to the help menu. And also add the deprecated flags which I have not added.

Shishir

@shishir-a412ed sorry if I wasn't clear. I wasn't talking about a map during parsing, I was talking about a map/struct during flag setup time. For example, if you look at the InstallFlags() func, that is what will register all of our daemon-side flags. And it does it via a list of flag.XXX calls. A hard coded list of calls. So knowing when a new one (like perhaps "-log-driver" at the end) is added is hard to detect when you also want to have your new code work. So, I was wondering if we could take all of the flag definitions from InstallFlags() and put them into a struct. InstallFlags() would then look over that struct to make all of the appropriate flag.XXX calls.
But, and this is the point, your new code can then use that same struct to perform the checks you need to know if any of them were specified w/o the -d flag. Then as new flags are added you won't need to update your code. Does this make any sense?

@duglin
I think I get the central idea of what you are trying to achieve.
You are trying to have a common struct/map that can be used to set the flags in daemon/config.go InstallFlags() method. By iterating over that data structure and populating the flags rather than hard coding it.

N use the same struct/map to do the look-up for my use-case to identify if it's a daemon flag or not.
So that updates are made at one central location rather than at two different places.

Few open questions:

1. " So, I was wondering if we could take all of the flag definitions from InstallFlags() and put them into a struct. InstallFlags() would then look over that struct to make all of the appropriate flag.XXX calls."
   Isn't that what we are doing right now ? flag.XXX call is using the daemon config struct to populate the flags. InstallFlags is a method typed to daemon config struct i.e. struct.InstallFlags()
   Not sure how you would iterate over a struct and avoid the hard coding ?

2. Populating a flag needs a struct. I am not sure how a map would work here, as iterating over a map won't tell me which field I am getting back. Checking daemon flags would needs a map to do the lookup {struct won't work here}. I feel these are two different use cases requiring two different data structures.

Nways let other maintainers jump in, and see what they feel.
I ll be happy to change the code, if there is a mutual consensus on a solution.
Till that time, I ll clean up the code.

Shishir

@duglin I have reordered the flags according to the help menu.
And added support for deprecated flags.

ping @tiborvass

Shishir

@shishir-a412ed to your questions:

1. no its not. Each call to flag.XXX is hand coded, not reading from a structure/array. This means that each time a new daemon flag is created people will need to not only add a new call to flag.XXX but also remember to update your Init() func - which I'm sure they'll forget. By having a single spot that is used to define the daemon flags (via a flag.XXX call) and setup your mapDaemonFlags structure, we avoid one being updated w/o the other.
2. wouldn't all daemon flags, except -d, be added to your mapDaemonFlags structure? If so, then we just need one if-statement to avoid adding it to your mapDaemonFlags.
   But perhaps I'm missing something, or defining such a structure/array is more complicated than I'm thinking.

@duglin I have re-factored the code. Does this solve your concern ?

@icecrime Thanks, I have made the changes. Please check now.

@shishir-a412ed I'm afraid this also needs to be rebased. The code looks good to me except for a minor comment.

sorry needs rebase

@jfrazelle I have rebased the PR.

@calavera I have changed this HasDaemonFlag = MapDaemonFlags[name] back to my original code.
This has a small bug. If a daemon flag is followed by a global flag. At the end of the flag parsing flag.Parse()
HasDaemonFlag would be set to false eventhough it should have been true.

@shishir-a412ed it makes sense, thanks for fixing it. I think it would make sense to add a few tests to validate this behavior.

I was also wondering if it would make sense to have two completely different FlagSet lists, one for the daemon flags and another for the rest. That way we wouldn't need to modify the flag package. I don't know if that would work as expected though.

We are planning on a more extensive patch to break these flags apart. But we need to do a lot of hacking for backwards compatibility. Sadly if we could go back to original docker and just have

docker daemon ...

All of this would be unnecessary...

@calavera I ll add the test cases in 1-2 days. Thanks.

@calavera I have added the test cases.
ping @tiborvass

@shishir-a412ed Thanks for your work Shishir, I'll carry this PR and #10535 to get it in asap.

Where is this going to be carried?

Carried in #13771