* Before, it was possible to add a slave connection even if a master did't exist (only a warning was issued). Now it is a hard error. We should allow adding slave connections without existing master (user may first create a slave, etc.)

* 'nmcli con add' description is updated in the manual page but not in connection.c for online help.
Actually I am not sure I understand the new options. Is the SLAVE_OPTIONS a separate group of options that comes in the order? Should
Actually I think it is beneficial to know the type of slave, e.g. next point, and maybe other reasons.
Maybe:
$ nmcli con add type ethernet slave-type bridge ...

* interactive connection adding now doesn't work well because if you are adding a slave connection nmcli doesn't know that and won't ask for corresponding options
$ nmcli -a con add type bridge-slave
vs. now
$ nmcli -a con add type ethernet

* are all examples in nmcli and nmcli-examples manual pages updated?

> cli: process slave parameters after the rest of the settings are set up
Doesn't
nmc_tab_completion.con_type = g_strdup (slave_type);
leak the string? Before it only used the constant string (setting name).

(In reply to Jiri Klimes from comment #1)

> * 'nmcli con add' description is updated in the manual page but not in
> connection.c for online help.
> Actually I am not sure I understand the new options. Is the SLAVE_OPTIONS a
> separate group of options that comes in the order? 

IMO better avoid adding another option group. These groups complicate bash completion and parsing of command line arguments.

(In reply to Jiri Klimes from comment #1)
> * Before, it was possible to add a slave connection even if a master did't
> exist (only a warning was issued). Now it is a hard error. We should allow
> adding slave connections without existing master (user may first create a
> slave, etc.)

Yes, fixed.

> * 'nmcli con add' description is updated in the manual page but not in
> connection.c for online help.
> Actually I am not sure I understand the new options. Is the SLAVE_OPTIONS a
> separate group of options that comes in the order?

Yes.

> Actually I think it is beneficial to know the type of slave, e.g. next
> point, and maybe other reasons.
> Maybe:
> $ nmcli con add type ethernet slave-type bridge ...

Fixed.

> * interactive connection adding now doesn't work well because if you are
> adding a slave connection nmcli doesn't know that and won't ask for
> corresponding options
> $ nmcli -a con add type bridge-slave
> vs. now
> $ nmcli -a con add type ethernet

Fixed.

> * are all examples in nmcli and nmcli-examples manual pages updated?

Hopefully.

> > cli: process slave parameters after the rest of the settings are set up
> Doesn't
> nmc_tab_completion.con_type = g_strdup (slave_type);
> leak the string? Before it only used the constant string (setting name).

Whoops. Fixed.

(In reply to Thomas Haller from comment #2)
> (In reply to Jiri Klimes from comment #1)
> 
> > * 'nmcli con add' description is updated in the manual page but not in
> > connection.c for online help.
> > Actually I am not sure I understand the new options. Is the SLAVE_OPTIONS a
> > separate group of options that comes in the order? 
> 
> IMO better avoid adding another option group. These groups complicate bash
> completion and parsing of command line arguments.

I don't think it's possible here.

adef7cf cli: add "nmcli c add master" to bash-completion
60236fc cli: add master option to "nmcli c add"
a7e43c1 cli: discover slave type for a connection with a master
387e979 cli: remove an extraneous _strip_master_prefix() call
1ce9d34 cli: process slave parameters after the rest of the settings are set up

(In reply to Lubomir Rintel from comment #3)
> (In reply to Jiri Klimes from comment #1)

> > > separate group of options that comes in the order? 
> > 
> > IMO better avoid adding another option group. These groups complicate bash
> > completion and parsing of command line arguments.
> 
> I don't think it's possible here.

+                  "  SLAVE_OPTIONS:\n"
+                  "    bridge:       [priority <0-63>]\n"
+                  "                  [path-cost <1-65535>]\n"
+                  "                  [hairpin yes|no]\n\n"
+                  "    team:         [config <file>|<raw JSON data>]\n\n"


why do we even need those slave options? With branch lr/cli-add-properties you could just do:

  nmcli con add type ethernet ... slave-type bridge ... -- bridge-port.priority 44








-                  "    bond-slave:   master <master (ifname, or connection UUID or name)>\n\n"
                   "    team:         [config <file>|<raw JSON data>]\n\n"
+               "    bond-slave:   master <master (ifname, or connection UUID or name)>\n\n"
                 
^^^ indention

(In reply to Thomas Haller from comment #4)
> (In reply to Lubomir Rintel from comment #3)
> > (In reply to Jiri Klimes from comment #1)
> 
> > > > separate group of options that comes in the order? 
> > > 
> > > IMO better avoid adding another option group. These groups complicate bash
> > > completion and parsing of command line arguments.
> > 
> > I don't think it's possible here.
> 
> +                  "  SLAVE_OPTIONS:\n"
> +                  "    bridge:       [priority <0-63>]\n"
> +                  "                  [path-cost <1-65535>]\n"
> +                  "                  [hairpin yes|no]\n\n"
> +                  "    team:         [config <file>|<raw JSON data>]\n\n"
> 
> 
> why do we even need those slave options? With branch lr/cli-add-properties
> you could just do:
> 
>   nmcli con add type ethernet ... slave-type bridge ... --
> bridge-port.priority 44

Well, this works well with the interactive mode, unlike properties. Discussed this on IRC; seems like there's no simple way to deal with that, so let's keep the options for now.

> -                  "    bond-slave:   master <master (ifname, or connection
> UUID or name)>\n\n"
>                    "    team:         [config <file>|<raw JSON data>]\n\n"
> +               "    bond-slave:   master <master (ifname, or connection
> UUID or name)>\n\n"
>                  
> ^^^ indention

Fixed

9d5f114 merge: branch 'lr/cli-add-master'
1ff98fc cli: add "nmcli c add master" to bash-completion
1375d9c cli: add master option to "nmcli c add"
aa12bb3 cli: discover slave type for a connection with a master
07912b6 cli: remove an extraneous _strip_master_prefix() call
00e0fff cli: process slave parameters after the rest of the settings are set up

btw. bash-completion doesn't work for correctly for SLAVE_OPTIONS. I think... how is this supposed to work?

Also, I don't think this works correctly:
example:
  $ nmcli --ask connection add type ethernet con-name TTT master tun7 autoconnect no slave-type team ifname 'test' cloned-mac a:1:2:3:4:5 config a


(In reply to Lubomir Rintel from comment #5)
> (In reply to Thomas Haller from comment #4)
> > (In reply to Lubomir Rintel from comment #3)
> > > (In reply to Jiri Klimes from comment #1)
> > 
> > why do we even need those slave options? With branch lr/cli-add-properties
> > you could just do:
> > 
> >   nmcli con add type ethernet ... slave-type bridge ... --
> > bridge-port.priority 44
> 
> Well, this works well with the interactive mode, unlike properties.
> Discussed this on IRC; seems like there's no simple way to deal with that,
> so let's keep the options for now.


Beside from the implementation being buggy...


I don't agree here.

"let's keep ... for now" is not correct, because adding it now as API means we have to keep it forever.


The synopsis for [SLAVE_OPTIONS]
 $ nmcli connection add COMMON_OPTIONS TYPE_SPECIFIC_OPTIONS \
       SLAVE_OPTIONS IP_OPTIONS [-- [+|-]<setting>.<property> <value> ...]
complicates the (already complicated) parsing of the add-arguments (see bash-completion and docu).


Also, the SLAVE_OPTIONS are redundant to the "--" options... redundancy is bad (and unneeded).


If --ask is broken for the `nmcli connection add [..] -- [MODIFIY_OPTIONS]` than that needs fixing anyway (or we just live with it for now and fix it later).


Reopening this. I'd remove the SLAVE_OPTIONS from add and rather fix --ask to work together with "--".

(In reply to Thomas Haller from comment #6)
> btw. bash-completion doesn't work for correctly for SLAVE_OPTIONS. I
> think... how is this supposed to work?
> 
> Also, I don't think this works correctly:
> example:
>   $ nmcli --ask connection add type ethernet con-name TTT master tun7
> autoconnect no slave-type team ifname 'test' cloned-mac a:1:2:3:4:5 config a

What's wrong there? That the configuration from command line overrides the editor? I think that the interaction between the configuration from editor and the command line was always poor; this branch doesn't really change anything there:

nmcli --ask connection add type ethernet con-name TTT autoconnect no ifname 'test' cloned-mac a:1:2:3:4:5

> (In reply to Lubomir Rintel from comment #5)
> > (In reply to Thomas Haller from comment #4)
> > > (In reply to Lubomir Rintel from comment #3)
> > > > (In reply to Jiri Klimes from comment #1)
> > > 
> > > why do we even need those slave options? With branch lr/cli-add-properties
> > > you could just do:
> > > 
> > >   nmcli con add type ethernet ... slave-type bridge ... --
> > > bridge-port.priority 44
> > 
> > Well, this works well with the interactive mode, unlike properties.
> > Discussed this on IRC; seems like there's no simple way to deal with that,
> > so let's keep the options for now.
> 
> 
> Beside from the implementation being buggy...
> 
> 
> I don't agree here.
> 
> "let's keep ... for now" is not correct, because adding it now as API means
> we have to keep it forever.
> 
> 
> The synopsis for [SLAVE_OPTIONS]
>  $ nmcli connection add COMMON_OPTIONS TYPE_SPECIFIC_OPTIONS \
>        SLAVE_OPTIONS IP_OPTIONS [-- [+|-]<setting>.<property> <value> ...]
> complicates the (already complicated) parsing of the add-arguments (see
> bash-completion and docu).

No, quite the contrary. It reuses the already existing code for "<master>-slave" types.

> Also, the SLAVE_OPTIONS are redundant to the "--" options... redundancy is
> bad (and unneeded).

Well, the same can be said about TYPE_SPECIFIC_OPTIONS. 

> If --ask is broken for the `nmcli connection add [..] -- [MODIFIY_OPTIONS]`
> than that needs fixing anyway (or we just live with it for now and fix it
> later).
> 
> 
> Reopening this. I'd remove the SLAVE_OPTIONS from add and rather fix --ask
> to work together with "--".

Removing SLAVE_OPTIONS adds some special casing; and I don't believe that use of properties justifies it. We're already using this approach (parameters that map to properties) for all types and slave types.

As for fixing --ask, I agree. However it would be a major rework: making the questionnaires map into properties. It should be probably done anyway; there's terrible duplication there; but it's not a simple task and should be probably dealt with separately.

(In reply to Lubomir Rintel from comment #7)
> (In reply to Thomas Haller from comment #6)
> > btw. bash-completion doesn't work for correctly for SLAVE_OPTIONS. I
> > think... how is this supposed to work?
> > 
> > Also, I don't think this works correctly:
> > example:
> >   $ nmcli --ask connection add type ethernet con-name TTT master tun7
> > autoconnect no slave-type team ifname 'test' cloned-mac a:1:2:3:4:5 config a
> 
> What's wrong there? That the configuration from command line overrides the
> editor? I think that the interaction between the configuration from editor
> and the command line was always poor; this branch doesn't really change
> anything there:
> 
> nmcli --ask connection add type ethernet con-name TTT autoconnect no ifname
> 'test' cloned-mac a:1:2:3:4:5

$ nmcli --ask connection add type ethernet con-name TTT master tun7 autoconnect no slave-type team ifname 'test' cloned-mac a:1:2:3:4:5 config b

first it says:
  There is 1 optional argument for 'team-slave' connection type.
  Do you want to provide it? (yes/no) [yes]

but if you type "yes", it doesn't ask for anything.

(this doesn't have to be fixed for 1.2. We can add a new BZ to track it -- or as I suggest below, drop SLAVE_OPTIONS).


> > The synopsis for [SLAVE_OPTIONS]
> >  $ nmcli connection add COMMON_OPTIONS TYPE_SPECIFIC_OPTIONS \
> >        SLAVE_OPTIONS IP_OPTIONS [-- [+|-]<setting>.<property> <value> ...]
> > complicates the (already complicated) parsing of the add-arguments (see
> > bash-completion and docu).
> 
> No, quite the contrary. It reuses the already existing code for
> "<master>-slave" types.

What do you mean? Why would adding a redundant way to configure options not complicate stuff? In bash-completion, it's quite complicated to get the parsing of the option-groups right.

> 
> > Also, the SLAVE_OPTIONS are redundant to the "--" options... redundancy is
> > bad (and unneeded).
> 
> Well, the same can be said about TYPE_SPECIFIC_OPTIONS. 

Definitely, but they were added at a time when `nmcli connection add` didn't support "--". It should have never existed, but as we have it now, we cannot drop it. We should however not extend this duplication of option syntax.


> > If --ask is broken for the `nmcli connection add [..] -- [MODIFIY_OPTIONS]`
> > than that needs fixing anyway (or we just live with it for now and fix it
> > later).
> > 
> > 
> > Reopening this. I'd remove the SLAVE_OPTIONS from add and rather fix --ask
> > to work together with "--".
> 
> Removing SLAVE_OPTIONS adds some special casing; and I don't believe that
> use of properties justifies it. We're already using this approach
> (parameters that map to properties) for all types and slave types.

Sorry, I don't follow. Why does it add special casing? Just remove SLAVE_OPTIONS entirely. The are already covered by the "--" options.



> As for fixing --ask, I agree. However it would be a major rework: making the
> questionnaires map into properties. It should be probably done anyway;
> there's terrible duplication there; but it's not a simple task and should be
> probably dealt with separately.

I don't like --ask anyway. It should be fixed eventually, no need to do it now. But adding now SLAVE_OPTION with the justification to support (already broken) --ask is not a good reasoning.

This problem has been fixed in the unstable development version. The fix will be available in the next major software release. You may need to upgrade your Linux distribution to obtain that newer version.

Follow-up in bug #764308