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

Message ID 1533762554-11008-1-git-send-email-dlu998@gmail.com
State New
Headers show
Series
  • [ovs-dev,v7] dpctl: Make opt_dpif_open() more general.
Related show

Commit Message

Darrell Ball Aug. 8, 2018, 9:09 p.m.
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@gmail.com>
---
 lib/dpctl.c             | 56 ++++++++++++++++++++++++++++++++++++++++++++-----
 tests/system-traffic.at | 10 ++++-----
 2 files changed, 56 insertions(+), 10 deletions(-)

Comments

Darrell Ball Aug. 8, 2018, 9:12 p.m. | #1
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@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@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
>
>
Darrell Ball Aug. 10, 2018, 5:02 p.m. | #2
On Wed, Aug 8, 2018 at 2:12 PM, Darrell Ball <dlu998@gmail.com> wrote:

> 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@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@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;"
>


I ended up sending a V8 with a little more compact code. at the expense of
a slight decrease
in readability and this label is gone in V8.



>
>
>
>> +    }
>> +
>> +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
>>
>>
>

Patch

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;
+    }
+
+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])