| Message ID | 20170404203157.12923-1-e@erig.me |
|---|---|
| State | Superseded |
| Headers | show |
On 4 April 2017 at 13:31, Eric Garver <e@erig.me> wrote: > Furthermore, the return code of dpif_netlink_port_query__() was not > being checked. > > Signed-off-by: Eric Garver <e@erig.me> > --- For context, yes this adds the call to the Linux path as well but this is already in the proposed rtnetlink patch series so I'm not concerned about this. Here's my Ack. I'd like some windows folk to be aware and take a look too though: Acked-by: Joe Stringer <joe@ovn.org> > lib/dpif-netlink.c | 22 ++++++++++++++-------- > 1 file changed, 14 insertions(+), 8 deletions(-) > > diff --git a/lib/dpif-netlink.c b/lib/dpif-netlink.c > index bad575c9cf65..860df6013827 100644 > --- a/lib/dpif-netlink.c > +++ b/lib/dpif-netlink.c > @@ -60,9 +60,6 @@ VLOG_DEFINE_THIS_MODULE(dpif_netlink); > #ifdef _WIN32 > #include "wmi.h" > enum { WINDOWS = 1 }; > -static int dpif_netlink_port_query__(const struct dpif_netlink *dpif, > - odp_port_t port_no, const char *port_name, > - struct dpif_port *dpif_port); > #else > enum { WINDOWS = 0 }; > #endif > @@ -224,6 +221,9 @@ static void dpif_netlink_vport_to_ofpbuf(const struct dpif_netlink_vport *, > struct ofpbuf *); > static int dpif_netlink_vport_from_ofpbuf(struct dpif_netlink_vport *, > const struct ofpbuf *); > +static int dpif_netlink_port_query__(const struct dpif_netlink *dpif, > + odp_port_t port_no, const char *port_name, > + struct dpif_port *dpif_port); > > static struct dpif_netlink * > dpif_netlink_cast(const struct dpif *dpif) > @@ -948,19 +948,23 @@ dpif_netlink_port_del__(struct dpif_netlink *dpif, odp_port_t port_no) > OVS_REQ_WRLOCK(dpif->upcall_lock) > { > struct dpif_netlink_vport vport; > + struct dpif_port dpif_port; > int error; > > + error = dpif_netlink_port_query__(dpif, port_no, NULL, &dpif_port); > + if (error) { > + return error; > + } > + > dpif_netlink_vport_init(&vport); > vport.cmd = OVS_VPORT_CMD_DEL; > vport.dp_ifindex = dpif->dp_ifindex; > vport.port_no = port_no; > #ifdef _WIN32 > - struct dpif_port temp_dpif_port; > - dpif_netlink_port_query__(dpif, port_no, NULL, &temp_dpif_port); > - if (!strcmp(temp_dpif_port.type, "internal")) { > - if (!delete_wmi_port(temp_dpif_port.name)){ > + if (!strcmp(dpif_port.type, "internal")) { > + if (!delete_wmi_port(dpif_port.name)){ > VLOG_ERR("Could not delete wmi port with name: %s", > - temp_dpif_port.name); > + dpif_port.name); > }; > } > #endif > @@ -968,6 +972,8 @@ dpif_netlink_port_del__(struct dpif_netlink *dpif, odp_port_t port_no) > > vport_del_channels(dpif, port_no); > > + dpif_port_destroy(&dpif_port); > + > return error; > } > > -- > 2.9.3 > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Do any of the windows folks have any feedback on the below patch? Thanks. Eric. On Tue, Apr 04, 2017 at 03:31:29PM -0700, Joe Stringer wrote: > On 4 April 2017 at 13:31, Eric Garver <e@erig.me> wrote: > > Furthermore, the return code of dpif_netlink_port_query__() was not > > being checked. > > > > Signed-off-by: Eric Garver <e@erig.me> > > --- > > For context, yes this adds the call to the Linux path as well but this > is already in the proposed rtnetlink patch series so I'm not concerned > about this. > > Here's my Ack. I'd like some windows folk to be aware and take a look > too though: > Acked-by: Joe Stringer <joe@ovn.org> > > > lib/dpif-netlink.c | 22 ++++++++++++++-------- > > 1 file changed, 14 insertions(+), 8 deletions(-) > > > > diff --git a/lib/dpif-netlink.c b/lib/dpif-netlink.c > > index bad575c9cf65..860df6013827 100644 > > --- a/lib/dpif-netlink.c > > +++ b/lib/dpif-netlink.c > > @@ -60,9 +60,6 @@ VLOG_DEFINE_THIS_MODULE(dpif_netlink); > > #ifdef _WIN32 > > #include "wmi.h" > > enum { WINDOWS = 1 }; > > -static int dpif_netlink_port_query__(const struct dpif_netlink *dpif, > > - odp_port_t port_no, const char *port_name, > > - struct dpif_port *dpif_port); > > #else > > enum { WINDOWS = 0 }; > > #endif > > @@ -224,6 +221,9 @@ static void dpif_netlink_vport_to_ofpbuf(const struct dpif_netlink_vport *, > > struct ofpbuf *); > > static int dpif_netlink_vport_from_ofpbuf(struct dpif_netlink_vport *, > > const struct ofpbuf *); > > +static int dpif_netlink_port_query__(const struct dpif_netlink *dpif, > > + odp_port_t port_no, const char *port_name, > > + struct dpif_port *dpif_port); > > > > static struct dpif_netlink * > > dpif_netlink_cast(const struct dpif *dpif) > > @@ -948,19 +948,23 @@ dpif_netlink_port_del__(struct dpif_netlink *dpif, odp_port_t port_no) > > OVS_REQ_WRLOCK(dpif->upcall_lock) > > { > > struct dpif_netlink_vport vport; > > + struct dpif_port dpif_port; > > int error; > > > > + error = dpif_netlink_port_query__(dpif, port_no, NULL, &dpif_port); > > + if (error) { > > + return error; > > + } > > + > > dpif_netlink_vport_init(&vport); > > vport.cmd = OVS_VPORT_CMD_DEL; > > vport.dp_ifindex = dpif->dp_ifindex; > > vport.port_no = port_no; > > #ifdef _WIN32 > > - struct dpif_port temp_dpif_port; > > - dpif_netlink_port_query__(dpif, port_no, NULL, &temp_dpif_port); > > - if (!strcmp(temp_dpif_port.type, "internal")) { > > - if (!delete_wmi_port(temp_dpif_port.name)){ > > + if (!strcmp(dpif_port.type, "internal")) { > > + if (!delete_wmi_port(dpif_port.name)){ > > VLOG_ERR("Could not delete wmi port with name: %s", > > - temp_dpif_port.name); > > + dpif_port.name); > > }; > > } > > #endif > > @@ -968,6 +972,8 @@ dpif_netlink_port_del__(struct dpif_netlink *dpif, odp_port_t port_no) > > > > vport_del_channels(dpif, port_no); > > > > + dpif_port_destroy(&dpif_port); > > + > > return error; > > } > > > > -- > > 2.9.3 > > > > _______________________________________________ > > dev mailing list > > dev@openvswitch.org > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Thanks a lot for fixing the leak Eric! We should apply on 2.7 as well, but needs a rebase. Is it okay if we add the function call on 2.7 as well or should we limit it too WIN32? Acked-by: Alin Gabriel Serdean <aserdean@cloudbasesolutions.com> > -----Original Message----- > From: ovs-dev-bounces@openvswitch.org [mailto:ovs-dev- > bounces@openvswitch.org] On Behalf Of Eric Garver > Sent: Tuesday, April 11, 2017 4:15 PM > To: ovs dev <dev@openvswitch.org> > Subject: Re: [ovs-dev] [PATCH] dpif-netlink: fix memory leak in > dpif_netlink_port_del__() on WIN32 > > Do any of the windows folks have any feediback on the below patch? > > Thanks. > Eric. > > On Tue, Apr 04, 2017 at 03:31:29PM -0700, Joe Stringer wrote: > > On 4 April 2017 at 13:31, Eric Garver <e@erig.me> wrote: > > > Furthermore, the return code of dpif_netlink_port_query__() was not > > > being checked. > > > > > > Signed-off-by: Eric Garver <e@erig.me> > > > --- > > > > For context, yes this adds the call to the Linux path as well but this > > is already in the proposed rtnetlink patch series so I'm not concerned > > about this. > > > > Here's my Ack. I'd like some windows folk to be aware and take a look > > too though: > > Acked-by: Joe Stringer <joe@ovn.org>
On Wed, Apr 12, 2017 at 04:12:51PM +0000, Alin Serdean wrote: > Thanks a lot for fixing the leak Eric! > > We should apply on 2.7 as well, but needs a rebase. > > Is it okay if we add the function call on 2.7 as well or should we limit it too WIN32? I don't think it would hurt anything. If others disagree I can submit a 2.7 only patch. Joe, Do you have an opinion? > Acked-by: Alin Gabriel Serdean <aserdean@cloudbasesolutions.com> > > > -----Original Message----- > > From: ovs-dev-bounces@openvswitch.org [mailto:ovs-dev- > > bounces@openvswitch.org] On Behalf Of Eric Garver > > Sent: Tuesday, April 11, 2017 4:15 PM > > To: ovs dev <dev@openvswitch.org> > > Subject: Re: [ovs-dev] [PATCH] dpif-netlink: fix memory leak in > > dpif_netlink_port_del__() on WIN32 > > > > Do any of the windows folks have any feediback on the below patch? > > > > Thanks. > > Eric. > > > > On Tue, Apr 04, 2017 at 03:31:29PM -0700, Joe Stringer wrote: > > > On 4 April 2017 at 13:31, Eric Garver <e@erig.me> wrote: > > > > Furthermore, the return code of dpif_netlink_port_query__() was not > > > > being checked. > > > > > > > > Signed-off-by: Eric Garver <e@erig.me> > > > > --- > > > > > > For context, yes this adds the call to the Linux path as well but this > > > is already in the proposed rtnetlink patch series so I'm not concerned > > > about this. > > > > > > Here's my Ack. I'd like some windows folk to be aware and take a look > > > too though: > > > Acked-by: Joe Stringer <joe@ovn.org> > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
On 12 April 2017 at 12:43, Eric Garver <e@erig.me> wrote: > On Wed, Apr 12, 2017 at 04:12:51PM +0000, Alin Serdean wrote: >> Thanks a lot for fixing the leak Eric! >> >> We should apply on 2.7 as well, but needs a rebase. >> >> Is it okay if we add the function call on 2.7 as well or should we limit it too WIN32? > > I don't think it would hurt anything. If others disagree I can submit a > 2.7 only patch. > > Joe, > Do you have an opinion? Hi Eric, I think that if you split this patch up and simplified it then it would make it easier to review, apply, and backport. Really the refactor belongs with the rtnetlink series. Since I was already looking at this, I went ahead and did it: https://mail.openvswitch.org/pipermail/ovs-dev/2017-April/330743.html https://mail.openvswitch.org/pipermail/ovs-dev/2017-April/330741.html https://mail.openvswitch.org/pipermail/ovs-dev/2017-April/330742.html I'll apply the first two to master and branch-2.7 shortly. Feel free to take the third patch into your rtnetlink series when you resubmit. Cheers, Joe
On Wed, Apr 12, 2017 at 01:21:30PM -0700, Joe Stringer wrote: > On 12 April 2017 at 12:43, Eric Garver <e@erig.me> wrote: > > On Wed, Apr 12, 2017 at 04:12:51PM +0000, Alin Serdean wrote: > >> Thanks a lot for fixing the leak Eric! > >> > >> We should apply on 2.7 as well, but needs a rebase. > >> > >> Is it okay if we add the function call on 2.7 as well or should we limit it too WIN32? > > > > I don't think it would hurt anything. If others disagree I can submit a > > 2.7 only patch. > > > > Joe, > > Do you have an opinion? > > Hi Eric, > > I think that if you split this patch up and simplified it then it > would make it easier to review, apply, and backport. Really the > refactor belongs with the rtnetlink series. You're right. > > Since I was already looking at this, I went ahead and did it: > https://mail.openvswitch.org/pipermail/ovs-dev/2017-April/330743.html > https://mail.openvswitch.org/pipermail/ovs-dev/2017-April/330741.html > https://mail.openvswitch.org/pipermail/ovs-dev/2017-April/330742.html Great. > > I'll apply the first two to master and branch-2.7 shortly. Feel free > to take the third patch into your rtnetlink series when you resubmit. Sounds like a plan. Thanks.
diff --git a/lib/dpif-netlink.c b/lib/dpif-netlink.c index bad575c9cf65..860df6013827 100644 --- a/lib/dpif-netlink.c +++ b/lib/dpif-netlink.c @@ -60,9 +60,6 @@ VLOG_DEFINE_THIS_MODULE(dpif_netlink); #ifdef _WIN32 #include "wmi.h" enum { WINDOWS = 1 }; -static int dpif_netlink_port_query__(const struct dpif_netlink *dpif, - odp_port_t port_no, const char *port_name, - struct dpif_port *dpif_port); #else enum { WINDOWS = 0 }; #endif @@ -224,6 +221,9 @@ static void dpif_netlink_vport_to_ofpbuf(const struct dpif_netlink_vport *, struct ofpbuf *); static int dpif_netlink_vport_from_ofpbuf(struct dpif_netlink_vport *, const struct ofpbuf *); +static int dpif_netlink_port_query__(const struct dpif_netlink *dpif, + odp_port_t port_no, const char *port_name, + struct dpif_port *dpif_port); static struct dpif_netlink * dpif_netlink_cast(const struct dpif *dpif) @@ -948,19 +948,23 @@ dpif_netlink_port_del__(struct dpif_netlink *dpif, odp_port_t port_no) OVS_REQ_WRLOCK(dpif->upcall_lock) { struct dpif_netlink_vport vport; + struct dpif_port dpif_port; int error; + error = dpif_netlink_port_query__(dpif, port_no, NULL, &dpif_port); + if (error) { + return error; + } + dpif_netlink_vport_init(&vport); vport.cmd = OVS_VPORT_CMD_DEL; vport.dp_ifindex = dpif->dp_ifindex; vport.port_no = port_no; #ifdef _WIN32 - struct dpif_port temp_dpif_port; - dpif_netlink_port_query__(dpif, port_no, NULL, &temp_dpif_port); - if (!strcmp(temp_dpif_port.type, "internal")) { - if (!delete_wmi_port(temp_dpif_port.name)){ + if (!strcmp(dpif_port.type, "internal")) { + if (!delete_wmi_port(dpif_port.name)){ VLOG_ERR("Could not delete wmi port with name: %s", - temp_dpif_port.name); + dpif_port.name); }; } #endif @@ -968,6 +972,8 @@ dpif_netlink_port_del__(struct dpif_netlink *dpif, odp_port_t port_no) vport_del_channels(dpif, port_no); + dpif_port_destroy(&dpif_port); + return error; }
Furthermore, the return code of dpif_netlink_port_query__() was not being checked. Signed-off-by: Eric Garver <e@erig.me> --- lib/dpif-netlink.c | 22 ++++++++++++++-------- 1 file changed, 14 insertions(+), 8 deletions(-)