[ovs-dev] [PATCH 2/5] dpctl: Use common code to open dpif with optional name.
Justin Pettit
jpettit at ovn.org
Fri Jun 15 08:07:26 UTC 2018
> On Jun 14, 2018, at 10:40 PM, Darrell Ball <dlu998 at gmail.com> wrote:
>
>> On Thu, Jun 14, 2018 at 12:00 PM, Justin Pettit <jpettit at ovn.org> wrote:
>> Signed-off-by: Justin Pettit <jpettit at ovn.org>
>> ---
>> 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
More information about the dev
mailing list