[ovs-dev] [cleanups 03/12] Add functions to determine how port should be opened based on type.

[Ben Pfaff]

On Fri, Nov 16, 2012 at 10:14:52AM -0800, Justin Pettit wrote:
> On Fri, Nov 16, 2012 at 9:40 AM, Ben Pfaff <blp at nicira.com> wrote:
> 
> > On Fri, Nov 16, 2012 at 12:02:56AM -0800, Justin Pettit wrote:
> > > Depending on the port and type of datapath, a port may need to be opened
> > > as a different type of device than it's configured.  For example, an
> > > "internal" port on a "dummy" datapath should opened as a "dummy" port.
> > > This commit adds the ability for a dpif to provide this information to a
> > > caller.  It will be used in a future commit.
> > >
> > > Signed-off-by: Justin Pettit <jpettit at nicira.com>
> >
> > This looks good, with a few nits.
> >
> > I think that the comments on the various incarnations of
> > "port_open_type" are likely to be very confusing to anyone new to the
> > code.  I recommend adding a little bit of background on why the type
> > might need to be translated and to give an example.
> >
> > Also, the comment:
> > > +     * The caller must not free the returned string. */
> > is slightly confusing, because these functions are likely to return
> > their own arguments.  It might be better to say something like:
> >
> >         Returns either 'type' itself or a string literal.
> >
> > Or, possibly, that might be even more confusing.  Not sure.
> >
> 
> I changed all the references to be of a form similar to the following:
> 
>     /* Returns how a port of 'type' should be opened based on using a
>      * dpif of class 'dpif_class'.  This is necessary when the specified
>      * type is different from how the datapath needs it opened.  For
>      * example, when using the userspace datapath, a port of type
>      * "internal" needs to be opened as "tap".
>      *
>      * Returns either 'type' itself or a string literal, which must not
>      * be freed. */
> 
> I think that addresses both your issues and improves them overall.  Let me
> know if you disagree.

It's an improvement, thanks.

It might still be improved, perhaps, because "how" seems awfully vague
to me.  Maybe:

    Returns the type to pass to netdev_open() when a dpif of class
    'dpif_class' has a port of type 'type', for a few special cases when
    a netdev type differs from a port type.

etc.

Thanks!

Ben