[ovs-dev] [PATCH] LLDP: add new command to show LLDP neighbor info
Aaron Conole
aconole at redhat.com
Thu Apr 8 13:08:44 UTC 2021
"Rick Zhong" <winsome8282 at 163.com> writes:
> Hi Ilya,
>
> I took your advice and will submit a new merge request.
Please send an email with a link to the pull request. It's best if you
can use 'git send-email' to send the series to the list instead.
> As to the existing "autoattach" commands, it requires DCBX enabled on peer device. Otherwise, nothing is shown by the
> command.
> That's why I didn't merge into it.
>
> Best regards,
> Rick Zhong
>
> At 2021-04-08 01:55:20, "Ilya Maximets" <i.maximets at ovn.org> wrote:
>>On 4/7/21 7:08 PM, Rick Zhong wrote:
>>> 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.
>>
>>It's part of a C standard starting at least from C89:
>>"""
>>4.10.3.2 The free function
>>...
>>If ptr is a null pointer, no action occurs.
>>"""
>>
>>'man 3 free' suggests the same.
>>
>>>
>>>>> + 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.
>>
>>mutex protects lldp data structures, but there is no need to
>>hold it while sending a simple string to the user, so unlock
>>could be safely moved above the unixctl_command_reply().
>>
>>>
>>>>> +}
>>>>> +
>>>>> /* 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.
>>
>>I'm not confident enough in this area to answer.
>>
>>Aaron, do you have an opinion?
>>
>>OTOH, maybe we can just add missing information to the
>>result of autoattach/status instead of creating a new
>>command?
>>
>>>
>>>>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