On 12/24/25 8:41 AM, chandler wrote: > prior to the patch, the last monitor vconn copied to the deleted one, > causing the monitor fd to be leaked, the patch fixes it.
Hi, chandler. Thanks for the patch! And sorry for delay, it's been a long holiday shutdown and then soft freeze as usual. The issue appears to be in the code from the beginning of this repo. Likely because OpenFlow snooping is not a very frequently used feature. > Signed-off-by: chandler <[email protected]> The Developer's Certificate of Origin normally requires a full name to be provided. General format as stated in the contribution guide is: Signed-off-by: Firstname Lastname <[email protected]> Exact formatting is not very important, but it shouldn't be just a first name or a pseudonym. I see you previously sent emails with a signature 'Chandler Wu', which will be sufficient for the DCO. Note: the tag should match the From: field, which is the Author: field of the commit. > --- > lib/rconn.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/lib/rconn.c b/lib/rconn.c > index 4afa21515..3815cfed5 100644 > --- a/lib/rconn.c > +++ b/lib/rconn.c > @@ -1285,6 +1285,7 @@ close_monitor(struct rconn *rc, size_t idx, int retval) > VLOG_DBG("%s: closing monitor connection to %s: %s", > rconn_get_name(rc), vconn_get_name(rc->monitors[idx]), > ovs_retval_to_string(retval)); > + vconn_close(rc->monitors[idx]); > rc->monitors[idx] = rc->monitors[--rc->n_monitors]; > } > Would be good to have a test case covering the disconnection of the snooping client. I played around with it for a bit and came up with the following change that makes tests fail if run under address sanitizer if the fix is not applied: diff --git a/tests/ovs-ofctl.at b/tests/ovs-ofctl.at index c82af037e..751a934e4 100644 --- a/tests/ovs-ofctl.at +++ b/tests/ovs-ofctl.at @@ -3561,8 +3561,10 @@ dnl setup controller for br0 before starting the controller AT_CHECK([ovs-vsctl -vsyslog:off set-controller br0 unix:testcontroller]) dnl then start listening on the '.snoop' connection -on_exit 'kill `cat ovs-ofctl-snoop.pid`' -AT_CHECK([ovs-ofctl -vsyslog:off --detach --no-chdir --pidfile=ovs-ofctl-snoop.pid snoop br0 > snoopbr0.txt 2>&1]) +on_exit 'test -e ovs-ofctl-snoop.pid && kill `cat ovs-ofctl-snoop.pid`' +AT_CHECK([ovs-ofctl -vsyslog:off --detach --no-chdir \ + --unixctl=snoop.ctl --pidfile=ovs-ofctl-snoop.pid \ + snoop br0 > snoopbr0.txt 2>&1]) dnl finally start the controller on_exit 'kill `cat ovs-testcontroller.pid`' @@ -3574,6 +3576,9 @@ OVS_WAIT_UNTIL([grep -E "OFPT_FEATURES_REQUEST" snoopbr0.txt >/dev/null 2>&1]) OVS_WAIT_UNTIL([grep -E "OFPT_FEATURES_REPLY" snoopbr0.txt >/dev/null 2>&1]) OVS_WAIT_UNTIL([grep -E "OFPT_SET_CONFIG" snoopbr0.txt >/dev/null 2>&1]) +AT_CHECK([ovs-appctl -t $(pwd)/snoop.ctl exit]) +OVS_WAIT_WHILE([test -e ovs-ofctl-snoop.pid]) + dnl need to suppress the 'connection failed' WARN message in ovs-vswitchd dnl because we need ovs-vswitchd to have the controller config before starting dnl the controller to 'snoop' the OpenFlow messages from beginning --- If you can include something like this in the patch, update the tags, and send a v2, that would be great. Best regards, Ilya Maximets. _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
