GNOME Bugzilla – Bug 697169
[review] jklimes/cli-syntax: changes to nmcli without 'connection addition' feature
Last modified: 2013-06-04 12:51:58 UTC
Please review jklimes/cli-syntax branch containing changes to nmcli.
One note, there are some commits that changes previous commits syntax. I didn't squashed them together, first it would possibly cause problems when rebasing, and second I basically want to retain the history. So it may be good to compile and run the stuff first to see and play with the final version.
just look at the man page: "networking" and "radio" have the same documentation ("Get or set general networking state of NetworkManager...") There are two stray references to "WWAN" instead of "mobile". (And I don't think "mobile" should be all-caps like it is in most places?) "nm con down" lists an "apath" keyword, but doesn't say what it does. "nm con delete" shows id, uuid, or path in the summary, but only says id or uuid in the docs after that. "nm dev wifi connect" seems like it should be part of "nm con", not "nm dev", since it's primarly a connection-related action. Also, the "private" option to "wifi connect" seems like it shouldn't have a "--" in front of it, because it's part of the connection definition, not something that changes nmcli's behavior like --timeout or --nowait.
(In reply to comment #2) > just look at the man page: > > "networking" and "radio" have the same documentation ("Get or set general > networking state of NetworkManager...") > > There are two stray references to "WWAN" instead of "mobile". (And I don't > think "mobile" should be all-caps like it is in most places?) > > "nm con down" lists an "apath" keyword, but doesn't say what it does. > > "nm con delete" shows id, uuid, or path in the summary, but only says id or > uuid in the docs after that. > I've fixed all these errors and also correct some typos and other problems. id|uuid|path|apath description added to "show active" and other commands refer to it. Feel free to point out bad English or clumsy expressions. > "nm dev wifi connect" seems like it should be part of "nm con", not "nm dev", > since it's primarly a connection-related action. > This command is somewhere in the middle, because it creates a connection but also activates it on a device. And there is also "wifi" subcommand of "device", so I lean towards "device connect". I had talked to jpirko before adding the command and he had a similar feeling. > Also, the "private" option to "wifi connect" seems like it shouldn't have a > "--" in front of it, because it's part of the connection definition, not > something that changes nmcli's behavior like --timeout or --nowait. Thinking about it a bit and presenting it to pavlix and jpirko as well, I guess, we should move --nowait and timeout options to global nmcli options. As for "--private" I have no preference, I can remove "--" and maybe add yes|no argument to it: [private yes|no] What is your opinion?
> cli: make id|uuid optional for 'nmcli connection delete' If there's an unrecognized connection in the middle of a list of connections you're going to delete, then the code will delete everything up until the error, and then fail. It might be nicer to build up the list of connections to delete first, then in a second loop actually ask NM to delete them. That way the operation is all-or-nothing. Alternatively, the code could just warn about unrecognized connections and delete everything it can delete, which is what tools like 'rm' and 'yum' do; they warn about invalid input but perform the operation on all the input parameters that do exist. I suppose the other operations that accept multiple connections, like up, down, list, delete, etc.
> cli: nmc_get_user_input() util function for getting user input In nmc_get_user_input() we'll access random memory if read == 0. I don't actually know how that could happen, but the if (read < 1 || ...) line should probably be if (read <= 0 || ...) just to make sure. Then you can actually get rid of the read == 1 check too: if (read <= 0 || line[0] == '\n')
> cli: 'connection [up | down]' - ask for a connection name if not provided Given that the code matches anything, an ID, a UUID, path, index, or active path, we should probaby have a more descriptive prompt than "Connection name:". Maybe "Connection (name, ID, UUID, or path): ". That may be too verbose, I'm not really sure, but better to start with more verbose and reduce it later I think.
I'm not 100% sold on the WWAN -> Mobile switch either; terminology isn't very consistent among other OSs either. WWAN is pretty popular in other OSs and with OEM connection managers (HP and Lenovo use WWAN pretty much everywhere, Apple seems to use WWAN too), while other stuff uses Mobile Broadband. I prefer Mobile Broadband, but that's a really long string that doesn't often fit in UIs very well :( But I'm not sure shortening that to just "Mobile" works any better than WWAN...
> man: update nmcli manual page for recent changes Ok, here comes the nitpicking :) "This option can be used to force \fInmcli\fP to skip checking \fInmcli\fP and \fINetworkManager\fP versions compatibility. Use it with care, because using incompatible version may produce incorrect results." "versions compatibility" -> "version compatibility" (it's an adjective here thus not plural) "incompatible version" -> "incompatible versions" (it's a noun here thus plural) --- "When using this option \fInmcli\fP will ask for some missing mandatory arguments. That's why do not use the option for non-interactive purposes, like scripts, etc." maybe make this "When using this option nmcli will stop and ask for any missing required arguments, so do not use this option for non-interactive purposes like scripts." --- "Use this object to inquire NetworkManager ..." This is used in a couple places. Should probably be "Use this object to show NetworkManager ..." instead of "inquire". --- "Disabling networking puts all devices to 'unmanaged' state." -> "... changes all devices to the 'unmanaged' state." --- "Inquire or set status of mobile in NetworkManager. If no arguments are supplied, mobile status is printed; \fIon\fP enables mobile; \fIoff\fP disables mobile." As in my earlier comment on the WWAN/Mobile thing, the use of "mobile" here doesn't really work in English, and neither does WWAN. This is a place where "mobile broadband" makes more sense than either "mobile" or "WWAN" really. So perhaps: "Show or set the status of WWAN/mobile broadband in NetworkManager. If no arguments are supplied, mobile broadband status is printed; on enables mobile broadband, off disables it." --- "at once" -> "at the same time" (just "at once" by itself means "right now" instead of "at the same time", but "all at once" would work too, except that sounds odd due to the previous "all" in the sentence) --- "Get information about configured \fINetworkManager\fP's connections." NetworkManager's -> NetworkManager (another adjective, which can't have ownership of anything like a noun can) --- Shows active connections. Without a parameter, all active connections are listed. In order to show the connection details, \fI<ID>\fP has to be provided. \fIid\fP, \fIuuid\fP, \fIpath\fP and \fIapath\fP keywords can be used if \fI<ID>\fP is ambiguous." "has to be" -> "must be" also same for configured connections just below the active section, and for the "show <iface>" section. --- "+Activate a connection. The connection is identified by its name, UUID od D-Bus" od -> or --- "Instruct \fINetworkManager\fP to scan..." "Request that NetworkManager immediately scan..." --- The rest of the branch looks good.
(In reply to comment #4) > > cli: make id|uuid optional for 'nmcli connection delete' > > If there's an unrecognized connection in the middle of a list of connections > you're going to delete, then the code will delete everything up until the > error, and then fail. It might be nicer to build up the list of connections to > delete first, then in a second loop actually ask NM to delete them. That way > the operation is all-or-nothing. > > Alternatively, the code could just warn about unrecognized connections and > delete everything it can delete, which is what tools like 'rm' and 'yum' do; > they warn about invalid input but perform the operation on all the input > parameters that do exist. > New commit added (the top one): cli: 'connection delete' - do not stop on invalid connection arguments > I suppose the other operations that accept multiple connections, like up, down, > list, delete, etc. Actually, 'con up' and 'con down' only take one connection at a time. I guess I did it (partly) due to reporting progress issues (if the operations would happen in parallel). 'con show configured|active' support multiple connections.
(In reply to comment #5) > > cli: nmc_get_user_input() util function for getting user input > > In nmc_get_user_input() we'll access random memory if read == 0. I don't > actually know how that could happen, but the if (read < 1 || ...) line should > probably be if (read <= 0 || ...) just to make sure. Then you can actually get > rid of the read == 1 check too: > > if (read <= 0 || line[0] == '\n') read < 1 is the same as read <= 0 I think the code is right. I don't want to return empty string nor single newline, but NULL instead. The 'if' branch does that: if read == -1 or 0 or there is only '\n' in the buffer, NULL is returned. Else branch is executed when read is 1,2,3,... and it trims the last '\n'.
(In reply to comment #6) > > cli: 'connection [up | down]' - ask for a connection name if not provided > > Given that the code matches anything, an ID, a UUID, path, index, or active > path, we should probaby have a more descriptive prompt than "Connection name:". > Maybe "Connection (name, ID, UUID, or path): ". That may be too verbose, I'm > not really sure, but better to start with more verbose and reduce it later I > think. Fixed, made the string translatable and merged to the corresponding commits.
(In reply to comment #7) > I'm not 100% sold on the WWAN -> Mobile switch either; terminology isn't very > consistent among other OSs either. WWAN is pretty popular in other OSs and > with OEM connection managers (HP and Lenovo use WWAN pretty much everywhere, > Apple seems to use WWAN too), while other stuff uses Mobile Broadband. I > prefer Mobile Broadband, but that's a really long string that doesn't often fit > in UIs very well :( But I'm not sure shortening that to just "Mobile" works > any better than WWAN... I really don't know what is better. I think Dan Winship suggested we use mobile instead of WWAN. So fight with him and tell me who won ;-)
(In reply to comment #8) > > man: update nmcli manual page for recent changes > > Ok, here comes the nitpicking :) > Thanks. All stuff corrected and merged to the manpage commit. The whole branch was rebased to master.
(In reply to comment #10) > (In reply to comment #5) > > > cli: nmc_get_user_input() util function for getting user input > > > > In nmc_get_user_input() we'll access random memory if read == 0. I don't > > actually know how that could happen, but the if (read < 1 || ...) line should > > probably be if (read <= 0 || ...) just to make sure. Then you can actually get > > rid of the read == 1 check too: > > > > if (read <= 0 || line[0] == '\n') > > read < 1 is the same as read <= 0 > > I think the code is right. I don't want to return empty string nor single > newline, but NULL instead. The 'if' branch does that: if read == -1 or 0 or > there is only '\n' in the buffer, NULL is returned. > Else branch is executed when read is 1,2,3,... and it trims the > last '\n'. Totally correct. For some reason I was seeing a "-1" there but the existing code is right. Sorry!
(In reply to comment #11) > (In reply to comment #6) > > > cli: 'connection [up | down]' - ask for a connection name if not provided > > > > Given that the code matches anything, an ID, a UUID, path, index, or active > > path, we should probaby have a more descriptive prompt than "Connection name:". > > Maybe "Connection (name, ID, UUID, or path): ". That may be too verbose, I'm > > not really sure, but better to start with more verbose and reduce it later I > > think. > > Fixed, made the string translatable and merged to the corresponding commits. There's a missing terminating ")" in the string on each of these three commits, so we need: -nmc_get_user_input (_("Connection (name, UUID, or path: ")); +nmc_get_user_input (_("Connection (name, UUID, or path): "));
(In reply to comment #9) > (In reply to comment #4) > > > cli: make id|uuid optional for 'nmcli connection delete' > > > > If there's an unrecognized connection in the middle of a list of connections > > you're going to delete, then the code will delete everything up until the > > error, and then fail. It might be nicer to build up the list of connections to > > delete first, then in a second loop actually ask NM to delete them. That way > > the operation is all-or-nothing. > > > > Alternatively, the code could just warn about unrecognized connections and > > delete everything it can delete, which is what tools like 'rm' and 'yum' do; > > they warn about invalid input but perform the operation on all the input > > parameters that do exist. > > > New commit added (the top one): > cli: 'connection delete' - do not stop on invalid connection arguments New commit looks good.
(In reply to comment #13) > (In reply to comment #8) > > > man: update nmcli manual page for recent changes > > > > Ok, here comes the nitpicking :) > > > > Thanks. > All stuff corrected and merged to the manpage commit. > > The whole branch was rebased to master. Manpage changes look good. So we're just waiting on the mobile vs. WWAN thing, I've pinged danw to discuss.
I don't remember the details of the discussion where I was preferring "mobile broadband" over WWAN, so I don't have any good arguments for it now. I'm fine sticking with WWAN if that's what other vendors use.
(In reply to comment #15) > There's a missing terminating ")" in the string on each of these three commits, > so we need: > > -nmc_get_user_input (_("Connection (name, UUID, or path: ")); > +nmc_get_user_input (_("Connection (name, UUID, or path): ")); Fixed. (In reply to comment #18) > I don't remember the details of the discussion where I was preferring "mobile > broadband" over WWAN, so I don't have any good arguments for it now. I'm fine > sticking with WWAN if that's what other vendors use. Well, it looks like vendors use WWAN term for mobile broadband. And being short it fits in UI. So, I removed "cli: rename 'wwan' -> 'mobile'" commit and updated the man page. Some links on WWAN vs. mobile broadband: http://www.vaio-link.com/troubleshoot/wwan/faq_wwan.html http://h71036.www7.hp.com/hho/us/en/pclc/articles/connectivity-wireless-wwan-wlan.html http://download.lenovo.com/ibmdl/pub/pc/pccbbs/mobiles_pdf/c79szac51.pdf
I think the branch is good to merge then; all outstanding issues have been addressed. You'll run into merge errors because I merged dcbw/private-bus today, but I already fixed all those and pushed the result to the 'dcbw/cli-syntax-merge' branch; take a look at that and feel free to use that branch for the final merge to git master if you like.
(In reply to comment #20) > I think the branch is good to merge then; all outstanding issues have been > addressed. > > You'll run into merge errors because I merged dcbw/private-bus today, but I > already fixed all those and pushed the result to the 'dcbw/cli-syntax-merge' > branch; take a look at that and feel free to use that branch for the final > merge to git master if you like. Merged into master using your 'dcbw/cli-syntax-merge', thanks.
NM bugzilla reorganization... sorry for the bug spam.
NM master commits (for the record): (last) 654a79b cli: update bash completion file for the new syntax c84315f cli: 'connection delete' - do not stop on invalid connection arguments b68dbf8 man: update nmcli manual page for recent changes 7dddead trivial: format nmcli usage help 15398f6 trivial: fix some comments and error messages in devices.c 9ebe727 cli: add 'nmcli general logging' e2d8ca7 cli: util function to parse command-line arguments 9ddd7bc cli: split 'nmcli switch' --> 'nmcli networking' and 'nmcli radio' 7e73354 cli: rename HARDWARE -> HW in status/switch fields 2bafd7a cli: split 'general' into 'nmcli switch' and 'nmcli general' commands 7a8d654 cli: rename 'device list' -> 'device show' and remove 'iface' keyword 51f055f cli: remove iface keyword for 'nmcli device disconnect' 60f54cd cli: 'device disconnect' - ask for interface when missing and '--ask' is present 0db4b4d cli: 'device wifi connect' - ask for SSID/BSSID and password if not provided 9e80133 cli: 'connection delete' - ask for a connection name if not provided 786ba05 cli: 'connection down' - ask for a connection name if not provided 97dbf98 cli: 'connection up' - ask for a connection name if not provided b0bee19 cli: add nmc_string_to_arg_array() to split a string to arguments array 7c629bf cli: nmc_get_user_input() util function for getting user input 981a687 cli: add '--ask' global option abbde8d cli: add 'nmcli device wifi scan' command 831b7e2 cli: move 'nmcli con status' under 'nmcli connection show' as 'active' 245d86b cli: make id|uuid optional for 'nmcli connection down' 6d5a88f cli: make id|uuid optional for 'nmcli connection up' 4eef48d cli: make id|uuid optional for 'nmcli connection delete' 1e106e3 cli: make id|uuid specifiers optional for 'connection list' and 'connection status' 5e4d264 cli: rename 'nm' object to 'general' (first)