[ovs-dev] [PATCH 1/3] dpctl: Fix error handling and reporting regressions.

[Daniele Di Proietto]

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 <diproiettod at vmware.com>

Thanks for reporting and fixing this!

> On 15 Apr 2015, at 19:19, Ben Pfaff <blp at nicira.com> 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 <kevlo at FreeBSD.org>
> Signed-off-by: Ben Pfaff <blp at nicira.com>
> ---
> 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;
>         }