> -----Original Message----- > From: David Marchand [mailto:david.march...@redhat.com] > Sent: Wednesday, April 19, 2023 2:44 PM > To: wangyunjian <wangyunj...@huawei.com> > Cc: d...@openvswitch.org; i.maxim...@ovn.org; luyicai <luyi...@huawei.com> > Subject: Re: [ovs-dev] [PATCH] dpif-netlink: Fix memory leak > dpif_netlink_open(). > > On Tue, Apr 18, 2023 at 3:16 PM wangyunjian <wangyunj...@huawei.com> > wrote: > > > I wonder though if we should pass any ofpbuf at all, as we don't > > > care about the reply from the kernel for this case. > > > Something like: > > > > > > $ git diff > > > diff --git a/lib/dpif-netlink.c b/lib/dpif-netlink.c index > > > de07112778..60bd39643c > > > 100644 > > > --- a/lib/dpif-netlink.c > > > +++ b/lib/dpif-netlink.c > > > @@ -395,7 +395,7 @@ dpif_netlink_open(const struct dpif_class *class > > > OVS_UNUSED, const char *name, > > > dp_request.user_features |= OVS_DP_F_UNALIGNED; > > > dp_request.user_features |= OVS_DP_F_VPORT_PIDS; > > > dp_request.user_features |= OVS_DP_F_UNSUPPORTED; > > > - error = dpif_netlink_dp_transact(&dp_request, &dp, &buf); > > > + error = dpif_netlink_dp_transact(&dp_request, NULL, NULL); > > > if (error) { > > > /* The Open vSwitch kernel module has two modes for > dispatching > > > * upcalls: per-vport and per-cpu. > > > > > > WDYT? > > > > Currently, the 'buf' will check whether it is NULL in the > dpif_netlink_dp_transact(). > > And it will be used in dpif_netlink_dp_from_ofpbuf(). > > - Looking at dpif_netlink_dp_transact() comments. > > /* Executes 'request' in the kernel datapath. If the command fails, returns a > * positive errno value. Otherwise, if 'reply' and 'bufp' are null, returns 0 > * without doing anything else. If 'reply' and 'bufp' are nonnull, then the > * result of the command is expected to be of the same form, which is decoded > * and stored in '*reply' and '*bufp'. The caller must free '*bufp' when the > * reply is no longer needed ('reply' will contain pointers into '*bufp'). */ > static > int dpif_netlink_dp_transact(const struct dpif_netlink_dp *request, > struct dpif_netlink_dp *reply, struct ofpbuf > **bufp) > > I understand that dpif_netlink_dp_transact() makes it possible for a caller to > simply ignore the reply from the kernel by passing both 'reply' and 'bufp' as > NULL. > > > - In the specific call to dpif_netlink_dp_transact() (line 398) in > dpif_netlink_open(), the 'dp' content is not considered in the branch when no > error returned (starting line 430). > Besides, 'dp' / 'buf' are overwritten later in this same branch, by sending a > new > netlink request (line 437). > > So if this code is not going to use 'dp' and 'buf' content, why not simply > pass > NULL/NULL?
You are right. I fix it on your suggestions. http://patchwork.ozlabs.org/project/openvswitch/patch/1681890598-18740-1-git-send-email-wangyunj...@huawei.com/ Thank you. Yunjian > > > -- > David Marchand _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
