| Message ID | 4cf2cc0.2b08.17862605d2c.Coremail.winsome8282@163.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [ovs-dev] LLDP: add new command to show LLDP neighbor info | expand |
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@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? > + 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"); > + } > + } > + > + port_id = port->p_id; This copy seems unnecessary. > + > + 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. > + 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. > +} > + > /* 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. Also, it seems like none of these commands documented anywhere. We will need to fix that someday. Best regards, Ilya Maximets.
On Wed, Apr 07, 2021 at 02:21:19PM +0200, Ilya Maximets wrote: > Also, it seems like none of these commands documented anywhere. > We will need to fix that someday. I'd recommend requiring documentation for anything being added, so the situation doesn't get worse. We usually document this kind of command in ovs-vswitchd(8), when we remember.
On 4/7/21 4:40 PM, Ben Pfaff wrote: > On Wed, Apr 07, 2021 at 02:21:19PM +0200, Ilya Maximets wrote: >> Also, it seems like none of these commands documented anywhere. >> We will need to fix that someday. > > I'd recommend requiring documentation for anything being added, so the > situation doesn't get worse. Good point. > > We usually document this kind of command in ovs-vswitchd(8), when we > remember. > Rick, could you, please, add a short description of your new command to 'RUNTIME MANAGEMENT COMMANDS' section of vswitchd/ovs-vswitchd.8.in? Best regards, Ilya Maximets.
Hi Ilya and Ben, Actually I was thinking how to make documentation for such kind of user commands change. Sure, I will provide it as you mentioned. Best regards, Rick Zhong At 2021-04-07 23:11:00, "Ilya Maximets" <i.maximets@ovn.org> wrote: >On 4/7/21 4:40 PM, Ben Pfaff wrote: >> On Wed, Apr 07, 2021 at 02:21:19PM +0200, Ilya Maximets wrote: >>> Also, it seems like none of these commands documented anywhere. >>> We will need to fix that someday. >> >> I'd recommend requiring documentation for anything being added, so the >> situation doesn't get worse. > >Good point. > >> >> We usually document this kind of command in ovs-vswitchd(8), when we >> remember. >> > >Rick, could you, please, add a short description of your new command >to 'RUNTIME MANAGEMENT COMMANDS' section of vswitchd/ovs-vswitchd.8.in? > >Best regards, Ilya Maximets.
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@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@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. >> + 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.
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@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@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.
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" <i.maximets@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@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@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.
"Rick Zhong" <winsome8282@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@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@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@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.
> >>> + 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. It's also part of the coding style document (more for Rick than for Ilya): Functions that destroy an instance of a dynamically-allocated type should accept and ignore a null pointer argument. Code that calls such a function (including the C standard library function ``free()``) should omit a null-pointer check. We find that this usually makes code easier to read.
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 = ""; + 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; + } + } + + port_id = port->p_id; + + 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) { + 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); +} + /* 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); } /* Returns true if 'lldp' should process packets from 'flow'. Sets
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@163.com) ================================================================================= ================================================================================= Regards, Rick Zhong