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" <[email protected]> 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 ([email protected])
>>
>>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.
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
