[ovs-discuss] [PATCH 00/20] Implement abstract interfaces to network devices
Ben Pfaff
blp at nicira.com
Tue Jul 28 20:20:39 UTC 2009
Thanks for the review.
Jesse Gross <jesse at nicira.com> writes:
> In ofproto.c/ofproto_create(), it seems weird to be calling
> pick_fallback_dpid() twice - once for p->fallback_dpid and once for
> p->datapath_id since this will generate two different random ID's.
> p->datapath_id gets overwritten later on with the final ID, so it
> seems like it is either redundant to generate it earlier (if no one
> uses it between those to calls) or potentially bad (if someone uses
> datapath_id and assumes that it is the final value).
Oops. You're right.
I fixed this up by changing the second assignment to just do
"p->datapath_id = p->fallback_id;"
There's only one use of datapath_id, which is in generating the
reply to an OpenFlow features request, so it can't get used in
between.
> In bridge.c, I'm not sure that open_iface_netdev is the best name for
> the function if it also retrieves other information like carrier
> status.
OK, I applied this incrementally on the commit that creates that
function. Do you like that better?
diff --git a/vswitchd/bridge.c b/vswitchd/bridge.c
index 95764f0..8ff536a 100644
--- a/vswitchd/bridge.c
+++ b/vswitchd/bridge.c
@@ -359,8 +359,11 @@ bridge_configure_ssl(void)
}
#endif
+/* for_each_iface() callback function that opens the network device for
+ * 'iface', if it is not already open, and retrieves the interface's MAC
+ * address and carrier status. */
static bool
-open_iface_netdev(struct bridge *br UNUSED, struct iface *iface,
+init_iface_netdev(struct bridge *br UNUSED, struct iface *iface,
void *aux UNUSED)
{
if (iface->netdev) {
@@ -371,6 +374,8 @@ open_iface_netdev(struct bridge *br UNUSED, struct iface *iface,
netdev_get_carrier(iface->netdev, &iface->enabled);
return true;
} else {
+ /* If the network device can't be opened, then we're not going to try
+ * to do anything with this interface. */
return false;
}
}
@@ -539,7 +544,7 @@ bridge_reconfigure(void)
struct svec nf_hosts;
bridge_fetch_dp_ifaces(br);
- for_each_iface(br, open_iface_netdev, NULL);
+ for_each_iface(br, init_iface_netdev, NULL);
local_iface = NULL;
for_each_iface(br, check_iface_dp_ifidx, &local_iface);
> The name lxnetdev isn't overly descriptive of the actual
> functionality. I'm not sure what's better though.
I wasn't very happy with that either, but it was the best that I
could come up with at the time.
Now I've renamed it to "rtnetlink" and moved it into a separate
source and header file. "rtnetlink" may not be the best name
either, but at least it accurately reflects the name of the
Linux kernel entities that it deals with.
> The fact that an initialization failure in one of the classes of
> netdevs blocks all netdevs seems a little fragile to me.
Hmm, I guess we can just block that particular netdev class. I
applied the following incremental change:
diff --git a/lib/netdev-provider.h b/lib/netdev-provider.h
index eef1061..def0568 100644
--- a/lib/netdev-provider.h
+++ b/lib/netdev-provider.h
@@ -73,7 +73,7 @@ struct netdev_class {
const char *name;
/* Called only once, at program startup. Returning an error from this
- * function will prevent any network device, of any class, from being
+ * function will prevent any network device in this class from being
* opened.
*
* This function may be set to null if a network device class needs no
diff --git a/lib/netdev.c b/lib/netdev.c
index 5752914..dcb63fa 100644
--- a/lib/netdev.c
+++ b/lib/netdev.c
@@ -43,7 +43,7 @@ static const struct netdev_class *netdev_classes[] = {
&netdev_linux_class,
&netdev_tap_class,
};
-enum { N_NETDEV_CLASSES = ARRAY_SIZE(netdev_classes) };
+static int n_netdev_classes = ARRAY_SIZE(netdev_classes);
/* All open network devices. */
static struct list netdev_list = LIST_INITIALIZER(&netdev_list);
@@ -65,16 +65,18 @@ netdev_initialize(void)
{
static int status = -1;
if (status < 0) {
- int i;
+ int i, j;
fatal_signal_add_hook(restore_all_flags, NULL, true);
status = 0;
- for (i = 0; i < N_NETDEV_CLASSES; i++) {
+ for (i = j = 0; i < n_netdev_classes; i++) {
const struct netdev_class *class = netdev_classes[i];
if (class->init) {
int retval = class->init();
- if (retval) {
+ if (!retval) {
+ netdev_classes[j++] = class;
+ } else {
VLOG_ERR("failed to initialize %s network device "
"class: %s", class->name, strerror(retval));
if (!status) {
@@ -83,6 +85,7 @@ netdev_initialize(void)
}
}
}
+ n_netdev_classes = j;
}
return status;
}
@@ -95,7 +98,7 @@ void
netdev_run(void)
{
int i;
- for (i = 0; i < N_NETDEV_CLASSES; i++) {
+ for (i = 0; i < n_netdev_classes; i++) {
const struct netdev_class *class = netdev_classes[i];
if (class->run) {
class->run();
@@ -111,7 +114,7 @@ void
netdev_wait(void)
{
int i;
- for (i = 0; i < N_NETDEV_CLASSES; i++) {
+ for (i = 0; i < n_netdev_classes; i++) {
const struct netdev_class *class = netdev_classes[i];
if (class->wait) {
class->wait();
@@ -136,11 +139,7 @@ netdev_open(const char *name_, int ethertype, struct netdev **netdevp)
int error;
int i;
- error = netdev_initialize();
- if (error) {
- return error;
- }
-
+ netdev_initialize();
colon = strchr(name, ':');
if (colon) {
*colon = '\0';
@@ -151,7 +150,7 @@ netdev_open(const char *name_, int ethertype, struct netdev **netdevp)
suffix = name;
}
- for (i = 0; i < N_NETDEV_CLASSES; i++) {
+ for (i = 0; i < n_netdev_classes; i++) {
const struct netdev_class *class = netdev_classes[i];
if (!strcmp(prefix, class->prefix)) {
error = class->open(name_, suffix, ethertype, &netdev);
@@ -220,13 +219,10 @@ netdev_enumerate(struct svec *svec)
svec_init(svec);
- error = netdev_initialize();
- if (error) {
- return error;
- }
+ netdev_initialize();
error = 0;
- for (i = 0; i < N_NETDEV_CLASSES; i++) {
+ for (i = 0; i < n_netdev_classes; i++) {
const struct netdev_class *class = netdev_classes[i];
if (class->enumerate) {
int retval = class->enumerate(svec);
> Two comments about comments in netdev-provider.c/netdev_class:
> * prefix says that the empty string matches class names that wouldn't
> otherwise match. In reality, the empty string only matches names
> without prefixes.
> * get_in6 refers to setting in4
Thanks, I fixed these comments.
> I believe that netdev-linux.c/netdev_tap_class needs to have a prefix
> of "tap"
Yes, thanks, I fixed this now.
> Having both a Linux netdev and a tap netdev will cause the
> initialization to happen twice, overwriting some of the static
> variables (mainly af_inet_sock).
Good catches. I applied the following:
diff --git a/lib/netdev-linux.c b/lib/netdev-linux.c
index 5a294ab..30a6d5d 100644
--- a/lib/netdev-linux.c
+++ b/lib/netdev-linux.c
@@ -106,7 +106,7 @@ static struct shash cache_map = SHASH_INITIALIZER(&cache_map);
static struct rtnetlink_notifier netdev_linux_cache_notifier;
/* An AF_INET socket (used for ioctl operations). */
-static int af_inet_sock;
+static int af_inet_sock = -1;
struct netdev_linux_notifier {
struct netdev_notifier notifier;
@@ -148,13 +148,15 @@ netdev_linux_cast(const struct netdev *netdev)
static int
netdev_linux_init(void)
{
- af_inet_sock = socket(AF_INET, SOCK_DGRAM, 0);
- if (af_inet_sock < 0) {
- VLOG_ERR("failed to create inet socket: %s", strerror(errno));
- return errno;
+ static int status = -1;
+ if (status < 0) {
+ af_inet_sock = socket(AF_INET, SOCK_DGRAM, 0);
+ status = af_inet_sock >= 0 ? 0 : errno;
+ if (status) {
+ VLOG_ERR("failed to create inet socket: %s", strerror(status));
+ }
}
-
- return 0;
+ return status;
}
static void
@@ -1273,12 +1275,12 @@ const struct netdev_class netdev_linux_class = {
};
const struct netdev_class netdev_tap_class = {
- "", /* prefix */
+ "tap", /* prefix */
"tap", /* name */
netdev_linux_init,
- netdev_linux_run,
- netdev_linux_wait,
+ NULL, /* run */
+ NULL, /* wait */
netdev_linux_open,
netdev_linux_close,
> Do we really want to reinitialize all of the netdev classes every time
> someone opens a netdev?
That doesn't happen, since the "status" variable in
netdev_initialize() is static.
> Some of the netdevs may want to use the config file, so it would be
> useful to have a reconfigure call in the netdev interface.
My gut reaction is that that would be a mistake, at least at this
point. If the netdevs need configuration then I think we should
add an interface to netdev_*() for doing that instead of letting
them read the configuration willy-nilly.
But I don't know what kind of configuration you have in mind.
Can you give an example?
I'll post the whole updated series when I've finished revising
it.
More information about the discuss
mailing list