Assigning this to myself since I proposed the syntax changes in bug 682056. If anyone feels like taking it, feel free.

Reassigning to jklimes as he worked on this feature. Jirka, feel free to close the report if you believe it has been coped with.

NM bugzilla reorganization... sorry for the bug spam.

Current status? I think jklimes was working on this some time ago.

I worked on documenting libnm-util's settings and properties, not the keyfile plugin itself. It is complete now:  man 5 nm-settings
But as keyfile basically uses keys that maps 1-1 to properties, the manual page can be used as a reference.
However, we may still want to create manual pages for plugins
nm-plugin-keyfile
nm-plugin-ifcfg-rh
....
They would describe the plugins and could refer to nm-settings.

(In reply to comment #5)
> I worked on documenting libnm-util's settings and properties, not the keyfile
> plugin itself. It is complete now:  man 5 nm-settings

Ah, good.

> But as keyfile basically uses keys that maps 1-1 to properties, the manual page
> can be used as a reference.

That's unfortunately not 100% true (look at addresses, for example), so we should decide whether nm-settings should be enhanced with keyfile bits where due or whether separate documentation is still needed.

> However, we may still want to create manual pages for plugins
> nm-plugin-keyfile
> nm-plugin-ifcfg-rh

I would prefer:

nm-settings-keyfile, nm-connections or nm-keyfile
nm-settings-ifcfg-rh or nm-ifcfg-rh

The ifcfg-rh manpage would be especially useful as it could link to ifcfg documentation and describe which parts of it are supported.

> ....
> They would describe the plugins and could refer to nm-settings.

+1

bug 731406, which will be committed imminently, changes the nm-settings.5 / settings-docs.xml generation system in a way that's more reusable. You get an XML file with just the docs, libnm-util/nm-setting-docs.xml, and then man/ and docs/api/ apply XSLT stylesheets to that to get the data into the form they want. So, we could generate a second man page from nm-setting-docs.xml, pulling in overrides from a second XML file or something... (It would be nice if we could include the extra documentation in the nm-utils files themselves, but there's no way to put it into the gtk-docs without having it show up in the online API docs.)

Bug 731406 has now been committed.

jk/bgo683111-keyfile-ifcfg-rh-docs adds a documenation for both 'keyfile' and 'ifcfg-rh' plugin and generate manual pages from it: nm-settings-keyfile(5) and nm-settings-ifcfg-rh(5)

