[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.


More information about the dev mailing list