"Rick Zhong" <[email protected]> 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" <[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
