>> cli/devices: rename some functions for consistency

subject should contain the keyword "trivial" (which always causes me to look at `git diff --color-words=.`).

  "cli/trivial: rename internal functions"


>> cli: split out do_cmd()

+nmc_do_cmd (NmCli *nmc, NMCCommand cmds[], const char *cmd, int argc, char  
                         ^^^ const


+static NMCCommand nmcli_cmds[] = {
      ^^^^ const


+typedef struct {
+        const char *cmd;
+        NMCResultCode (*func) (NmCli *nmc, int argc, char **argv);
+    void (*usage) (voi
indention



>> cli/device: use nmc_do_cmd()

-         nmc->return_value = NMC_RESULT_ERROR_USER_INPUT;
-         goto error;
+         return NMC_RESULT_ERROR_USER_INPUT;

good, dropping return_value, but let's just remove it entirely.


+static NMCCommand device_cmds[] = {
        ^^^^ const

>> cli/device: rename device_list() to get_device_list()

trivial



>> cli/device: split get_device()
    
Parsing the a single device name
       ^^^^^^^^
    
Shouldn't we support

nmcli device <CMD> [TYPE] <DEVICE>

like
nmcli device disconnect ifname eth0
nmcli device disconnect path /org/fd/NM/Devices/5
nmcli device disconnect path 5
nmcli device disconnect ifindex 42
nmcli device disconnect apath /org/fg/NM/ActiveConnection/7
nmcli device disconnect apath 7

like we also have `nmcli connection show [id|path|uuid] <CONNECTION>`.




+    NMDevice *device = get_device (nmc, &argc, &argv);

     ...

+    if (device) {
          show_device_info (device, nmc);
     } else {
+         NMDevice **devices = get_devices_sorted (nmc->client);
+         int i;


for `nmcli device show`, get_device will return %NULL and we show all devices (good). But in this case, get_device() will also do:
+    if (*argc == 0) {
+         if (nmc->ask)
+              ifname = ifname_ask = nmc_readline (PROMPT_INTERFACE);
+
+         if (!ifname_ask) {
+              g_string_printf (nmc->return_text, _("Error: No interfa...
+              nmc->return_value = NMC_RESULT_ERROR_USER_INPUT;
+              return NULL;
+         }

I think, get_device() should have a argument "allow_none", or alternatively you do
   device = argc > 0 ? get_device (nmc, &argc, &argv) : NULL;



In get_device(), the following if() is always FALSE (unless bug).
+    if (!ifname) {
+         g_string_printf (nmc->return_text, _("Error: No interface specified."));
+         nmc->return_value = NMC_RESULT_ERROR_USER_INPUT;
+         return NULL;
+    }



>> cli/device: add device name completion

- !(nm_streq0 (argv[2], "connection") || nm_streq0 (argv[2], "device"))
+ !NM_IN_STRSET (argv[1], "connection", "device")



 static NMCResultCode
 do_device_wifi (NmCli *nmc, int argc, char **argv)
 {
     GError *error = NULL;
 
+    /* Not (yet?) supported */
+    if (nmc->complete)
+         return nmc->return_value;

instead of adding this check to ever function, NMCCommand could have a field "supports_completion" and nmc_do_cmd() could return right away. I like that more, because then functions that support completion have to opt-in, instead of each one individually check and error out.
If you also set the flag in the toplevel nmcli_cmds, then you can drop the check
»···»···if ((argc == 2) || !(nm_streq0 (argv[2], "connection") || nm_
»···»···»···return nmc->return_value;



>> cli/connection: export read_connection_properties()
    
I know, nmcli is not consistent there, but could we give every function in header files a nm* prefix? For clients/cli/connections.h, a possible prefix would be nmc_connections_*(), and then call it nmc_connections_read_properties().


>> cli/device: add modify command
    
+                  "the connection profile. The changes have immediate effect. For multi-valued\n"
indention.




+                    mod|modi|modif|modify)
+                        if [[ ${#words[@]} -eq 3 ]]; then
+                            _nmcli_compl_COMMAND_nl "${words[2]}" "$(nmcli --complete-args connection add "" 2>/dev/null)"
+                        else
+                            _nmcli_array_delete_at words 0 1
+                            _nmcli_list_nl "$(nmcli --complete-args connection add "${words[@]}" 2>/dev/null)"

^^ "connection add" above.




+                    mod|modi|modif|modify)
+                    mon|moni|monit|monito|monitor)
you change the meaning of `nmcli device "m"|"mo"`. They should continue to mean "monitor". For that, you also must order "modify" after "monitor" in device_cmds.



+          <para>This command lets you do temporary changes to a connection active on
+          a particular device without preserving them.</para>


that makes it sound, like you are modifying the (settings) connection, but I have no better suggestion for now.




+
+    if (!read_connection_properties (info->nmc, connection, &info->argc, &info->argv, &error)) {
+                g_string_assign (nmc->return_text, error->message);
+                nmc->return_valu

indention.


+         g_error_free (error);

gs_free_error?





As a follow-up, for the modify command, we actually only need to read the list of devices (unless we implement `nmcli device modify apath /o/fd/NM/ActiveConnection/7`, in which case we first would have to search the proper ActiveConnection). Anyway, the point is, this *load* everything slows us so much down although for many cases we don't actually need it.


As another follow-up, could we step by step get rid of the fields in NmCli struct? First of all, the return_value seems wrong, why do we even set a global variable return_value, instead of having every function returning it's result? (which we already often do as well ("return nmc->return_value;")



Otherwise, very nice branch.

>>> cli/trivial: rename some functions for consistency
>
>subject should contain the keyword "trivial" (which always causes me to look at `git diff --color-words=.`).
>
>  "cli/trivial: rename internal functions"

Oh, learned something. I didn't know color-words.

>>> cli: split out do_cmd()
>
>+nmc_do_cmd (NmCli *nmc, NMCCommand cmds[], const char *cmd, int argc, char
>                         ^^^ const
>
>
>+static NMCCommand nmcli_cmds[] = {
>      ^^^^ const

Fixed.

>+typedef struct {
>+        const char *cmd;
>+        NMCResultCode (*func) (NmCli *nmc, int argc, char **argv);
>+    void (*usage) (voi
>indention

Fixed.

>>> cli/device: use nmc_do_cmd()
>
>-         nmc->return_value = NMC_RESULT_ERROR_USER_INPUT;
>-         goto error;
>+         return NMC_RESULT_ERROR_USER_INPUT;
>
>good, dropping return_value, but let's just remove it entirely.

Well; I'll gladly apply a fixup ;)

>+static NMCCommand device_cmds[] = {
>        ^^^^ const

Fixed.

>>> cli/device: rename device_list() to get_device_list()
>
>trivial

Ok.

>>> cli/device: split get_device()
>
>Parsing the a single device name
>       ^^^^^^^^

Fixed

>Shouldn't we support
>
>nmcli device <CMD> [TYPE] <DEVICE>
>
>like
>nmcli device disconnect ifname eth0
>nmcli device disconnect path /org/fd/NM/Devices/5
>nmcli device disconnect path 5
>nmcli device disconnect ifindex 42
>nmcli device disconnect apath /org/fg/NM/ActiveConnection/7
>nmcli device disconnect apath 7
>
>like we also have `nmcli connection show [id|path|uuid] <CONNECTION>`.

Maybe. We definitely don't support that in general. Some subcommands take
"ifname", some do optionally.

===
I'll finish addressing the review now, since the comments seem too complicated. Will get a beer now instead and see if they make more sense afterwards.

>>> cli/device: split get_device()

>+    NMDevice *device = get_device (nmc, &argc, &argv);
>
>     ...
>
>+    if (device) {
>          show_device_info (device, nmc);
>     } else {
>+         NMDevice **devices = get_devices_sorted (nmc->client);
>+         int i;
>
>
>for `nmcli device show`, get_device will return %NULL and we show all devices (good). But in this case, get_device() will also do:
>+    if (*argc == 0) {
>+         if (nmc->ask)
>+              ifname = ifname_ask = nmc_readline (PROMPT_INTERFACE);
>+
>+         if (!ifname_ask) {
>+              g_string_printf (nmc->return_text, _("Error: No interfa...
>+              nmc->return_value = NMC_RESULT_ERROR_USER_INPUT;
>+              return NULL;
>+         }
>
>I think, get_device() should have a argument "allow_none", or alternatively you do
>   device = argc > 0 ? get_device (nmc, &argc, &argv) : NULL;

Yes, the second option seems better to me.


>In get_device(), the following if() is always FALSE (unless bug).
>+    if (!ifname) {
>+         g_string_printf (nmc->return_text, _("Error: No interface specified."));
>+         nmc->return_value = NMC_RESULT_ERROR_USER_INPUT;
>+         return NULL;
>+    }

Right. Removed.

>>> cli/device: add device name completion
>
>- !(nm_streq0 (argv[2], "connection") || nm_streq0 (argv[2], "device"))
>+ !NM_IN_STRSET (argv[1], "connection", "device")

The same thing. It's ideally going away soon rather than later :(

> static NMCResultCode
> do_device_wifi (NmCli *nmc, int argc, char **argv)
> {
>     GError *error = NULL;
>
>+    /* Not (yet?) supported */
>+    if (nmc->complete)
>+         return nmc->return_value;
>
>instead of adding this check to ever function, NMCCommand could have a field "supports_completion" and nmc_do_cmd() could return right away. I like that more, because then functions that support completion have to opt-in, instead of each one individually check and error out.
>If you also set the flag in the toplevel nmcli_cmds, then you can drop the check
>»···»···if ((argc == 2) || !(nm_streq0 (argv[2], "connection") || nm_
>»···»···»···return nmc->return_value;

I don't know. It's supposed to go away soon and makes it easy to spot where completion needs to be finished.

>>> cli/connection: export read_connection_properties()
>
>I know, nmcli is not consistent there, but could we give every function in header files a nm* prefix? For clients/cli/connections.h, a possible prefix would be nmc_connections_*(), and then call it nmc_connections_read_properties().

Sounds like a good idea. Done.

>>> cli/device: add modify command
>
>+                  "the connection profile. The changes have immediate effect. For multi-valued\n"
>indention.

Fixed.

>+                    mod|modi|modif|modify)
>+                        if [[ ${#words[@]} -eq 3 ]]; then
>+                            _nmcli_compl_COMMAND_nl "${words[2]}" "$(nmcli --complete-args connection add "" 2>/dev/null)"
>+                        else
>+                            _nmcli_array_delete_at words 0 1
>+                            _nmcli_list_nl "$(nmcli --complete-args connection add "${words[@]}" 2>/dev/null)"
>
>^^ "connection add" above.

Fixed.

>+                    mod|modi|modif|modify)
>+                    mon|moni|monit|monito|monitor)
>you change the meaning of `nmcli device "m"|"mo"`. They should continue to mean "monitor". For that, you also must order "modify" after "monitor" in device_cmds.

Fixed.

>+          <para>This command lets you do temporary changes to a connection active on
>+          a particular device without preserving them.</para>
>
>
>that makes it sound, like you are modifying the (settings) connection, but I have no better suggestion for now.

Yes, spent some time figuring out the wording :( I've now changed it yet again, hopefully it's better now.

>+
>+    if (!read_connection_properties (info->nmc, connection, &info->argc, &info->argv, &error)) {
>+                g_string_assign (nmc->return_text, error->message);
>+                nmc->return_valu
>
>indention.
>
>
>+         g_error_free (error);
>
>gs_free_error?

Hmm, why not.

>As a follow-up, for the modify command, we actually only need to read the list of devices (unless we implement `nmcli device modify apath /o/fd/NM/ActiveConnection/7`, in which case we first would have to search the proper ActiveConnection). Anyway, the point is, this *load* everything slows us so much down although for many cases we don't actually need it.
>
>
>As another follow-up, could we step by step get rid of the fields in NmCli struct? First of all, the return_value seems wrong, why do we even set a global variable return_value, instead of having every function returning it's result? (which we already often do as well ("return nmc->return_value;")

Well, list of the possible follow-ups seems infinite to me now...

I intend to improve the completion coverage first; replace all sub-command parsing with nmc_do_cmd() first. That's going to make the rest easier anyway.

>Otherwise, very nice branch.

Updated the branch.

I do have more follow ups; but I'd prefer not to delay merge of the modify subcommand for the unrelated completions -- opened bug #768089 for that.

>> cli/device: split get_device()

get_device() is new code that makes heavy use of @nmc argument for no reason.
If we ever hope to get rid of that, we should avoid that.
The pair nmc->return_text and nmc->return_value should be replaced by a GError, possibly with a new special error domain "NMC_PROGRAM_ERROR" where the error codes directly map to the NMC_RESULT_ERROR_* values.

 static NMDevice *
-get_device (NmCli *nmc, int *argc, char ***argv)
+get_device (NMClient *client, gboolean ask, int *argc, char ***argv, GError **error)



rest lgtm

> cli/device: fix --help in usage

This seems inconsistent with other commands' syntax, e.g:

Usage: nmcli [OPTIONS] OBJECT { COMMAND | help }
Usage: nmcli connection { COMMAND | help }
Usage: nmcli radio { COMMAND | help }

If it needs to be fixed, I think it should be done everywhere.


> cli/device: add modify command

+  mod|modi|modif|modify)
+      if [[ ${#words[@]} -eq 3 ]]; then
+          _nmcli_compl_COMMAND_nl "${words[2]}" "$(nmcli --complete-args connection add "" 2>/dev/null)"

"connection add" -> "device modify"?

+      else
+          _nmcli_array_delete_at words 0 1
+          _nmcli_list_nl "$(nmcli --complete-args connection modify "${words[@]}" 2>/dev/null)"

"connection" -> "device" ?


Otherwise LGTM.

(In reply to Thomas Haller from comment #5)
> >> cli/device: split get_device()
> 
> get_device() is new code that makes heavy use of @nmc argument for no reason.
> If we ever hope to get rid of that, we should avoid that.
> The pair nmc->return_text and nmc->return_value should be replaced by a
> GError, possibly with a new special error domain "NMC_PROGRAM_ERROR" where
> the error codes directly map to the NMC_RESULT_ERROR_* values.
> 
>  static NMDevice *
> -get_device (NmCli *nmc, int *argc, char ***argv)
> +get_device (NMClient *client, gboolean ask, int *argc, char ***argv, GError
> **error)

Fixed the error handling. The nmc thing for the global flag is consistent with what we do elsewhere. Turning the global flags into function arguments seems outside of the scope of this branch if it even is a good idea.

(In reply to Beniamino Galvani from comment #6)
> > cli/device: fix --help in usage
> 
> This seems inconsistent with other commands' syntax, e.g:
> 
> Usage: nmcli [OPTIONS] OBJECT { COMMAND | help }
> Usage: nmcli connection { COMMAND | help }
> Usage: nmcli radio { COMMAND | help }
> 
> If it needs to be fixed, I think it should be done everywhere.

Dropped this patch. It seems like this is in fact the correct syntax. Good luck to the user that has a device called "help"...

> > cli/device: add modify command
> 
> +  mod|modi|modif|modify)
> +      if [[ ${#words[@]} -eq 3 ]]; then
> +          _nmcli_compl_COMMAND_nl "${words[2]}" "$(nmcli --complete-args
> connection add "" 2>/dev/null)"
> 
> "connection add" -> "device modify"?
> 
> +      else
> +          _nmcli_array_delete_at words 0 1
> +          _nmcli_list_nl "$(nmcli --complete-args connection modify
> "${words[@]}" 2>/dev/null)"
> 
> "connection" -> "device" ?

Fixed.