[ovs-dev] [patch v7] dpctl: Make opt_dpif_open() more general.

[Darrell Ball]

Ben

If you apply this, I have one line to delete inline.

Darrell

On Wed, Aug 8, 2018 at 2:09 PM, Darrell Ball <dlu998 at gmail.com> wrote:

> By making opt_dpif_open() more general, it can be used effectively
> by all potential callers and avoids trying to open potentially bogus
> datapaths provided by the user. Also, the error handling is improved by
> reducing bogus errors and having more specific real errors.
>
> Signed-off-by: Darrell Ball <dlu998 at gmail.com>
> ---
>  lib/dpctl.c             | 56 ++++++++++++++++++++++++++++++
> ++++++++++++++-----
>  tests/system-traffic.at | 10 ++++-----
>  2 files changed, 56 insertions(+), 10 deletions(-)
>
> diff --git a/lib/dpctl.c b/lib/dpctl.c
> index c600eeb..ef057aa 100644
> --- a/lib/dpctl.c
> +++ b/lib/dpctl.c
> @@ -187,18 +187,64 @@ parsed_dpif_open(const char *arg_, bool create,
> struct dpif **dpifp)
>      return result;
>  }
>
> +static bool
> +dp_exists(const char *queried_dp)
> +{
> +    struct sset dpif_names = SSET_INITIALIZER(&dpif_names),
> +                dpif_types = SSET_INITIALIZER(&dpif_types);
> +    bool found = false;
> +    char *queried_name, *queried_type;
> +
> +    dp_parse_name(queried_dp, &queried_name, &queried_type);
> +    dp_enumerate_types(&dpif_types);
> +
> +    if (!sset_contains(&dpif_types, queried_type)) {
> +        goto out;
> +    }
> +
> +    int error = dp_enumerate_names(queried_type, &dpif_names);
> +    if (!error && sset_contains(&dpif_names, queried_name)) {
> +        found = true;
> +        goto out;
>

Pls remove the above "goto out;"



> +    }
> +
> +out:
> +    sset_destroy(&dpif_names);
> +    sset_destroy(&dpif_types);
> +    return found;
> +}
> +
> +static bool
> +dp_arg_exists(int argc, const char *argv[])
> +{
> +    if (argc > 1 && dp_exists(argv[1])) {
> +        return true;
> +    }
> +    return false;
> +}
> +
>  /* Open a dpif with an optional name argument.
>   *
> - * The datapath name is not a mandatory parameter for this command.  If
> - * it is not specified -- so 'argc' < 'max_args' -- we retrieve it from
> - * the current setup, assuming only one exists.  On success stores the
> - * opened dpif in '*dpifp'. */
> + * The datapath name is not a mandatory parameter for this command.  If
> it is
> + * not specified, we retrieve it from the current setup, assuming only one
> + * exists.  On success stores the opened dpif in '*dpifp'. */
>  static int
>  opt_dpif_open(int argc, const char *argv[], struct dpctl_params *dpctl_p,
>                uint8_t max_args, struct dpif **dpifp)
>  {
> +    char *dpname;
> +    if (dp_arg_exists(argc, argv)) {
> +        dpname = xstrdup(argv[1]);
> +    } else if (argc != max_args) {
> +        dpname = get_one_dp(dpctl_p);
> +    } else {
> +        /* If the arguments are the maximum possible number and there is
> no
> +         * valid datapath argument, then we fall into the case of dpname
> is
> +         * NULL, since this is an error. */
> +        dpname = NULL;
> +    }
> +
>      int error = 0;
> -    char *dpname = argc >= max_args ? xstrdup(argv[1]) :
> get_one_dp(dpctl_p);
>      if (!dpname) {
>          error = EINVAL;
>          dpctl_error(dpctl_p, error, "datapath not found");
> diff --git a/tests/system-traffic.at b/tests/system-traffic.at
> index cbd9542..a7c8a24 100644
> --- a/tests/system-traffic.at
> +++ b/tests/system-traffic.at
> @@ -1040,7 +1040,7 @@ AT_CHECK([ovs-appctl dpctl/flush-conntrack zone=5
> $ICMP_TUPLE])
>  AT_CHECK([ovs-appctl dpctl/dump-conntrack | grep
> "orig=.src=10\.1\.1\.2,"], [1], [dnl
>  ])
>
> -OVS_TRAFFIC_VSWITCHD_STOP(["/could not create datapath/d"])
> +OVS_TRAFFIC_VSWITCHD_STOP
>  AT_CLEANUP
>
>  AT_SETUP([conntrack - IPv4 ping])
> @@ -1124,17 +1124,17 @@ ovs-appctl: ovs-vswitchd: server returned an error
>  ])
>
>  AT_CHECK([ovs-appctl dpctl/ct-set-maxconns one-bad-dp 10], [2], [], [dnl
> -ovs-vswitchd: opening datapath (Address family not supported by protocol)
> +ovs-vswitchd: datapath not found (Invalid argument)
>  ovs-appctl: ovs-vswitchd: server returned an error
>  ])
>
>  AT_CHECK([ovs-appctl dpctl/ct-get-maxconns one-bad-dp], [2], [], [dnl
> -ovs-vswitchd: opening datapath (Address family not supported by protocol)
> +ovs-vswitchd: datapath not found (Invalid argument)
>  ovs-appctl: ovs-vswitchd: server returned an error
>  ])
>
>  AT_CHECK([ovs-appctl dpctl/ct-get-nconns one-bad-dp], [2], [], [dnl
> -ovs-vswitchd: opening datapath (Address family not supported by protocol)
> +ovs-vswitchd: datapath not found (Invalid argument)
>  ovs-appctl: ovs-vswitchd: server returned an error
>  ])
>
> @@ -1164,7 +1164,7 @@ AT_CHECK([ovs-appctl dpctl/ct-get-maxconns], [], [dnl
>  10
>  ])
>
> -OVS_TRAFFIC_VSWITCHD_STOP(["/could not create datapath one-bad-dp of
> unknown type system/d"])
> +OVS_TRAFFIC_VSWITCHD_STOP
>  AT_CLEANUP
>
>  AT_SETUP([conntrack - IPv6 ping])
> --
> 1.9.1
>
>