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