On Wed, Aug 8, 2018 at 2:12 PM, Darrell Ball <[email protected]> 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 <[email protected]> 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 <[email protected]> >> --- >> 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 >> >> > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
