[ovs-dev] [next 32/35] ofproto: Improve abstraction by adding function ofproto_parse_name().
Ethan Jackson
ethan at nicira.com
Fri May 6 21:45:06 UTC 2011
Looks Good.
I think creating a whole c file for ofproto_parse_name looks a bit
strange/premature-optimization-ish to me. Perhaps a simple inline
function in the header file would be simpler? Your call.
Ethan
Ethan
On Tue, Apr 26, 2011 at 09:24, Ben Pfaff <blp at nicira.com> wrote:
> This means that ovs-ofctl and ovs-openflowd don't have to use the dpif
> layer at all, making it easier to change the ofproto implementation.
> ---
> ofproto/automake.mk | 1 +
> ofproto/names.c | 35 +++++++++++++++++++++++++++++++++++
> ofproto/ofproto.h | 2 ++
> utilities/automake.mk | 5 ++++-
> utilities/ovs-ofctl.c | 37 ++++++++++++-------------------------
> utilities/ovs-openflowd.c | 3 +--
> 6 files changed, 55 insertions(+), 28 deletions(-)
> create mode 100644 ofproto/names.c
>
> diff --git a/ofproto/automake.mk b/ofproto/automake.mk
> index ef45e9f..8153551 100644
> --- a/ofproto/automake.mk
> +++ b/ofproto/automake.mk
> @@ -15,6 +15,7 @@ ofproto_libofproto_a_SOURCES = \
> ofproto/fail-open.h \
> ofproto/in-band.c \
> ofproto/in-band.h \
> + ofproto/names.c \
> ofproto/netflow.c \
> ofproto/netflow.h \
> ofproto/ofproto.c \
> diff --git a/ofproto/names.c b/ofproto/names.c
> new file mode 100644
> index 0000000..724b184
> --- /dev/null
> +++ b/ofproto/names.c
> @@ -0,0 +1,35 @@
> +/*
> + * Copyright (c) 2011 Nicira Networks.
> + *
> + * Licensed under the Apache License, Version 2.0 (the "License");
> + * you may not use this file except in compliance with the License.
> + * You may obtain a copy of the License at:
> + *
> + * http://www.apache.org/licenses/LICENSE-2.0
> + *
> + * Unless required by applicable law or agreed to in writing, software
> + * distributed under the License is distributed on an "AS IS" BASIS,
> + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
> + * See the License for the specific language governing permissions and
> + * limitations under the License.
> + */
> +
> +#include <config.h>
> +
> +#include "ofproto/ofproto.h"
> +
> +#include "dpif.h"
> +
> +/* This function is in a separate file because it is the only ofproto function
> + * required by ovs-ofctl, which allows ovs-ofctl to avoid linking in extra
> + * baggage. */
> +
> +/* Parses 'ofproto_name', which is of the form [type@]name into component
> + * pieces that are suitable for passing to ofproto_create(). The caller must
> + * free 'name' and 'type'. */
> +void
> +ofproto_parse_name(const char *ofproto_name, char **name, char **type)
> +{
> + dp_parse_name(ofproto_name, name, type);
> +}
> +
> diff --git a/ofproto/ofproto.h b/ofproto/ofproto.h
> index cbdcd98..8751baa 100644
> --- a/ofproto/ofproto.h
> +++ b/ofproto/ofproto.h
> @@ -93,6 +93,8 @@ struct ofproto_controller {
> #define DEFAULT_SERIAL_DESC "None"
> #define DEFAULT_DP_DESC "None"
>
> +void ofproto_parse_name(const char *name, char **dp_name, char **dp_type);
> +
> int ofproto_create(const char *datapath, const char *datapath_type,
> struct ofproto **ofprotop);
> void ofproto_destroy(struct ofproto *);
> diff --git a/utilities/automake.mk b/utilities/automake.mk
> index fc3a19d..cb06f5a 100644
> --- a/utilities/automake.mk
> +++ b/utilities/automake.mk
> @@ -78,7 +78,10 @@ utilities_ovs_dpctl_SOURCES = utilities/ovs-dpctl.c
> utilities_ovs_dpctl_LDADD = lib/libopenvswitch.a
>
> utilities_ovs_ofctl_SOURCES = utilities/ovs-ofctl.c
> -utilities_ovs_ofctl_LDADD = lib/libopenvswitch.a $(SSL_LIBS)
> +utilities_ovs_ofctl_LDADD = \
> + ofproto/libofproto.a \
> + lib/libopenvswitch.a \
> + $(SSL_LIBS)
>
> utilities_ovs_openflowd_SOURCES = utilities/ovs-openflowd.c
> utilities_ovs_openflowd_LDADD = \
> diff --git a/utilities/ovs-ofctl.c b/utilities/ovs-ofctl.c
> index bc8cc6b..660fc3a 100644
> --- a/utilities/ovs-ofctl.c
> +++ b/utilities/ovs-ofctl.c
> @@ -32,7 +32,6 @@
> #include "command-line.h"
> #include "compiler.h"
> #include "dirs.h"
> -#include "dpif.h"
> #include "dynamic-string.h"
> #include "netlink.h"
> #include "nx-match.h"
> @@ -41,6 +40,7 @@
> #include "ofp-print.h"
> #include "ofp-util.h"
> #include "ofpbuf.h"
> +#include "ofproto/ofproto.h"
> #include "openflow/nicira-ext.h"
> #include "openflow/openflow.h"
> #include "random.h"
> @@ -220,12 +220,17 @@ static void
> open_vconn__(const char *name, const char *default_suffix,
> struct vconn **vconnp)
> {
> - struct dpif *dpif;
> + char *datapath_name, *datapath_type, *socket_name;
> + char *bridge_path;
> struct stat s;
> - char *bridge_path, *datapath_name, *datapath_type;
>
> bridge_path = xasprintf("%s/%s.%s", ovs_rundir(), name, default_suffix);
> - dp_parse_name(name, &datapath_name, &datapath_type);
> +
> + ofproto_parse_name(name, &datapath_name, &datapath_type);
> + socket_name = xasprintf("%s/%s.%s",
> + ovs_rundir(), datapath_name, default_suffix);
> + free(datapath_name);
> + free(datapath_type);
>
> if (strstr(name, ":")) {
> run(vconn_open_block(name, OFP_VERSION, vconnp),
> @@ -234,36 +239,18 @@ open_vconn__(const char *name, const char *default_suffix,
> open_vconn_socket(name, vconnp);
> } else if (!stat(bridge_path, &s) && S_ISSOCK(s.st_mode)) {
> open_vconn_socket(bridge_path, vconnp);
> - } else if (!dpif_open(datapath_name, datapath_type, &dpif)) {
> - char dpif_name[IF_NAMESIZE + 1];
> - char *socket_name;
> -
> - run(dpif_port_get_name(dpif, ODPP_LOCAL, dpif_name, sizeof dpif_name),
> - "obtaining name of %s", dpif_name);
> - dpif_close(dpif);
> - if (strcmp(dpif_name, name)) {
> - VLOG_DBG("datapath %s is named %s", name, dpif_name);
> - }
> -
> - socket_name = xasprintf("%s/%s.%s",
> - ovs_rundir(), dpif_name, default_suffix);
> - if (stat(socket_name, &s)) {
> - ovs_fatal(errno, "cannot connect to %s: stat failed on %s",
> - name, socket_name);
> - } else if (!S_ISSOCK(s.st_mode)) {
> + } else if (!stat(socket_name, &s)) {
> + if (!S_ISSOCK(s.st_mode)) {
> ovs_fatal(0, "cannot connect to %s: %s is not a socket",
> name, socket_name);
> }
> -
> open_vconn_socket(socket_name, vconnp);
> - free(socket_name);
> } else {
> ovs_fatal(0, "%s is not a valid connection method", name);
> }
>
> - free(datapath_name);
> - free(datapath_type);
> free(bridge_path);
> + free(socket_name);
> }
>
> static void
> diff --git a/utilities/ovs-openflowd.c b/utilities/ovs-openflowd.c
> index 4e83036..2d2446e 100644
> --- a/utilities/ovs-openflowd.c
> +++ b/utilities/ovs-openflowd.c
> @@ -28,7 +28,6 @@
> #include "compiler.h"
> #include "daemon.h"
> #include "dirs.h"
> -#include "dpif.h"
> #include "dummy.h"
> #include "leak-checker.h"
> #include "list.h"
> @@ -472,7 +471,7 @@ parse_options(int argc, char *argv[], struct ofsettings *s)
> }
>
> /* Local vconns. */
> - dp_parse_name(argv[0], &s->dp_name, &s->dp_type);
> + ofproto_parse_name(argv[0], &s->dp_name, &s->dp_type);
>
> /* Figure out controller names. */
> s->run_forever = false;
> --
> 1.7.4.4
>
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev
>
More information about the dev
mailing list