Looking at this patch, I found a potential issue with memory management and have some style questions.
git sha of the commit: 50340f8db18db19e4434c7cd45ca8b3c601c5f51 Author: Eelco Chaudron <[email protected]> One line subject of the commit: dpif-offload: Add provider for tc offload. This patch adds the tc provider framework by implementing a basic TC offload provider with open/close operations. It registers the provider for system type datapaths on Linux and adds test coverage for the offload show command. > diff --git a/lib/automake.mk b/lib/automake.mk > index 314102ecc..6e015ebfc 100644 > --- a/lib/automake.mk > +++ b/lib/automake.mk > @@ -475,6 +475,7 @@ lib_libopenvswitch_la_SOURCES += \ > lib/dpif-netlink.h \ > lib/dpif-netlink-rtnl.c \ > lib/dpif-netlink-rtnl.h \ > + lib/dpif-offload-tc.c \ > lib/if-notifier.c \ > lib/netdev-linux.c \ > lib/netdev-linux.h \ [ ... ] > diff --git a/lib/dpif-offload-tc.c b/lib/dpif-offload-tc.c > new file mode 100644 > index 000000000..f5d62566e > --- /dev/null > +++ b/lib/dpif-offload-tc.c > @@ -0,0 +1,47 @@ [ ... ] > +static int > +dpif_offload_tc_open(const struct dpif_offload_class *offload_class, > + struct dpif *dpif, struct dpif_offload **dpif_offload) > +{ > + struct dpif_offload *offload = xmalloc(sizeof(struct dpif_offload)); > + > + dpif_offload_init(offload, offload_class, dpif); > + *dpif_offload = offload; > + return 0; > +} Can dpif_offload_init() fail and potentially leave the allocated memory orphaned? Looking at the success path returning 0, if dpif_offload_init() could fail internally but not return an error, this could leak the offload structure. > +static void > +dpif_offload_tc_close(struct dpif_offload *dpif_offload) > +{ > + free(dpif_offload); > +} Should this function call any cleanup on the dpif_offload structure before freeing it? Does dpif_offload_init() allocate any resources that need to be released? [ ... ] > diff --git a/tests/system-offloads-traffic.at > b/tests/system-offloads-traffic.at > index f70d02926..88ee6238d 100644 > --- a/tests/system-offloads-traffic.at > +++ b/tests/system-offloads-traffic.at [ ... ] > +AT_SETUP([offloads - ovs-appctl dpif/offload/show - offloads enabled]) > +AT_KEYWORDS([dpif-offload]) > +OVS_TRAFFIC_VSWITCHD_START([], [], > + [-- set Open_vSwitch . other_config:hw-offload=true]) > + > +AT_CHECK([ovs-appctl dpif/offload/show], [0], [dnl > +system@ovs-system: > + tc > +]) Is the indentation correct here? The test shows two spaces before "tc" but should this follow the project's standard indentation pattern? > +AT_CHECK([ovs-appctl --format json --pretty dpif/offload/show], [0], [dnl > +{ > + "system@ovs-system": { > + "providers": [[ > + "tc"]]}} > +]) The JSON output shows nested arrays with "providers": [["tc"]]. Should providers be a simple array rather than an array of arrays? _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
