I have added a commit that allows modifying multiple properties. It is handy at least, but more importantly it allows changing related properties (without that the connection might no validate).

For example "manual" IP-method requires specifying IPs:
nmcli con mod em1-1 ipv4.method manual ipv4.addr "192.168.1.2/24, 10.10.1.5/8"

> cli: allow modifying multiple properties by 'nmcli con modify'

>-ambiguous, a keyword \fIid\fP, \fIuuid\fP or \fIpath\fP can be used.
>+ambiguous, \fIid\fP, \fIuuid\fP or \fIpath\fP keyword can be used.

You still need "a" (or "the") before "\fIid\fP"


One question on the behavior: if you do

  nmcli con modify em1 ipv4.addr 192.168.1.2 ipv4.addr 192.168.1.3

you'll end up with only one address... is that the most logical behavior? (I can see the argument for either way here... just want to make sure you've thought about it too.)

Random thought; what about:

nmcli con modify em1 +ipv4.addr 192.168.1.2/24
nmcli con modify em1 -ipv4.addr 192.168.1.2/24

or

nmcli con modify em1 ipv4.addr +192.168.1.2/24
nmcli con modify em1 ipv4.addr -192.168.1.2/24

Maybe that would be difficult to implement, but it would be a lot easier to type and maybe easier to remember than --append?

> cli: allow modifying multiple properties by 'nmcli con modify'

I think the "while (argc) {" loop needs a "g_strfreev (strv)" right before the end "}" brace.  Otherwise we'll leak all the split setting/props except the last one.  Which means it should also set strv = NULL; right after freeing it, and then at the end of the function if (strv) g_strfreev (strv);

(In reply to comment #2)
> > cli: allow modifying multiple properties by 'nmcli con modify'
> 
> >-ambiguous, a keyword \fIid\fP, \fIuuid\fP or \fIpath\fP can be used.
> >+ambiguous, \fIid\fP, \fIuuid\fP or \fIpath\fP keyword can be used.
> 
> You still need "a" (or "the") before "\fIid\fP"
> 
Reverted to the previous expression.

> 
> One question on the behavior: if you do
> 
>   nmcli con modify em1 ipv4.addr 192.168.1.2 ipv4.addr 192.168.1.3
> 
> you'll end up with only one address... is that the most logical behavior? (I
> can see the argument for either way here... just want to make sure you've
> thought about it too.)

Good point.
The last property wins here and it is equivalent to two subsequent commands:
nmcli con modify em1 ipv4.addr 192.168.1.2
nmcli con modify em1 ipv4.addr 192.168.1.3

With replacing '--append' with '+' there is more flexibility; append can be specified for each value rather than for the whole command.

