[ovs-dev] [PATCH v4 2/2] ovn-trace: New --ovs option to also print OpenFlow flows.

Justin Pettit jpettit at ovn.org
Wed Dec 28 07:13:01 UTC 2016


> On Dec 21, 2016, at 3:25 PM, Ben Pfaff <blp at ovn.org> wrote:
> 
> Sometimes seeing the OpenFlow flows that back a given logical flow can
> provide additional insight.  This commit adds a new --ovs option to
> ovn-trace that makes it connect to Open vSwitch over OpenFlow and retrieve
> and print the OpenFlow flows behind each logical flow encountered during
> a trace.

Neat!

> diff --git a/include/openvswitch/vconn.h b/include/openvswitch/vconn.h
> index d2fbd16..acdc79c 100644
> --- a/include/openvswitch/vconn.h
> +++ b/include/openvswitch/vconn.h
> @@ -1,5 +1,5 @@
> /*
> - * Copyright (c) 2008, 2009, 2010, 2011, 2012, 2013, 2014, 2015 Nicira, Inc.
> + * Copyright (c) 2008, 2009, 2010, 2011, 2012, 2013, 2014, 2015, 2016 Nicira, Inc.
>  *
>  * Licensed under the Apache License, Version 2.0 (the "License");
>  * you may not use this file except in compliance with the License.
> @@ -18,9 +18,10 @@
> #define OPENVSWITCH_VCONN_H 1
> 
> #include <stdbool.h>
> -#include <openvswitch/list.h>
> -#include <openvswitch/types.h>
> -#include <openflow/openflow.h>
> +#include "openvswitch/list.h"
> +#include "openvswitch/types.h"
> +#include "openvswitch/ofp-util.h"
> +#include "openflow/openflow.h"
> 
> #ifdef __cplusplus
> extern "C" {
> @@ -31,6 +32,8 @@ struct pvconn;
> struct pvconn_class;
> struct vconn;
> struct vconn_class;
> +struct ofputil_flow_stats;
> +struct ofputil_flow_stats_request;

Aren't these structures defined in "ofp-util.h", which you included above?

> diff --git a/lib/vconn.c b/lib/vconn.c
> index d5a17f6..57cf429 100644
> --- a/lib/vconn.c
> +++ b/lib/vconn.c
> @@ -1,5 +1,5 @@
> /*
> - * Copyright (c) 2008, 2009, 2010, 2011, 2012, 2013, 2014, 2015 Nicira, Inc.
> + * Copyright (c) 2008-2016 Nicira, Inc.

I thought you had some legal concern about specifying ranges for copyright years.  Regardless, it's a nit, but you add 2016 to "vconn.h" in this same commit, but didn't use a range.

> +/* Send 'request' to 'vconn', encoding it with the given 'protocol', and then
> + * waits for, parses, and accumulates all of the replies into '*fsesp' and
> + * '*n_fsesp'.  The caller is responsible for freeing all of the flows.
> + * Returns 0 if successful, otherwise a positive errno value.  Always frees
> + * 'request'. */

I think "request" should be "fsr" in the comment.  It says that it frees "request" (fsr?), but I'm not sure that's true.

> +int
> +vconn_dump_flows(struct vconn *vconn,
> +                 const struct ofputil_flow_stats_request *fsr,
> +                 enum ofputil_protocol protocol,
> +                 struct ofputil_flow_stats **fsesp, size_t *n_fsesp)
> +{
> +    struct ofputil_flow_stats *fses = NULL;
> +    size_t n_fses = 0;
> +    size_t allocated_fses = 0;
> +
> +    struct ofpbuf *request = ofputil_encode_flow_stats_request(fsr, protocol);
> +    const struct ofp_header *oh = request->data;
> +    ovs_be32 send_xid = oh->xid;
> +    int error = vconn_send_block(vconn, request);
> +    if (error) {
> +        goto exit;
> +    }
> +
> +    struct ofpbuf *reply = NULL;
> +    struct ofpbuf ofpacts;
> +    ofpbuf_init(&ofpacts, 0);
> +    for (;;) {
> +        if (n_fses >= allocated_fses) {
> +            fses = x2nrealloc(fses, &allocated_fses, sizeof *fses);
> +        }
> +
> +        struct ofputil_flow_stats *fs = &fses[n_fses];
> +        error = recv_flow_stats_reply(vconn, send_xid, &reply, fs, &ofpacts);
> +        if (error) {
> +            if (error == EOF) {
> +                error = 0;
> +            }
> +            break;
> +        }
> +        fs->ofpacts = xmemdup(fs->ofpacts, fs->ofpacts_len);
> +        n_fses++;
> +    }
> +    ofpbuf_uninit(&ofpacts);
> +    ofpbuf_delete(reply);
> +
> +    if (error) {
> +        for (size_t i = 0; i < n_fses; i++) {
> +            free(CONST_CAST(struct ofpact *, fses[i].ofpacts));
> +        }
> +        free(fses);
> +
> +        fses = NULL;
> +        n_fses = 0;
> +    }
> +
> +exit:
> +    *fsesp = fses;
> +    *n_fsesp = n_fses;
> +    return error;
> +}

Acked-by: Justin Pettit <jpettit at ovn.org>

--Justin





More information about the dev mailing list