Hi Eelco, I addressed your comments and also there was a bug uncovered with unit tests.
Here is the PATCH v4 <http://patchwork.ozlabs.org/project/openvswitch/patch/20210522181211.48976-1-vdas...@gmail.com/> Thanks *Vasu Dasari* On Fri, May 21, 2021 at 4:41 AM Eelco Chaudron <echau...@redhat.com> wrote: > > > 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 <echau...@redhat.com> > 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 <vdas...@gmail.com> > >>> 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 d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
