On Tue, Jul 11, 2023 at 7:40 PM Eelco Chaudron <[email protected]> wrote: > > I think we should not assert, but do proper error handling here with a > dpctl_error() >
Sure, will do that in the next patch version. > > Same as above. > Got it. > > I think if we do a pop from an empty set, we should just return NULL, not > assert. > Sure. > > Did you analyse we only need the check for parent and not vsctl_bridge or > port_config? Maybe it would be nicer to fix the calling functions to avoid > this warning, as I guess that is where the real problem exists? > After checking the caller functions again, I realized that they now have an `ovs_assert`. Hence, this should no longer be necessary. Will remove this. > > Is this really needed? As looking at the code add_port_to_cache() will assert > and never return Null? > > Asking as you did not add a similar assert for line 791 above; ‘br = > add_bridge_to_cache(vsctl_ctx, br_cfg, br_cfg->name, NULL, 0);’ > Yup, should not be necessary given that it uses xzalloc. Will remove this. > > See above. Got it. > > Cant we not log a warning and skip freeing the data and the node? > Sure, will do that instead. Best regards, James Raphael Tiovalen _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
