> On Jun 14, 2018, at 10:40 PM, Darrell Ball <[email protected]> wrote: > >> On Thu, Jun 14, 2018 at 12:00 PM, Justin Pettit <[email protected]> wrote: >> Signed-off-by: Justin Pettit <[email protected]> >> --- >> lib/dpctl.c | 158 >> +++++++++++++++--------------------------------------------- >> 1 file changed, 38 insertions(+), 120 deletions(-) >> >> diff --git a/lib/dpctl.c b/lib/dpctl.c >> index ec8c51e4b0a7..f522785a5f97 100644 >> --- a/lib/dpctl.c >> +++ b/lib/dpctl.c >> @@ -187,6 +187,31 @@ parsed_dpif_open(const char *arg_, bool create, struct >> dpif **dpifp) >> return result; >> } >> >> +/* 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 '*dpif'. */ >> +static int >> +opt_dpif_open(int argc, const char *argv[], struct dpctl_params *dpctl_p, >> + struct dpif **dpif, uint8_t max_args) >> +{ >> ... >> > When I wrote the generic function dpctl_ct_open_dp() for a few APIs related > to a specific commit, which you now > renamed to opt_dpif_open() here, I intended to come back and use it for the > other callers here as well :-). > Glad to see you got to it.
Yes, thanks for starting the work. I just noticed a few places it could also be leveraged when reviewing your fragment patches. > Minor comment is ‘dpif’ could be last parameter, since it is an ‘out’ > parameter Good point. I changed it. > Also, I am curious why you used ‘opt’ prefix, since this API is needed to > retrieve a dp even if one is not specified as one of > the arguments. You could use something like dpctl_open_dp_(), with a trailing > underscore to specify an internal API. The "opt" was used to indicate that the name argument was optional. I usually think of names with a trailing underscores as ones that makes up a function called by one without the underscores. I think the way it's written follows the convention of other functions in the file such as parsed_dpif_open(). I think between that and the comment that describes its use, it should be pretty clear. Thanks for the feedback. --Justin _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
