[ovs-dev] [netlink v4+2 2/2] datapath: Change dp_idx to dp_ifindex, the ifindex of the local port.
Ben Pfaff
blp at nicira.com
Sat Jan 22 01:05:40 UTC 2011
On Thu, Jan 20, 2011 at 07:35:59PM -0800, Jesse Gross wrote:
> On Fri, Jan 14, 2011 at 9:53 AM, Ben Pfaff <blp at nicira.com> wrote:
> > diff --git a/datapath/datapath.c b/datapath/datapath.c
> > index c73968c..fd19ef6 100644
> > --- a/datapath/datapath.c
> > +++ b/datapath/datapath.c
> > ?static int odpdc_dump(struct sk_buff *skb, struct netlink_callback *cb)
> > ?{
> > - ? ? ? u32 dp_idx;
> > + ? ? ? struct datapath *dp;
> > + ? ? ? int skip = cb->args[0];
> > + ? ? ? int i = 0;
> >
> > ? ? ? ?mutex_lock(&dp_mutex);
> > - ? ? ? for (dp_idx = cb->args[0]; dp_idx < ARRAY_SIZE(dps); dp_idx++) {
> > - ? ? ? ? ? ? ? struct datapath *dp = get_dp(dp_idx);
> > - ? ? ? ? ? ? ? if (!dp)
> > + ? ? ? list_for_each_entry_rcu (dp, &dps, list_node) {
>
> Is this actually protected by RCU? It looks like it's using dp_mutex.
Argh, you're right. I've now deleted all the "_rcu"s around list_node.
> > diff --git a/lib/dpif-linux.c b/lib/dpif-linux.c
> > index b097731..9ad20e1 100644
> > --- a/lib/dpif-linux.c
> > +++ b/lib/dpif-linux.c
> > ?static int
> > ?dpif_linux_get_all_names(const struct dpif *dpif_, struct svec *all_names)
> > ?{
> > - ? ?struct dpif_linux *dpif = dpif_linux_cast(dpif_);
> > -
> > - ? ?svec_add_nocopy(all_names, xasprintf("dp%d", dpif->dp_idx));
> > - ? ?svec_add(all_names, dpif->local_ifname);
> > + ? ?svec_add(all_names, dpif_base_name(dpif_));
> > ? ? return 0;
> > ?}
>
> I think we could just make this a NULL pointer, as the implementation
> in dpif.c does this by default. The comment above
> dpif_get_all_names() also refers to the Linux dpif using dpX, which is
> no longer true.
Good point, thanks, fixed.
> Do we actually even need dpif_get_all_names() anymore? I don't
> believe that anything is currently using and I'm not sure whether
> other dpif implementations would be likely to do so.
Another good point. Thanks, I've added another commit to remove that
function and references to it.
> > diff --git a/lib/dpif.man b/lib/dpif.man
> > index 775ec58..9597ca1 100644
> > --- a/lib/dpif.man
> > +++ b/lib/dpif.man
> > @@ -1,11 +1,5 @@
> > ?.RS
> > ?.TP
> > -[\fItype\fB@\fR]\fBdp\fIN\fR
> > -Datapath number \fIN\fR, where \fIN\fR is a number between 0 and 255,
> > -inclusive. ?If \fItype\fR is given, it specifies the datapath provider of
> > -\fBdp\fIN\fR, otherwise the default provider \fBsystem\fR is assumed.
> > -.
> > -.TP
> > ?[\fItype\fB@\fR]\fIname\fR
> > ?The name of the network device associated with the datapath's local
> > ?port. ?(\fB\*(PN\fR internally converts this into a datapath number,
>
> This still refers to datapath number.
>
> The text above this in the ovs-dpctl man page refers to multiple forms
> of input but there is only a single one now.
I think it's better to delete lib/dpif.man now and just describe the
syntax directly in the files that used to include it.
Here's the incremental, followed by the additional commit to remove
dpif_get_all_names().
--8<--------------------------cut here-------------------------->8--
diff --git a/datapath/datapath.c b/datapath/datapath.c
index a2351bd..5c28250 100644
--- a/datapath/datapath.c
+++ b/datapath/datapath.c
@@ -1387,7 +1387,7 @@ static int odpdc_new(struct sk_buff *skb, struct genl_info *info)
if (IS_ERR(reply))
goto err_destroy_local_port;
- list_add_tail_rcu(&dp->list_node, &dps);
+ list_add_tail(&dp->list_node, &dps);
dp_sysfs_add_dp(dp);
mutex_unlock(&dp_mutex);
@@ -1432,7 +1432,7 @@ static int odpdc_del(struct sk_buff *skb, struct genl_info *info)
dp_detach_port(vport);
dp_sysfs_del_dp(dp);
- list_del_rcu(&dp->list_node);
+ list_del(&dp->list_node);
dp_detach_port(get_vport_protected(dp, ODPP_LOCAL));
call_rcu(&dp->rcu, destroy_dp_rcu);
@@ -1504,7 +1504,7 @@ static int odpdc_dump(struct sk_buff *skb, struct netlink_callback *cb)
int i = 0;
mutex_lock(&dp_mutex);
- list_for_each_entry_rcu (dp, &dps, list_node) {
+ list_for_each_entry (dp, &dps, list_node) {
if (i < skip)
continue;
if (odpdc_fill_info(dp, skb, NETLINK_CB(cb->skb).pid,
diff --git a/lib/automake.mk b/lib/automake.mk
index ba02d41..c51d3ed 100644
--- a/lib/automake.mk
+++ b/lib/automake.mk
@@ -1,4 +1,4 @@
-# Copyright (C) 2009, 2010 Nicira Networks, Inc.
+# Copyright (C) 2009, 2010, 2011 Nicira Networks, Inc.
#
# Copying and distribution of this file, with or without modification,
# are permitted in any medium without royalty provided the copyright
@@ -222,7 +222,6 @@ EXTRA_DIST += \
lib/common-syn.man \
lib/daemon.man \
lib/daemon-syn.man \
- lib/dpif.man \
lib/leak-checker.man \
lib/ssl-bootstrap.man \
lib/ssl-bootstrap-syn.man \
diff --git a/lib/dpif-linux.c b/lib/dpif-linux.c
index 8adfca5..dbd8b71 100644
--- a/lib/dpif-linux.c
+++ b/lib/dpif-linux.c
@@ -116,6 +116,7 @@ static void dpif_linux_flow_get_stats(const struct dpif_linux_flow *,
/* Datapath interface for the openvswitch Linux kernel module. */
struct dpif_linux {
struct dpif dpif;
+ int dp_ifindex;
/* Multicast group messages. */
struct nl_sock *mc_sock;
@@ -124,9 +125,6 @@ struct dpif_linux {
uint32_t mcgroup_sample;
unsigned int listen_mask;
- /* Used by dpif_linux_get_all_names(). */
- int dp_ifindex;
-
/* Change notification. */
struct shash changed_ports; /* Ports that have changed. */
struct rtnetlink_notifier port_notifier;
@@ -256,13 +254,6 @@ dpif_linux_close(struct dpif *dpif_)
}
static int
-dpif_linux_get_all_names(const struct dpif *dpif_, struct svec *all_names)
-{
- svec_add(all_names, dpif_base_name(dpif_));
- return 0;
-}
-
-static int
dpif_linux_destroy(struct dpif *dpif_)
{
struct dpif_linux *dpif = dpif_linux_cast(dpif_);
@@ -915,7 +906,7 @@ const struct dpif_class dpif_linux_class = {
dpif_linux_enumerate,
dpif_linux_open,
dpif_linux_close,
- dpif_linux_get_all_names,
+ NULL, /* get_all_names */
dpif_linux_destroy,
dpif_linux_get_stats,
dpif_linux_get_drop_frags,
diff --git a/lib/dpif.man b/lib/dpif.man
deleted file mode 100644
index 9597ca1..0000000
--- a/lib/dpif.man
+++ /dev/null
@@ -1,8 +0,0 @@
-.RS
-.TP
-[\fItype\fB@\fR]\fIname\fR
-The name of the network device associated with the datapath's local
-port. (\fB\*(PN\fR internally converts this into a datapath number,
-as above.) If \fItype\fR is given, it specifies the datapath provider of
-\fIname\fR, otherwise the default provider \fBsystem\fR is assumed.
-.RE
diff --git a/utilities/ovs-dpctl.8.in b/utilities/ovs-dpctl.8.in
index f551052..12fa27b 100644
--- a/utilities/ovs-dpctl.8.in
+++ b/utilities/ovs-dpctl.8.in
@@ -24,10 +24,12 @@ that network device to the datapath.
If \fBovs\-vswitchd\fR(8) is in use, use \fBovs\-vsctl\fR(8) instead
of \fBovs\-dpctl\fR.
.PP
-Most \fBovs\-dpctl\fR commands that work with datapaths take an argument
-that specifies the name of the datapath, in one of the following
-forms:
-.so lib/dpif.man
+Most \fBovs\-dpctl\fR commands that work with datapaths take an
+argument that specifies the name of the datapath. Datapath names take
+the form [\fItype\fB@\fR]\fIname\fR, where \fIname\fR is the network
+device associated with the datapath's local port. If \fItype\fR is
+given, it specifies the datapath provider of \fIname\fR, otherwise the
+default provider \fBsystem\fR is assumed.
.PP
The following commands manage datapaths.
.
diff --git a/utilities/ovs-openflowd.8.in b/utilities/ovs-openflowd.8.in
index b84f8e7..9dec805 100644
--- a/utilities/ovs-openflowd.8.in
+++ b/utilities/ovs-openflowd.8.in
@@ -19,10 +19,12 @@ OpenFlow controllers over TCP or SSL.
For a more powerful alternative to \fBovs\-openflowd\fR, see
\fBovs\-vswitchd\fR(8). Do not run both daemons at the same time.
.PP
-The mandatory \fIdatapath\fR argument argument specifies the local datapath
-to relay. It takes one of the following forms:
-.
-.so lib/dpif.man
+The mandatory \fIdatapath\fR argument argument specifies the local
+datapath to relay. It takes the form [\fItype\fB@\fR]\fIname\fR,
+where \fIname\fR is the network device associated with the datapath's
+local port. If \fItype\fR is given, it specifies the datapath
+provider of \fIname\fR, otherwise the default provider \fBsystem\fR is
+assumed.
.
.PP
The optional \fIcontroller\fR arguments specify how to connect to the
--8<--------------------------cut here-------------------------->8--
>From bfdc0c63eaf757957b07d57b3b17501170d920ca Mon Sep 17 00:00:00 2001
From: Ben Pfaff <blp at nicira.com>
Date: Fri, 21 Jan 2011 16:56:26 -0800
Subject: [PATCH] dpif: Remove dpif_get_all_names().
None of the remaining dpif implementations have more than one name per
dpif, so there's no need for this function anymore.
Suggested-by: Jesse Gross <jesse at nicira.com>
---
lib/dpif-linux.c | 1 -
lib/dpif-netdev.c | 1 -
lib/dpif-provider.h | 17 -----------------
lib/dpif.c | 27 ---------------------------
lib/dpif.h | 1 -
vswitchd/bridge.c | 32 ++++++++++----------------------
6 files changed, 10 insertions(+), 69 deletions(-)
diff --git a/lib/dpif-linux.c b/lib/dpif-linux.c
index dbd8b71..4cdbb1d 100644
--- a/lib/dpif-linux.c
+++ b/lib/dpif-linux.c
@@ -906,7 +906,6 @@ const struct dpif_class dpif_linux_class = {
dpif_linux_enumerate,
dpif_linux_open,
dpif_linux_close,
- NULL, /* get_all_names */
dpif_linux_destroy,
dpif_linux_get_stats,
dpif_linux_get_drop_frags,
diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
index dd0f9a0..b02d5db 100644
--- a/lib/dpif-netdev.c
+++ b/lib/dpif-netdev.c
@@ -1412,7 +1412,6 @@ const struct dpif_class dpif_netdev_class = {
NULL, /* enumerate */
dpif_netdev_open,
dpif_netdev_close,
- NULL, /* get_all_names */
dpif_netdev_destroy,
dpif_netdev_get_stats,
dpif_netdev_get_drop_frags,
diff --git a/lib/dpif-provider.h b/lib/dpif-provider.h
index 9554f92..23dfd4b 100644
--- a/lib/dpif-provider.h
+++ b/lib/dpif-provider.h
@@ -102,23 +102,6 @@ struct dpif_class {
/* Closes 'dpif' and frees associated memory. */
void (*close)(struct dpif *dpif);
- /* Enumerates all names that may be used to open 'dpif' into 'all_names'.
- * The Linux datapath, for example, supports opening a datapath both by
- * number, e.g. "dp0", and by the name of the datapath's local port. For
- * some datapaths, this might be an infinite set (e.g. in a file name,
- * slashes may be duplicated any number of times), in which case only the
- * names most likely to be used should be enumerated.
- *
- * The caller has already initialized 'all_names' and might already have
- * added some names to it. This function should not disturb any existing
- * names in 'all_names'.
- *
- * If a datapath class does not support multiple names for a datapath, this
- * function may be a null pointer.
- *
- * This is used by the vswitch at startup, */
- int (*get_all_names)(const struct dpif *dpif, struct svec *all_names);
-
/* Attempts to destroy the dpif underlying 'dpif'.
*
* If successful, 'dpif' will not be used again except as an argument for
diff --git a/lib/dpif.c b/lib/dpif.c
index 4795fa6..1209ee7 100644
--- a/lib/dpif.c
+++ b/lib/dpif.c
@@ -363,33 +363,6 @@ dpif_base_name(const struct dpif *dpif)
return dpif->base_name;
}
-/* Enumerates all names that may be used to open 'dpif' into 'all_names'. The
- * Linux datapath, for example, supports opening a datapath both by number,
- * e.g. "dp0", and by the name of the datapath's local port. For some
- * datapaths, this might be an infinite set (e.g. in a file name, slashes may
- * be duplicated any number of times), in which case only the names most likely
- * to be used will be enumerated.
- *
- * The caller must already have initialized 'all_names'. Any existing names in
- * 'all_names' will not be disturbed. */
-int
-dpif_get_all_names(const struct dpif *dpif, struct svec *all_names)
-{
- if (dpif->dpif_class->get_all_names) {
- int error = dpif->dpif_class->get_all_names(dpif, all_names);
- if (error) {
- VLOG_WARN_RL(&error_rl,
- "failed to retrieve names for datpath %s: %s",
- dpif_name(dpif), strerror(error));
- }
- return error;
- } else {
- svec_add(all_names, dpif_base_name(dpif));
- return 0;
- }
-}
-
-
/* Destroys the datapath that 'dpif' is connected to, first removing all of its
* ports. After calling this function, it does not make sense to pass 'dpif'
* to any functions other than dpif_name() or dpif_close(). */
diff --git a/lib/dpif.h b/lib/dpif.h
index 3534df7..0007f7d 100644
--- a/lib/dpif.h
+++ b/lib/dpif.h
@@ -54,7 +54,6 @@ void dpif_close(struct dpif *);
const char *dpif_name(const struct dpif *);
const char *dpif_base_name(const struct dpif *);
-int dpif_get_all_names(const struct dpif *, struct svec *);
int dpif_delete(struct dpif *);
diff --git a/vswitchd/bridge.c b/vswitchd/bridge.c
index 6bbb2ab..0aff59f 100644
--- a/vswitchd/bridge.c
+++ b/vswitchd/bridge.c
@@ -373,34 +373,22 @@ bridge_configure_once(const struct ovsrec_open_vswitch *cfg)
svec_init(&dpif_types);
dp_enumerate_types(&dpif_types);
for (i = 0; i < dpif_types.n; i++) {
- struct dpif *dpif;
- int retval;
size_t j;
dp_enumerate_names(dpif_types.names[i], &dpif_names);
- /* For each dpif... */
+ /* Delete each dpif whose name is not in 'bridge_names'. */
for (j = 0; j < dpif_names.n; j++) {
- retval = dpif_open(dpif_names.names[j], dpif_types.names[i], &dpif);
- if (!retval) {
- struct svec all_names;
- size_t k;
-
- /* ...check whether any of its names is in 'bridge_names'. */
- svec_init(&all_names);
- dpif_get_all_names(dpif, &all_names);
- for (k = 0; k < all_names.n; k++) {
- if (svec_contains(&bridge_names, all_names.names[k])) {
- goto found;
- }
+ if (!svec_contains(&bridge_names, dpif_names.names[j])) {
+ struct dpif *dpif;
+ int retval;
+
+ retval = dpif_open(dpif_names.names[j], dpif_types.names[i],
+ &dpif);
+ if (!retval) {
+ dpif_delete(dpif);
+ dpif_close(dpif);
}
-
- /* No. Delete the dpif. */
- dpif_delete(dpif);
-
- found:
- svec_destroy(&all_names);
- dpif_close(dpif);
}
}
}
--
1.7.1
More information about the dev
mailing list