On 05.01.2020 10:42, Roi Dayan wrote: > > > On 2020-01-04 2:44 AM, Ilya Maximets wrote: >> dpif logging functions expects to be called after the operation. >> log_flow_del_message() dumps flow stats on success which are not >> initialized before the actual call to netdev_flow_del(): >> >> Conditional jump or move depends on uninitialised value(s) >> at 0x6090875: _itoa_word (_itoa.c:179) >> by 0x6093F0D: vfprintf (vfprintf.c:1642) >> by 0x60C090F: vsnprintf (vsnprintf.c:114) >> by 0xE5E7EC: ds_put_format_valist (dynamic-string.c:155) >> by 0xE5E755: ds_put_format (dynamic-string.c:142) >> by 0xE5A5E6: dpif_flow_stats_format (dpif.c:903) >> by 0xE5B708: log_flow_message (dpif.c:1763) >> by 0xE5BCA4: log_flow_del_message (dpif.c:1809) >> by 0xFA6076: try_send_to_netdev (dpif-netlink.c:2190) >> by 0xFA0D3C: dpif_netlink_operate (dpif-netlink.c:2248) >> by 0xE5AFAC: dpif_operate (dpif.c:1376) >> by 0xDF176E: push_dp_ops (ofproto-dpif-upcall.c:2367) >> by 0xDF04C8: push_ukey_ops (ofproto-dpif-upcall.c:2447) >> by 0xDF008F: revalidator_sweep__ (ofproto-dpif-upcall.c:2805) >> by 0xDF5DC6: revalidator_sweep (ofproto-dpif-upcall.c:2816) >> by 0xDF1E83: udpif_revalidator (ofproto-dpif-upcall.c:949) >> by 0xF3A3FE: ovsthread_wrapper (ovs-thread.c:383) >> by 0x565F6DA: start_thread (pthread_create.c:463) >> by 0x615988E: clone (clone.S:95) >> Uninitialised value was created by a stack allocation >> at 0xDEFC24: revalidator_sweep__ (ofproto-dpif-upcall.c:2733) >> >> Also, by calling the log functions after the actual operation we could >> pass the real error code instead of reporting success unconditionally. >> >> CC: Roi Dayan <[email protected]> >> Fixes: 3cd99886191e ("dpif-netlink: Use dpif logging functions") >> Signed-off-by: Ilya Maximets <[email protected]> >> --- >> lib/dpif-netlink.c | 6 +++--- >> 1 file changed, 3 insertions(+), 3 deletions(-) >> >> diff --git a/lib/dpif-netlink.c b/lib/dpif-netlink.c >> index a08c2378a..25423e86c 100644 >> --- a/lib/dpif-netlink.c >> +++ b/lib/dpif-netlink.c >> @@ -2176,8 +2176,8 @@ try_send_to_netdev(struct dpif_netlink *dpif, struct >> dpif_op *op) >> break; >> } >> >> - log_flow_put_message(&dpif->dpif, &this_module, put, 0); >> err = parse_flow_put(dpif, put); >> + log_flow_put_message(&dpif->dpif, &this_module, put, err); >> break; >> } >> case DPIF_OP_FLOW_DEL: { >> @@ -2187,9 +2187,9 @@ try_send_to_netdev(struct dpif_netlink *dpif, struct >> dpif_op *op) >> break; >> } >> >> - log_flow_del_message(&dpif->dpif, &this_module, del, 0); >> err = netdev_ports_flow_del(dpif->dpif.dpif_class, del->ufid, >> del->stats); >> + log_flow_del_message(&dpif->dpif, &this_module, del, err); >> break; >> } >> case DPIF_OP_FLOW_GET: { >> @@ -2199,8 +2199,8 @@ try_send_to_netdev(struct dpif_netlink *dpif, struct >> dpif_op *op) >> break; >> } >> >> - log_flow_get_message(&dpif->dpif, &this_module, get, 0); >> err = parse_flow_get(dpif, get); >> + log_flow_get_message(&dpif->dpif, &this_module, get, err); >> break; >> } >> case DPIF_OP_EXECUTE: >> > > > Hi Ilya, > > The original prints were debug prints. > and the original commit (fixes line) was to remove the custom dbg function > and use the dpif functions. but still as debug so we pass err code 0. > So we call the debug before the action because each flow not supported to > be offloaded will return an error to fallback to SW but we dont want to > consider this error there. With this patch I think we will get too many > errors in the log you don't care about when ovs will try to add/del/get > a flow that is not offloaded. > > The call to log_flow_get_message() with an error code is from dpif_operate() > which first will try offload to netdev, then callback to netlink, and only if > that > fails call to log_flow_get_message() with the last error code.
I see your point. I'll send v2 without passing err to log functions. Best regards, Ilya Maximets. _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
