[ovs-dev] [PATCH 4/4] netdev: Add support for "patch" type

Justin Pettit jpettit at nicira.com
Thu Apr 15 08:41:22 UTC 2010

On Apr 14, 2010, at 10:20 AM, Ben Pfaff wrote:

> I realize that you didn't introduce the above, but it still seems like a
> silly amount of overhead in the assertions.  Why not something like this:
>        assert(netdev_get_dev(netdev)->netdev_class->init == netdev_linux_init);
> Much cheaper than up to 4 string compares.

I don't disagree, but I don't think this is the patch to clean that up.  It sounds like Jesse is going to address that in his new GRE set.

> Did you consider rate-limiting the log messages in create_patch()?

Not really, since we don't do that in any of the other create_* error paths.  However, I went ahead and added them, since you're implying that might be a good idea.  I actually added them to all my new messages.  If you think it's important, we should do the same for the other netdev types.

> I think you could use shash_find_data(args, "peer") to simplify
> setup_patch().

That's true, but we wouldn't be able to detect unused arguments.  However, the usefulness of that is probably limited, so I went ahead and did a shash_find_data(). 

> setup_patch() has a label "error:" but I don't see anything that jumps
> to it.

D'oh.  Missed that on some late night restructuring last night.

> netdev_linux_create_patch() could avoid a free() by postponing the
> xzalloc() until after setup_patch() succeeds or fails.

Good point.  I cleaned that up.

> In a few places, the code assumes that fprintf() returns a negative
> errno value, but it doesn't: the error code must still be obtained from
> errno on failure.

Double d'oh.  Thanks!

> It might be a cleanup to factor out writing a string to
> /sys/class/net/veth_pairs into a new function, since those 15 or so
> lines appear twice.

Okay, I went ahead and did that.

> The XML file should also document the "peer" option for patch devices.
> (I think that it doesn't document the options for "gre" devices, but in
> my opinion that's a bug.)

I'm embarrassed to say this, but I didn't immediately see how to markup "peer", so I just skipped documenting it at the time.  Assuming that I should use <code> tags, I've actually fully explained how it should be used.

I've incorporated your suggestions and reworked a lot of the patch based on Jesse's concerns.  The changes were extensive enough that I sent them back out for another review.



More information about the dev mailing list