Hi Ilya,
I took your advice and will submit a new merge request. 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" <[email protected]> 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" <[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
