[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