(In reply to comment #7)
> (It would be nice if we
> could include the extra documentation in the nm-utils files themselves, but
> there's no way to put it into the gtk-docs without having it show up in the
> online API docs.)

Yeah, it would be useful. But we can do even without gtk-docs.
I think it is good to have plugin documentation co-located with the properties, because they are tightly coupled.  Also, the developer will easily see that he should also add the description, when he adds a new property.

So, I have documented the properties in a simple homebrew format in nm-setting-*.c.  And I have written a script that extracts these comments and creates an XML. The generated manual pages take the info from this XML file.

Status of the branch:
Even if the work is mostly done, it is a fiddly work and thus I might have overlooked something, made copy&paste errors, etc. I am going to go through the documentation once again checking it and finishing what might be missing.
Nonetheless while you review, it would help if you could:
* check English language usage (mainly native speakers)
* check whether all properties are documented for ifcfg-rh
* suggest what are the main differences NM vs. initscrips for the section in nm-settings-ifcfg-rh
* what ifcfg-rh variables changed semantics against ifup
* Should we put configuration example for each connection type to the manual pages? What else info should go to the pages?
...

> libnm-util: add ifcfg-rh specific description for properties

The values that go into IEEE_8021X_INNER_AUTH_METHODS might also include "EAP-TLS", "EAP-MSCHAPV2", "EAP-MD5", or "EAP-GTC".  These "EAP-" prefixed values are only used with TTLS, because TTLS allows EAP-in-EAP.  Yeah, confusing.  It's worse because you can also use the non-EAP variants of some of these, like TTLS/MSCHAPV2.  Unfortunately, you just have to know which your authentication server uses :(

Should we somehow document the password flags too?  They get used pretty often and would be important to document.  For the ifcfg-rh plugin they even have human-readable values.

IEEE_8021X_PRIVATE_KEY and IEEE_8021X_INNER_AUTH_METHODS should have an example too; it's a path-based variable just like the CLIENT_CERT or CA_CERT ones.

Instead of having a "not handled by the ifcfg-rh plugin", should we just have some kind of "skip" notation, like "skip: true" maybe?

Would be nice to auto-generate the default/min/max values like libnm-util/generate-setting-docs.py does...  not sure how easy that would be.

"is NM extension." -> "is an NM extension."
"are NM extension." -> "are NM extensions."

"also PHYSDEV must be specified." -> "PHYSDEV must also be specified."

"Subchannels for IMB S390 hosts." -> "Subchannels for IBM S390 hosts."

"Login nmae for LEAP." -> name

The description for the 802.11 "MODE" appears to be a C&P error.


> libnm-util: generate-plugin-docs.pl script for extracting plugin docs

This does add a perl build-time dependency, but I guess gtk-doc requires perl anyway, so we're probably fine here.

(In reply to comment #10)
> > libnm-util: add ifcfg-rh specific description for properties
> 
> The values that go into IEEE_8021X_INNER_AUTH_METHODS might also include
> "EAP-TLS", "EAP-MSCHAPV2", "EAP-MD5", or "EAP-GTC".  These "EAP-" prefixed
> values are only used with TTLS, because TTLS allows EAP-in-EAP.  Yeah,
> confusing.  It's worse because you can also use the non-EAP variants of some of
> these, like TTLS/MSCHAPV2.  Unfortunately, you just have to know which your
> authentication server uses :(
> 
I have fixed the description of 'phase2-autheap' saying that IEEE_8021X_INNER_AUTH_METHODS can contain EAP-* values.

> Should we somehow document the password flags too?  They get used pretty often
> and would be important to document.  For the ifcfg-rh plugin they even have
> human-readable values.
> 
Added a section both to keyfile and ifcfg-rh.

> IEEE_8021X_PRIVATE_KEY and IEEE_8021X_INNER_AUTH_METHODS should have an example
> too; it's a path-based variable just like the CLIENT_CERT or CA_CERT ones.
> 
Added.

> Instead of having a "not handled by the ifcfg-rh plugin", should we just have
> some kind of "skip" notation, like "skip: true" maybe?
> 
Actually, I think it is useful to list all properties for ifcfg-rh. That way one can easily see that a property is not read/saved by the plugin (and thus not preserved).
If there are no plugin comments, the property won't be displayed.
For the settings, the better way may be saying: "The following properties are not supported by ifcfg-rh plugin: ppp, pppoe, serial, ..." instead listing it separately.

> Would be nice to auto-generate the default/min/max values like
> libnm-util/generate-setting-docs.py does...  not sure how easy that would be.
> 
Hmm, generate-setting-docs.py only gets the default value. We could just use the generated nm-settings.xml for it. But I think it is sufficient to refer to nm-settings(5) from nm-settings-keyfile(5). For ifcfg-rh, the values are specific anyway, so we are still better to use the explicit "default: " tag.

> "is NM extension." -> "is an NM extension."
> "are NM extension." -> "are NM extensions."
> 
> "also PHYSDEV must be specified." -> "PHYSDEV must also be specified."
> 
> "Subchannels for IMB S390 hosts." -> "Subchannels for IBM S390 hosts."
> 
> "Login nmae for LEAP." -> name
> 
> The description for the 802.11 "MODE" appears to be a C&P error.
> 
Fixed.

> 
> > libnm-util: generate-plugin-docs.pl script for extracting plugin docs
> 
> This does add a perl build-time dependency, but I guess gtk-doc requires perl
> anyway, so we're probably fine here.
Yeah, gtk-doc uses and depends on perl, so I think that's fine.

In addition, I have added/changed/enhanced several descriptions and pushed the branch again.

not sure, but wouldn't it be:
  "is an NM extension"
instead of:
  "is a NM extension"
?


>> libnm-util: generate-plugin-docs.pl script for extracting plugin docs

I am impressed of the shortness of this perl script. But I see one downside: with python we could load instances of NM objects and do some introspection on the code. This would allow us to generate more based on the actual type implementation and require less explicit documentation.

But anyway. I am fine with how you did it -- and I appreciate, that the documentation is so close to the code.




Looks all good to me. It does not have to be the perfect documentation when you commit it first. We can improve it over time now that we have a best-practice how to do it.

maybe you could move the commits that add generate-plugin-docs.pl and the XSLT first, and later add the actual code docs... seems more logical to me

+     * ---keyfile---
+     * property: mac-address-blacklist
+     * variable: mac-address-blacklist

often the variable and property are the same.
How about allowing to omit "variable", and fallback to "property"




how about supporting a "format-type".
-     * format: list of MACs (separated with semicolons)
+     * format-type: mac-address-list
similarly:
-     * format: ususal hex-digits-and-colons notation
+     * format-type: mac-address

and the script adds a missing "format:" entry itself by looking up in a dictionary. Advantage: do not duplicate actual text that is supposed to be the same for any type. You still can explicitly write "format:" to overwrite this.




In the generated man-page, there are tables for each setting. Can we force a common column-width?

In ifcfg-rh manual, maybe we could add another column stating that this property is an NM extension to initscripts. That seems interesting enough to highlight in a separate column.

(In reply to comment #12)
> not sure, but wouldn't it be:
>   "is an NM extension"
> instead of:
>   "is a NM extension"
> ?
> 
Yes. If I recall correctly, a/an should be chosen according to the pronunciation. So it is "an NM" (pronounces an [ən ɛn-ɛm]), but "a NetworkManager" (pronunced as [ə networkmanager].
Fixed.

> 
> >> libnm-util: generate-plugin-docs.pl script for extracting plugin docs
> 
> I am impressed of the shortness of this perl script. But I see one downside:
> with python we could load instances of NM objects and do some introspection on
> the code. This would allow us to generate more based on the actual type
> implementation and require less explicit documentation.
> 
We can use GObject introspection in Perl too. But, it would be sufficient to simply use nm-setting-docs.xml generated with generate-setting-docs.py, when necessary.

(In reply to comment #13)
> maybe you could move the commits that add generate-plugin-docs.pl and the XSLT
> first, and later add the actual code docs... seems more logical to me
Done.

(In reply to comment #14)
> +     * ---keyfile---
> +     * property: mac-address-blacklist
> +     * variable: mac-address-blacklist
> 
> often the variable and property are the same.
> How about allowing to omit "variable", and fallback to "property"
> 
Done.
 
> 
> how about supporting a "format-type".
> -     * format: list of MACs (separated with semicolons)
> +     * format-type: mac-address-list
> similarly:
> -     * format: ususal hex-digits-and-colons notation
> +     * format-type: mac-address
> 
> and the script adds a missing "format:" entry itself by looking up in a
> dictionary. Advantage: do not duplicate actual text that is supposed to be the
> same for any type. You still can explicitly write "format:" to overwrite this.
> 
That could be done. But there are only a few strings now, so I don't think that's needed ATM.
  
> In the generated man-page, there are tables for each setting. Can we force a
> common column-width?
I was trying to find out how to set column/table width. But I haven't found anything useful. These docbook/manual page conversions are quite magic.

(In reply to comment #15)
> In ifcfg-rh manual, maybe we could add another column stating that this
> property is an NM extension to initscripts. That seems interesting enough to
> highlight in a separate column.
The problem is that the tables tend to be quite wide, and the lines are wrapped on narrow displays. Thus I want to have as few columns as possible. However I agree that the extension information is important/useful. We might make it bold. Or maybe we could move it to the "variable" column.
Note that the extension 'flag' can't only be a yes/no value, because sometimes a property is represented with more variables and only some of them are extensions.

Anyway, I think the documentation is more or less complete. We can extend or tweak it later. Nevertheless, I would be happy if we enhance the section about NM vs. initscripts differences. Any suggestions? More examples?

(In reply to comment #16)
> (In reply to comment #12)
> > not sure, but wouldn't it be:
> >   "is an NM extension"
> > instead of:
> >   "is a NM extension"
> > ?
> > 
> Yes. If I recall correctly, a/an should be chosen according to the
> pronunciation. So it is "an NM" (pronounces an [ən ɛn-ɛm]), but "a
> NetworkManager" (pronunced as [ə networkmanager].
> Fixed.

I agree, I would also read it as [ən ɛn-ɛm].



> (In reply to comment #15)
> > In ifcfg-rh manual, maybe we could add another column stating that this
> > property is an NM extension to initscripts. That seems interesting enough to
> > highlight in a separate column.
> The problem is that the tables tend to be quite wide, and the lines are wrapped
> on narrow displays. Thus I want to have as few columns as possible. However I
> agree that the extension information is important/useful. We might make it
> bold. Or maybe we could move it to the "variable" column.
> Note that the extension 'flag' can't only be a yes/no value, because sometimes
> a property is represented with more variables and only some of them are
> extensions.

How about adding "(*)" behind the property name to mark NM extensions?
e.g.
       
│zone  │ ZONE(*)  |  
? 

and somewhere at the end of the table it reads:
"Names marked with (*) are NetworkManager specific extensions not understood by initscripts."

and in code: 

     /* plugins docs
      * ---ifcfg-rh---
      * property: zone
      * variable: ZONE
+     * nm-extension: yes
      * description: Trust level of this connection. The string is usually used
      *   for a firewall. ZONE is an NM extension.
      * example: ZONE=Work
      * ---end---
      */
where this flag, causes the XSLT to add the "(*)" to the @variable. (however you can achieve that :) )
Or maybe the perl script concatenates in this case ZONE+(*), but concatenating them in XSLT seems more flexible (but probably requires some XSLT know-how I cannot provide)



Also, now we need several perl libraries, which is fine as long as they are common extensions. But could we add some detection for them in configure? E.g. I needed at least installing perl-autodie.
And on rhel-7, I am missing perl-YAML-LibYAML. What do do about that? I think it is quite annoying, that now we require some non-common packages to fully build NM.


Also, I think you have to adjust the contrib/rpm scripts to include the new manual pages... (I did not test that though, please make sure that every commit correctly builds on the build server before merging).

(In reply to comment #17)
> (In reply to comment #16)
> Also, I think you have to adjust the contrib/rpm scripts to include the new
> manual pages... (I did not test that though, please make sure that every commit
> correctly builds on the build server before merging).

Ah, the NetworkManager.spec file includes 
  %{_mandir}/man1/*
  %{_mandir}/man5/*
  %{_mandir}/man8/*
so, according to my tests all is correct already(??).

(In reply to comment #17)
> Also, now we need several perl libraries, which is fine as long as they are
> common extensions. But could we add some detection for them in configure? E.g.
> I needed at least installing perl-autodie.
> And on rhel-7, I am missing perl-YAML-LibYAML. What do do about that? I think
> it is quite annoying, that now we require some non-common packages to fully
> build NM.

Yeah, we should at least make this part of configure, so that you're told which package you need instead of:

./generate-plugin-docs.pl keyfile nm-keyfile-docs.xml
Can't locate YAML/XS.pm in @INC (you may need to install the YAML::XS module) (@INC contains: /usr/local/lib64/perl5 /usr/local/share/perl5 /usr/lib64/perl5/vendor_perl /usr/share/perl5/vendor_perl /usr/lib64/perl5 /usr/share/perl5 .) at ./generate-plugin-docs.pl line 44.
BEGIN failed--compilation aborted at ./generate-plugin-docs.pl line 44.


Also, I wonder if for 'keyfile' we should just add *all* settings to the nm-settings-keyfile manpage, instead of just the ones that are different.  Since the content is auto-generated anyway, we wouldn't need to update two places.  Plus then nm-settings-keyfile is a one-stop reference, instead of having to look at both nm-settings and nm-settings-keyfile.  How much work would that be?

---

For the IPv4 and IPv6 routes sections, I think I'd reword the description to be:

+	/* plugins docs
+	 * ---keyfile---
+	 * property: routes
+	 * variable: route1, route2, ...
+	 * format: route/plen[,gateway,metric]
+	 * description: List of IP routes.
+	 * example: route1=8.8.8.0/24,10.1.1.1,77
+	 *   route2=7.7.0.0/16
+	 * ---end---

(and the same for IPv6).  I think that's a bit clearer than having it be the same description as the 'addresses' property.

---

connection/interface-name: s/bind/bound

---

That's all I've got.  Regardless of whether we choose to do Thomas' suggestions, this branch is a huge improvement over what we have right now (ie, nothing) :)

(In reply to comment #17)
> 
> How about adding "(*)" behind the property name to mark NM extensions?
> e.g.
> 
> │zone  │ ZONE(*)  |  
> ? 
> 
I have marked the extension variables with (*) in the "variable: " tag. Then, this is emphasized in XSLT to make it more visible.
 
(In reply to comment #19)
> (In reply to comment #17)
> > Also, now we need several perl libraries, which is fine as long as they are
> > common extensions. But could we add some detection for them in configure? E.g.
> > I needed at least installing perl-autodie.
> > And on rhel-7, I am missing perl-YAML-LibYAML. What do do about that? I think
> > it is quite annoying, that now we require some non-common packages to fully
> > build NM.
> 
> Yeah, we should at least make this part of configure, so that you're told which
> package you need instead of:
> 
> ./generate-plugin-docs.pl keyfile nm-keyfile-docs.xml
> Can't locate YAML/XS.pm in @INC (you may need to install the YAML::XS module)
> (@INC contains: /usr/local/lib64/perl5 /usr/local/share/perl5
> /usr/lib64/perl5/vendor_perl /usr/share/perl5/vendor_perl /usr/lib64/perl5
> /usr/share/perl5 .) at ./generate-plugin-docs.pl line 44.
> BEGIN failed--compilation aborted at ./generate-plugin-docs.pl line 44.
> 
I have removed "autodie" module. We don't need that.
The configure.ac has been enhanced to check for required modules (YAML::XS for now).

> Also, I wonder if for 'keyfile' we should just add *all* settings to the
> nm-settings-keyfile manpage, instead of just the ones that are different. 
> Since the content is auto-generated anyway, we wouldn't need to update two
> places.  Plus then nm-settings-keyfile is a one-stop reference, instead of
> having to look at both nm-settings and nm-settings-keyfile.  How much work
> would that be?
> 
Hmm, maybe we could. But I prefer having only the few differences there and not duplicating all the information. If we listed all properties, the special ones would become less evident. And what would we put to "description", for example?

> 
> For the IPv4 and IPv6 routes sections, I think I'd reword the description to
> be:
> 
> +    /* plugins docs
> +     * ---keyfile---
> +     * property: routes
> +     * variable: route1, route2, ...
> +     * format: route/plen[,gateway,metric]
> +     * description: List of IP routes.
> +     * example: route1=8.8.8.0/24,10.1.1.1,77
> +     *   route2=7.7.0.0/16
> +     * ---end---
> 
> (and the same for IPv6).  I think that's a bit clearer than having it be the
> same description as the 'addresses' property.
> 
Fixed.

> 
> connection/interface-name: s/bind/bound
> 
Fixed.

(In reply to comment #20)
> (In reply to comment #17)
> > Yeah, we should at least make this part of configure, so that you're told which
> > package you need instead of:
> > 
> > ./generate-plugin-docs.pl keyfile nm-keyfile-docs.xml
> > Can't locate YAML/XS.pm in @INC (you may need to install the YAML::XS module)
> > (@INC contains: /usr/local/lib64/perl5 /usr/local/share/perl5
> > /usr/lib64/perl5/vendor_perl /usr/share/perl5/vendor_perl /usr/lib64/perl5
> > /usr/share/perl5 .) at ./generate-plugin-docs.pl line 44.
> > BEGIN failed--compilation aborted at ./generate-plugin-docs.pl line 44.
> > 
> I have removed "autodie" module. We don't need that.
> The configure.ac has been enhanced to check for required modules (YAML::XS for
> now).

YAML::XS seems not available on RHEL-7. Does that mean we cannot build man pages on rhel7?

Maybe not highlight the (*)? Looks a bit strange, and the variable name is probably more important then the highlighted part.

We could replace "(*)" by "(+)" or "+". I think "(+)" is nice to show that it is an addition.

Otherwise looks good to me.

*** Bug 728802 has been marked as a duplicate of this bug. ***

(In reply to comment #21)
> 
> YAML::XS seems not available on RHEL-7. Does that mean we cannot build man
> pages on rhel7?

YAML::XS is based on Kirill Siminov's libyaml C library and it is one of the best YAML implementations.
There is a perl-YAML-LibYAML package in brew containing the module. However it is not tagged for general repos. So I have switched to YAML (perl-YAML) instead. I seem to remember having had some problems with that in the beginning. But now it works fine.

(In reply to comment #22)
> Maybe not highlight the (*)? Looks a bit strange, and the variable name is
> probably more important then the highlighted part.
> 
Actually I like the highlighting. It makes it easily visible that the (*) is not part of the variable name.

> We could replace "(*)" by "(+)" or "+". I think "(+)" is nice to show that it
> is an addition.
Done.

I have corrected some texts and pushed the code into master:
7917b4d Merge code for documenting 'keyfile' and 'ifcfg-rh' plugins (bgo #683111)
291674d libnm-util: add ifcfg-rh specific description for properties
71e0734 libnm-util: add keyfile specific description for properties
f27fac5 man: generate nm-settings-ifcfg-rh(5) manual page
24edee2 man: generate nm-settings-keyfile(5) manual page
1c2174a libnm-util: generate-plugin-docs.pl script for extracting plugin docs
def5d6c trivial: only use spaces in nm-setting.xml

There still may be some inaccurancies or visual issues. Let's fix them when we find them.
Later we can also move the descriptions from libnm-util to libnm-core.