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 757307 - [review] nmcli: add command for displaying LLDP neighbors
[review] nmcli: add command for displaying LLDP neighbors
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:
Blocks: nm-review
 
 
Reported: 2015-10-29 13:16 UTC by Beniamino Galvani
Modified: 2015-11-10 13:11 UTC
See Also:
GNOME target: ---
GNOME version: ---

Attachments
[PATCH] cli: add command for displaying LLDP neighbors (13.08 KB, patch)
2015-10-29 13:18 UTC, Beniamino Galvani 		none Details | Review
[PATCH v2] cli: add command for displaying LLDP neighbors (14.39 KB, patch)
2015-11-02 10:21 UTC, Beniamino Galvani 		none Details | Review

Description Beniamino Galvani 2015-10-29 13:16:29 UTC 
The list of LLDP neighbors is available through the D-Bus interface
and libnm already provides functions to retrieve it; make the list
available through nmcli as well.
Comment 1 Beniamino Galvani 2015-10-29 13:18:43 UTC 
Created attachment 314395 [details] [review]
[PATCH] cli: add command for displaying LLDP neighbors
Comment 2 Jiri Klimes 2015-10-30 13:38:09 UTC 
The patch looks good to me.

I just wonder whether the is a plan to have additional commands except 'list', in the future.
And maybe the command could support multiple device arguments?
Comment 3 Jiri Klimes 2015-10-30 13:56:27 UTC 
+ nmc->print_fields.header_name = (char *) construct_header_name (_("Device LLDP neighbors"),
                                                                  nm_device_get_iface (device));
+ nmc->print_fields.indices = parse_output_fields (fields_str, tmpl, FALSE, NULL, NULL);

just spotted that parse_output_fields() should be passed an error and checked that. Because it parses fields_str from user. See e.g. show_nm_status() in general.c.
Comment 4 Beniamino Galvani 2015-11-02 10:21:45 UTC 
Created attachment 314615 [details] [review]
[PATCH v2] cli: add command for displaying LLDP neighbors

(In reply to Jiri Klimes from comment #2)
> I just wonder whether the is a plan to have additional commands except
> 'list', in the future.
> And maybe the command could support multiple device arguments?

I don't know, probably not. Now I've changed the command syntax to be
similar to the 'wifi list' command:

  lldp [list [ifname <ifname>]]

so that one can simply issue a 'nmcli d lldp' to get the list for all
devices, and we still can add other commands in the future. About
allowing multiple 'ifname' options, I would keep it simple for now and
add this later if needed.

(In reply to Jiri Klimes from comment #3)
> just spotted that parse_output_fields() should be passed an error and
> checked that. Because it parses fields_str from user. See e.g.
> show_nm_status() in general.c.

Fixed, thanks.
Comment 5 Thomas Haller 2015-11-03 13:45:49 UTC 
Missing bash completion.

rest LGTM
Comment 6 Beniamino Galvani 2015-11-04 09:05:20 UTC 
Added bash completion and repushed.
Comment 7 Jiri Klimes 2015-11-10 09:11:33 UTC 
> bg/lldp-cli
Pushed a fixup to the branch for
$nmcli dev lldp list blabla
Unknown parameter: blabla
Success

Other than that, the branch looks good to me.