(In reply to comment #3)
> Random thought; what about:
> 
> nmcli con modify em1 +ipv4.addr 192.168.1.2/24
> nmcli con modify em1 -ipv4.addr 192.168.1.2/24
> 
> or
> 
> nmcli con modify em1 ipv4.addr +192.168.1.2/24
> nmcli con modify em1 ipv4.addr -192.168.1.2/24
> 
> Maybe that would be difficult to implement, but it would be a lot easier to
> type and maybe easier to remember than --append?

Very good idea. It allows specifying set/append for particular properties.
I went with 'nmcli con modify em1 [+|-]<setting>.<property> <value>

For '+' it is quite easy and means appending value. For '-', i.e. removing values, it would be quite complicated to accept actual values and match them with what's in properties. So for '-', <value> is either index (e.g. for addresses, dns, ...) or option name (e.g. bond.options). This behaviour is also consistent with handling inside the interactive editor.

nmcli connection modify my-em1 +ipv4.addr "1.2.3.4/24, 1.2.3.5/24, 1.2.3.6/24"
nmcli connection modify my-em1 ipv4.dns 8.8.8.8 +ipv4.dns 8.8.4.4
nmcli con mod myeth -ipv4.dns 1
nmcli con mod bond0 +bond.options mii=600
nmcli con mod bond0 -bond.options downdelay


(In reply to comment #4)
> > cli: allow modifying multiple properties by 'nmcli con modify'
> 
> I think the "while (argc) {" loop needs a "g_strfreev (strv)" right before the
> end "}" brace.  Otherwise we'll leak all the split setting/props except the
> last one.  Which means it should also set strv = NULL; right after freeing it,
> and then at the end of the function if (strv) g_strfreev (strv);

Yeah, you are sure. Fixed.

The code looks good to me.

For the - option, would it be easier to do remove-by-value in nmcli if we added remove-by-value functions to libnm-util?  I see now that's why it's a lot easier to do remove-by-index, since the functions are already there.

If we added functions like nm_setting_ip4_config_remove_dns_by_value() then we could convert the DNS stuff over to DEFINE_REMOVER_OPTION and pass a rem_func() that just did inet_pton() and then passed the result to nm_setting_ip4_config_remove_dns_by_value().

How much work do you think that would be?  We could do it progressively too, it doesn't all have to happen at once.

Remove-by-index is perfectly valid, but I think it would be easier to use if we could do remove-by-value for common stuff too.  At least IP addressing stuff; 802.1x would also be a candidate but is less important.

Well, I added remove_*_by_value() functions to libnm-util for all indexed container-type properties.
Add using them in nmcli now. So both 'nmcli con modify' and 'nmcli con edit' can remove items ether by value or by index.

nmcli con modify -ipv4.addr "192.168.1.33/24 192.168.1.1"
nmcli con modify -ipv4.addr 0
are both valid commands

I have updated jk/nmcli-modify branch, please re-review.

(In reply to comment #7)
> Well, I added remove_*_by_value() functions to libnm-util for all indexed
> container-type properties.

I didn't look into these in too much detail, but the ones I did look at look right, and it looks like you remembered "Since: 0.9.10" and "NM_AVAILABLE_IN_0_9_10" everywhere.

>+/**
>+ PHASE2_* nm_setting_802_1x_remove_phase2_altsubject_match_by_value:

some sort of typo/cut-and-paste-o there


> cli: allow removing properties by-value (in addition to by-index)

>+			char *value_stripped = g_strdup (value); \
>+			g_strstrip (value_stripped); \

g_strstrip() returns its input, so you can do "value_stripped = g_strstrip (g_strdup (value));" if you want

>+static gboolean
>+_validate_and_remove_connection_permission (NMSettingConnection *setting,

maybe you should move the validation and GError-setting logic into the remove functions themselves?


> cli: update 'remove' command description for 'nmcli con edit'

>-          "properties it replaces the value (same as 'set').\n"));
>+          "properties the property value is replaces (same as 'set').\n"));

"is replaceD"

>+          "Removes the property value (sets it to default).\n"
>+          "Without an argument, the value is removed. For container-type\n"
>+          "properties you can specify an argument to remove just a single\n"
>+          "item or option. The argument is either a value or index of item\n"

Might be clearer to say something like:

    Removes the property value. For single-valued properties, this
    sets the property back to its default value. For container-type
    properties, this removes all the values of that property, or you
    can specify an argument to remove just a single item or option.
    The argument is...

> >+static gboolean
> >+_validate_and_remove_connection_permission (NMSettingConnection *setting,
> 
> maybe you should move the validation and GError-setting logic into the remove
> functions themselves?
> 

In general, other libnm-util also don't use GError and the error might not be interesting for other libnm-util users. In addition, in _validate_* nmcli function we may need a specific validation before calling the actual remove function. 

All other stuff fixed, thanks.

Committed to master:
1472d76 Merge changes for 'nmcli con modify' (rh #1044027)
ebb6013 cli: update 'nmcli con modify' help
2ef1979 man: update nmcli manual page to stress that we can remove values by name
a5673d1 cli: update 'remove' command description for 'nmcli con edit'
f29ad0b cli: allow removing properties by-value (in addition to by-index)
642cfde libnm-util: add *_remove_*_by_value() functions for 'vlan' setting
1e5370b libnm-util: add *_remove_*_by_value() functions for '802-11-wireless-security' setting
de646d9 libnm-util: add *_remove_*_by_value() functions for '802-11-wireless' setting
ca0aa81 libnm-util: add *_remove_*_by_value() functions for '802-3-ethernet' setting
b59bd75 libnm-util: add *_remove_*_by_value() functions for 'connection' setting
7c817d4 libnm-util: add *_remove_*_by_value() functions for '802-1x' setting
1303ac3 libnm-util: add *_remove_*_by_value() functions for 'ipv4' and 'ipv6' settings
c1ace1b cli: support removing items from container-type properties in 'nmcli con modify'
363ec8d man: update nmcli manual page - 'nmcli con modify' description
a1c3021 cli: allow modifying multiple properties by 'nmcli con modify'
431b758 cli: set vs. append property value by 'nmcli con modify' (rh #1044027)
a8e6094 cli: allow temporary connection modification by 'nmcli con modify'