On 20 May 2021, at 19:09, Vasu Dasari wrote:
> Thank you Eelco for testing the patch. > > My responses are inline: Thanks, looking forward for your next revision. Would be good if you can add a test case for the mac move and flush change. //Eelco > *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
