After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 697169 - [review] jklimes/cli-syntax: changes to nmcli without 'connection addition' feature
[review] jklimes/cli-syntax: changes to nmcli without 'connection addition' f...
Status: RESOLVED FIXED
Product: NetworkManager
Classification: Platform
Component: nmcli
git master
Other Linux
: Normal normal
: ---
Assigned To: NetworkManager maintainer(s)
NetworkManager maintainer(s)
Depends on: tracker-nmcli
Blocks:
 
 
Reported: 2013-04-03 11:53 UTC by Jiri Klimes
Modified: 2013-06-04 12:51 UTC
See Also:
GNOME target: ---
GNOME version: ---



Description Jiri Klimes 2013-04-03 11:53:31 UTC
Please review jklimes/cli-syntax branch containing changes to nmcli.
Comment 1 Jiri Klimes 2013-04-03 15:13:24 UTC
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.
Comment 2 Dan Winship 2013-04-03 17:15:13 UTC
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.
Comment 3 Jiri Klimes 2013-04-04 10:33:29 UTC
(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?
Comment 4 Dan Williams 2013-04-04 21:24:10 UTC
> 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.
Comment 5 Dan Williams 2013-04-05 03:27:14 UTC
> 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')
Comment 6 Dan Williams 2013-04-05 03:33:15 UTC
> 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.
Comment 7 Dan Williams 2013-04-05 03:40:06 UTC
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...
Comment 8 Dan Williams 2013-04-05 04:05:45 UTC
> 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.
Comment 9 Jiri Klimes 2013-04-05 11:23:24 UTC
(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.
Comment 10 Jiri Klimes 2013-04-05 11:30:06 UTC
(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'.
Comment 11 Jiri Klimes 2013-04-05 11:31:47 UTC
(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.
Comment 12 Jiri Klimes 2013-04-05 11:35:20 UTC
(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 ;-)
Comment 13 Jiri Klimes 2013-04-05 11:39:39 UTC
(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.
Comment 14 Dan Williams 2013-04-05 21:07:07 UTC
(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!
Comment 15 Dan Williams 2013-04-05 21:22:24 UTC
(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): "));
Comment 16 Dan Williams 2013-04-05 21:24:05 UTC
(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.
Comment 17 Dan Williams 2013-04-05 21:27:05 UTC
(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.
Comment 18 Dan Winship 2013-04-06 14:28:51 UTC
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.
Comment 19 Jiri Klimes 2013-04-08 10:24:55 UTC
(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
Comment 20 Dan Williams 2013-04-08 21:15:14 UTC
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.
Comment 21 Jiri Klimes 2013-04-09 08:41:47 UTC
(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.
Comment 22 Dan Winship 2013-05-02 15:59:57 UTC
NM bugzilla reorganization... sorry for the bug spam.
Comment 23 Jiri Klimes 2013-06-04 12:51:58 UTC
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)