Thank you Eelco for testing the patch. My responses are inline:
*Vasu Dasari* On Thu, May 20, 2021 at 5:20 AM Eelco Chaudron <[email protected]> wrote: > > > On 14 May 2021, at 21:33, Vasu Dasari wrote: > > > Currently there is an option to add/flush/show ARP/ND neighbor. This > covers L3 > > side. For L2 side, there is only fdb show command. This patch gives an > option > > to add/del an fdb entry via ovs-appctl. ovs-appctl command looks like > this: > > > > To add: > > ovs-appctl fdb/add <bridge> <port> <vlan> <Mac> > > ovs-appctl fdb/add br0 p1 0 50:54:00:00:00:05 > > > > To del: > > ovs-appctl fdb/del <bridge> <port> <vlan> <Mac> > > ovs-appctl fdb/del br0 p1 0 50:54:00:00:00:05 > > > > Static entry should not age. To indicate that entry being programmed is > a static entry, > > 'expires' field in 'struct mac_entry' will be set to a > MAC_ENTRY_AGE_STATIC_ENTRY. A > > check for this value is made while deleting mac entry as part of regular > aging process. > > Another check as part of mac-update process, when a packet with same > source mac as this > > entry arrives on the configured port will not modify the expires field > > > > Added two new APIs to provide convenient interface to add and delete > static-macs. > > void xlate_add_static_mac_entry(const struct ofproto_dpif *, ofp_port_t > in_port, > > struct eth_addr dl_src, int vlan); > > void xlate_delete_static_mac_entry(const struct ofproto_dpif *, > > struct eth_addr dl_src, int vlan); > > > > Signed-off-by: Vasu Dasari <[email protected]> > > Reported-at: > https://mail.openvswitch.org/pipermail/ovs-discuss/2019-June/048894.html > > Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=1597752 > > I did some testing and found the issues below, once fixed, I’ll do a full > code review. When you do an FDB flush, it also clears the static FDB entries. I think > this is wrong, as all hardware vendors I know will retain the static FDB > entries. > I took the liberty of having fdb/flush clear static entries as well. I do not mind changing the code to delete only dynamic ones. Will take care of this. > When you create a static entry for lets say Port A, when a packet with the > same MAC comes from Port B the entry will be updated with Port B. This > should not happen for static entries. > > I agree. I tested this case too. It fails. Will fix it. > When you add a static MAC entry, the command just returns "OK". Other > commands do not return anything on a successful addition. You should either > follow the same behavior or be more verbose (Static FDB successfully > added?) on your return. > > Sure, will make the command return error string only if it fails, otherwise will be silent. > Also, it might be nice to be more verbose when you replace an existing > static or dynamic FDB entry, i.e. especially if the physical port is > different (mac move case). > > Sure, will add more detailed output. > Cheers, > > > Eelco > > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
