On Fri, Nov 16, 2012 at 9:40 AM, Ben Pfaff <b...@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 <jpet...@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.
Thanks,
--Justin
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
