[ovs-dev] [PATCH 4/4] netdev: Add support for "patch" type
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
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