On Wed, Aug 12, 2015 at 09:12:57PM +0000, Keller, Jacob E wrote:
> General comments:
>
> it doesn't appear to enforce any specific change on configuration file
> format. I know my earlier proposal was maybe to at some point update
> the format. Specifically, change the per-port configurations to use
> "port:" or some prefix which might help ensure possible future
> expansion of sections. The primary reasoning being separating out
> options used by differing programs.
Although I have "port" as an identifier, the new code does not care
about the section names at all. It would work with files that had:
[phc2sys]
someOption 1
[pmc]
somethingElse 0
[future-section]
whatever 1
It would be up to the caller to specify the desired section. For
example:
x = config_get_int(cfg, "phc2sys", "someOption");
The only built in assumption is that every option can appear under
[global] as a default. It does *not* support options that can only
appear in a non-global section.
I will replace "port" with "section" in conifg.h and config.c.
> Are there any options you think might be more difficult to convert? I
> think some of the string options might not move as cleanly to the new
> API.. each call site would have to parse the string instead of parsing
> an enumeration. Maybe that can be solved by having a GLOB_ITEM_ENUM or
> something where the setup passes in a map of string->value that could
> be used instead, so that callsites get an enum/int of values instead of
> strings?
Yes, I was thinking of ENUM options with little string tables
attached, but I haven't yet worked out the details.
> Other comment is whether we want phc2sys and ptp4l to read from the
> same configuration file or should they just read separate ones.
I am not sure what is better. We had some discussion on this point
that I should look up again. The new code doesn't really care.
If we wanted distinct sets of legal options, then we would need
separate config_tab[] instances and different config_init() functions
(or one config_init(flavor) with switch/case inside). That change
will be easy to add, if we decide we need it later on.
> Aside from my general comments, I looked over the series and it looks
> great. Most of my comments here wouldn't sway me to say don't apply it
> now. We can always continue to work towards moving all the options from
> legacy into the new format.
Thanks for the review!
Richard
------------------------------------------------------------------------------
_______________________________________________
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
