I’ve spend some time looking at this series, since I introduced most of these bugs (sorry!)
With a minor comment below Acked-by: Daniele Di Proietto <[email protected]> Thanks for reporting and fixing this! > On 15 Apr 2015, at 19:19, Ben Pfaff <[email protected]> wrote: > > Fixes multiple weaknesses in dpctl error reporting: > > * dpctl_set_if() didn't stop processing or report to the caller > attempts to change a port type or number. > > * dpctl_set_if() didn't report the specifics when netdev_set_config() > reported an error setting port configuration (which can happen even > it returns 0). > > * The unixctl handler didn't report errors encountered during command > processing through the JSON-RPC error mechanism, which meant that > ovs-appctl's return code wasn't useful (as ovs-dpctl's return code > is useful) for detecting errors in command execution. > > At least the first of these is a regression from OVS 2.3.x. > > A followup commit will add tests. > > Reported-by: Kevin Lo <[email protected]> > Signed-off-by: Ben Pfaff <[email protected]> > --- > lib/dpctl.c | 34 +++++++++++++++++++++++----------- > 1 file changed, 23 insertions(+), 11 deletions(-) > > diff --git a/lib/dpctl.c b/lib/dpctl.c > index 4c4d1c3..a9a56ff 100644 > --- a/lib/dpctl.c > +++ b/lib/dpctl.c [...] > @@ -397,7 +399,13 @@ dpctl_set_if(int argc, const char *argv[], struct > dpctl_params *dpctl_p) > } > > /* Update configuration. */ > - error = netdev_set_config(netdev, &args, NULL); > + char *err_s = NULL; > + error = netdev_set_config(netdev, &args, &err_s); > + if (err_s || error) { > + dpctl_error(dpctl_p, error, > + err_s ? err_s : "Error updating configuration"); > + free(err_s); > + } Not strictly related to this patch, but I believe we could also remove the following lines, since the "next_destroy_args:" label is immediately below. > if (error) { > goto next_destroy_args; > } _______________________________________________ dev mailing list [email protected] http://openvswitch.org/mailman/listinfo/dev
