[ovs-dev] [PATCH] LLDP: add new command to show LLDP neighbor info
Rick Zhong
winsome8282 at 163.com
Wed Apr 7 17:08:14 UTC 2021
Hi Ilya,
Many thanks for your attention on this and reply. Please see my comments inline.
And I will try the 'git' commands as you mentioned.
Best regards,
Rick Zhong
At 2021-04-07 20:21:19, "Ilya Maximets" <i.maximets at ovn.org> wrote:
>On 3/24/21 4:56 AM, Rick Zhong wrote:
>> Dear OVS reviewers/supervisors:
>>
>>
>> This patch is related to LLDP which provides a new command "ovs-appctl lldp/neighbor" to show LLDP neighbor info when LLDP is enabled on OVS interfaces.
>>
>>
>> With this new command, user is enable to get LLDP neighbor info even if not in SPB network.
>>
>>
>> One limitation is that when multiple peer Management IP addresses are found by LLDP, only one Management IP address is displayed by the command.
>>
>>
>> The patch is well-tested on Linux.
>>
>>
>> Related commit: af4b3d3 and e4bc70c (add new command to show LLDP neighbor info #349)
>>
>>
>> Signed-off-by: Rick Zhong (winsome8282 at 163.com)
>
>Hi. Thanks for working on this!
>The patch format is a bit unusual, you might want to consider using
>'git format-patch' and 'git send-email' to send patches to the mail-list.
>
>Aaron, could you take a look at this change from the LLDP perspective?
>
>Few comments inline.
>
>>
>>
>> =================================================================================
>> diff --git a/lib/ovs-lldp.c b/lib/ovs-lldp.c
>> index 05c1dd434..4c8ab9126 100644
>> --- a/lib/ovs-lldp.c
>> +++ b/lib/ovs-lldp.c
>> @@ -324,6 +324,84 @@ aa_print_isid_status(struct ds *ds, struct lldp *lldp) OVS_REQUIRES(mutex)
>> }
>> }
>>
>> +static void
>> +lldp_print_neighbor_port(struct ds *ds, struct lldpd_hardware *hw)
>> +{
>> + struct lldpd_port *port;
>> +
>> + LIST_FOR_EACH (port, p_entries, &hw->h_rports) {
>> + const char *none_str = "";
>
>Can we use "<None>" here like other commands does?
[Rick] Sure. No problem.
>> + char *id = NULL;
>> + const char *name = NULL;
>> + const char *port_id = NULL;
>> + char ipaddress[INET6_ADDRSTRLEN + 1];
>> + memset(ipaddress, 0, INET6_ADDRSTRLEN + 1);
>> +
>> + if (port->p_chassis) {
>> + if (port->p_chassis->c_id_len > 0) {
>> + chassisid_to_string(port->p_chassis->c_id,
>> + port->p_chassis->c_id_len, &id);
>> + }
>> +
>> + name = port->p_chassis->c_name;
>> +
>> + struct lldpd_mgmt *mgmt;
>> + LIST_FOR_EACH (mgmt, m_entries, &port->p_chassis->c_mgmt) {
>> + int af;
>> + size_t alen;
>> + switch (mgmt->m_family) {
>> + case LLDPD_AF_IPV4:
>> + alen = INET_ADDRSTRLEN + 1;
>> + af = AF_INET;
>> + break;
>> + case LLDPD_AF_IPV6:
>> + alen = INET6_ADDRSTRLEN + 1;
>> + af = AF_INET6;
>> + break;
>> + default:
>> + continue;
>> + }
>> +
>> + if (inet_ntop(af, &mgmt->m_addr, ipaddress, alen) == NULL) {
>> + continue;
>> + }
>> + break;
>
>OVS already has some formatting functions that converts ip addresses
>to strings, so it's better to use them. For this particular case,
>I think, we can use some thing like this:
>
> struct in6_addr ip = in6addr_any;
> ...
>
> LIST_FOR_EACH (mgmt, m_entries, &port->p_chassis->c_mgmt) {
> switch (mgmt->m_family) {
> case LLDPD_AF_IPV4:
> in6_addr_set_mapped_ipv4(&ip, &mgmt->m_addr.inet);
> break;
> case LLDPD_AF_IPV6:
> ip = mgmt->m_addr.inet6;
> break;
> default:
> continue;
> }
> }
>
> ...
> ds_put_cstr(ds, " Neighbor Management IP: ");
> ipv6_format_mapped(&ip, ds);
> ds_put_char(ds, "\n");
[Rick] Thanks for your example. Actually this part of IP conversion codes were copied from another function somewhere:-) I will try your codes and make a test.
>> + }
>> + }
>> +
>> + port_id = port->p_id;
>This copy seems unnecessary.
[Rick] Ok. I will remove it.
>> +
>> + ds_put_format(ds, " Neighbor Chassis ID: %s\n",
>> + id ? id : none_str);
>> + ds_put_format(ds, " Neighbor Chassis SysName: %s\n",
>> + name ? name : none_str);
>> + ds_put_format(ds, " Neighbor Management IP: %s\n",
>> + ipaddress);
>> + ds_put_format(ds, " Neighbor Port ID: %s\n",
>> + port_id ? port_id : none_str);
>> +
>> + if (id != NULL) {
>
>It's safe to call free(NULL), so , please, don't check.
[Rick] Does it mean that we override the original free() method, so that it won't crash when we call free(NULL)? If so, that is good and I don't need to check here.
>> + free(id);
>> + }
>> + }
>> +}
>> +
>> +static void
>> +lldp_print_neighbor(struct ds *ds, struct lldp *lldp) OVS_REQUIRES(mutex)
>> +{
>> + struct lldpd_hardware *hw;
>> +
>> + ds_put_format(ds, "LLDP: %s\n", lldp->name);
>> +
>> + if (!lldp->lldpd) {
>> + return;
>> + }
>> +
>> + LIST_FOR_EACH (hw, h_entries, &lldp->lldpd->g_hardware) {
>> + lldp_print_neighbor_port(ds, hw);
>> + }
>> +}
>> +
>> static void
>> aa_unixctl_status(struct unixctl_conn *conn, int argc OVS_UNUSED,
>> const char *argv[] OVS_UNUSED, void *aux OVS_UNUSED)
>> @@ -382,6 +460,25 @@ aa_unixctl_statistics(struct unixctl_conn *conn, int argc OVS_UNUSED,
>> unixctl_command_reply(conn, ds_cstr(&ds));
>> }
>>
>> +static void
>> +lldp_unixctl_show_neighbor(struct unixctl_conn *conn, int argc OVS_UNUSED,
>> + const char *argv[] OVS_UNUSED, void *aux OVS_UNUSED)
>> + OVS_EXCLUDED(mutex)
>> +{
>> + struct lldp *lldp;
>> + struct ds ds = DS_EMPTY_INITIALIZER;
>> +
>> + ovs_mutex_lock(&mutex);
>> +
>> + HMAP_FOR_EACH (lldp, hmap_node, all_lldps) {
>> + lldp_print_neighbor(&ds, lldp);
>> + }
>> + unixctl_command_reply(conn, ds_cstr(&ds));
>> + ds_destroy(&ds);
>> +
>> + ovs_mutex_unlock(&mutex);
>
>It's, probbaly, better to unlock before sending reply.
[Rick] Umm, sorry, this function is almost 100% copied from another similiar command. I'm not quite confident there is no problem to move the unlock ahead.
>> +}
>> +
>> /* An Auto Attach mapping was configured. Populate the corresponding
>> * structures in the LLDP hardware.
>> */
>> @@ -649,6 +746,8 @@ lldp_init(void)
>> aa_unixctl_show_isid, NULL);
>> unixctl_command_register("autoattach/statistics", "[bridge]", 0, 1,
>> aa_unixctl_statistics, NULL);
>> + unixctl_command_register("lldp/neighbor", "[bridge]", 0, 1,
>> + lldp_unixctl_show_neighbor, NULL);
>
>All other functions named 'autoattach' and has 'aa_' prefix instead
>of 'lldp_', so it's better, I think, to have similar name for the
>new command.
[Rick] Yes, you are right. All the other commands in this file are with 'aa_' prefix. In my option, the new LLDP command can work independently.
However, the 'autoattach' commands should work only in the DCBX scenario. That's why I made a different prefix.
Anyway, I'm open to hear your advice.
>Also, it seems like none of these commands documented anywhere.
>We will need to fix that someday.
>
>Best regards, Ilya Maximets.
More information about the dev
mailing list