Looks good to me, with a small nit/comment below,
Acked-by: Jarno Rajahalme <jrajaha...@nicira.com>
Jarno
On Jan 1, 2015, at 11:06 AM, Ben Pfaff <b...@nicira.com> wrote:
> The hidden rules created by in-band control and fail-open should not be
> included in the table stats reported via OpenFlow. I seem to recall that
> this was done correctly in some previous version but it has broken since
> then. This commit fixes the problem and adds a test that should make it
> harder to break again in the future.
>
> Reported-by: Ashok Chippa <a.n.chi...@gmail.com>
> Signed-off-by: Ben Pfaff <b...@nicira.com>
> ---
> ofproto/connmgr.c | 19 ++++++++++++++++++-
> ofproto/connmgr.h | 4 +++-
> ofproto/fail-open.c | 13 ++++++++++++-
> ofproto/fail-open.h | 4 +++-
> ofproto/in-band.c | 10 +++++++++-
> ofproto/in-band.h | 4 +++-
> ofproto/ofproto.c | 6 +++++-
> tests/ofproto.at | 52 ++++++++++++++++++++++++++++++++++++++++++++++++++++
> 8 files changed, 105 insertions(+), 7 deletions(-)
>
> diff --git a/ofproto/connmgr.c b/ofproto/connmgr.c
> index 08abe1e..3d69122 100644
> --- a/ofproto/connmgr.c
> +++ b/ofproto/connmgr.c
> @@ -1,5 +1,5 @@
> /*
> - * Copyright (c) 2009, 2010, 2011, 2012, 2013, 2014 Nicira, Inc.
> + * Copyright (c) 2009, 2010, 2011, 2012, 2013, 2014, 2015 Nicira, Inc.
> *
> * Licensed under the Apache License, Version 2.0 (the "License");
> * you may not use this file except in compliance with the License.
> @@ -1980,6 +1980,23 @@ connmgr_flushed(struct connmgr *mgr)
> ofpbuf_uninit(&ofpacts);
> }
> }
> +
> +/* Returns the number of hidden rules created by the in-band and fail-open
> + * implementations in table 0. (Subtracting this count from the number of
> + * rules in the table 0 classifier, as returned by classifier_count(), yields
> + * the number of flows that OVS should report via OpenFlow for table 0.) */
> +int
> +connmgr_count_hidden_rules(const struct connmgr *mgr)
> +{
> + int n_hidden = 0;
> + if (mgr->in_band) {
> + n_hidden += in_band_count_rules(mgr->in_band);
> + }
> + if (mgr->fail_open) {
> + n_hidden += fail_open_count_rules(mgr->fail_open);
> + }
> + return n_hidden;
> +}
>
> /* Creates a new ofservice for 'target' in 'mgr'. Returns 0 if successful,
> * otherwise a positive errno value.
> diff --git a/ofproto/connmgr.h b/ofproto/connmgr.h
> index c0f7c35..dd1a027 100644
> --- a/ofproto/connmgr.h
> +++ b/ofproto/connmgr.h
> @@ -1,5 +1,5 @@
> /*
> - * Copyright (c) 2009, 2010, 2011, 2012, 2013, 2014 Nicira, Inc.
> + * Copyright (c) 2009, 2010, 2011, 2012, 2013, 2014, 2015 Nicira, Inc.
> *
> * Licensed under the Apache License, Version 2.0 (the "License");
> * you may not use this file except in compliance with the License.
> @@ -193,6 +193,8 @@ bool connmgr_has_in_band(struct connmgr *);
> /* Fail-open and in-band implementation. */
> void connmgr_flushed(struct connmgr *);
>
> +int connmgr_count_hidden_rules(const struct connmgr *);
> +
> /* A flow monitor managed by NXST_FLOW_MONITOR and related requests. */
> struct ofmonitor {
> struct ofconn *ofconn; /* Owning 'ofconn'. */
> diff --git a/ofproto/fail-open.c b/ofproto/fail-open.c
> index ecdba44..d3faf10 100644
> --- a/ofproto/fail-open.c
> +++ b/ofproto/fail-open.c
> @@ -1,5 +1,5 @@
> /*
> - * Copyright (c) 2008, 2009, 2010, 2011, 2012, 2013 Nicira, Inc.
> + * Copyright (c) 2008, 2009, 2010, 2011, 2012, 2013, 2015 Nicira, Inc.
> *
> * Licensed under the Apache License, Version 2.0 (the "License");
> * you may not use this file except in compliance with the License.
> @@ -76,6 +76,7 @@ struct fail_open {
> int last_disconn_secs;
> long long int next_bogus_packet_in;
> struct rconn_packet_counter *bogus_packet_counter;
> + bool fail_open_active;
> };
>
> static void fail_open_recover(struct fail_open *);
> @@ -234,6 +235,15 @@ fail_open_flushed(struct fail_open *fo)
>
> ofpbuf_uninit(&ofpacts);
> }
> + fo->fail_open_active = open;
> +}
> +
> +/* Returns the number of fail-open rules currently installed in the flow
> + * table. */
> +int
> +fail_open_count_rules(const struct fail_open *fo)
> +{
> + return fo->fail_open_active;
I’m not sure this is in line with CodingStyle, but it may be that CodingStyle
could be updated now that we compile with C99?
> }
>
> /* Creates and returns a new struct fail_open for 'ofproto' and 'mgr'. */
> @@ -246,6 +256,7 @@ fail_open_create(struct ofproto *ofproto, struct connmgr
> *mgr)
> fo->last_disconn_secs = 0;
> fo->next_bogus_packet_in = LLONG_MAX;
> fo->bogus_packet_counter = rconn_packet_counter_create();
> + fo->fail_open_active = false;
> return fo;
> }
>
> diff --git a/ofproto/fail-open.h b/ofproto/fail-open.h
> index 725b82d..4056b3e 100644
> --- a/ofproto/fail-open.h
> +++ b/ofproto/fail-open.h
> @@ -1,5 +1,5 @@
> /*
> - * Copyright (c) 2008, 2009, 2010, 2011, 2013 Nicira, Inc.
> + * Copyright (c) 2008, 2009, 2010, 2011, 2013, 2015 Nicira, Inc.
> *
> * Licensed under the Apache License, Version 2.0 (the "License");
> * you may not use this file except in compliance with the License.
> @@ -47,4 +47,6 @@ void fail_open_run(struct fail_open *);
> void fail_open_maybe_recover(struct fail_open *) OVS_EXCLUDED(ofproto_mutex);
> void fail_open_flushed(struct fail_open *) OVS_EXCLUDED(ofproto_mutex);
>
> +int fail_open_count_rules(const struct fail_open *);
> +
> #endif /* fail-open.h */
> diff --git a/ofproto/in-band.c b/ofproto/in-band.c
> index bde893a..01950fa 100644
> --- a/ofproto/in-band.c
> +++ b/ofproto/in-band.c
> @@ -1,5 +1,5 @@
> /*
> - * Copyright (c) 2008, 2009, 2010, 2011, 2012, 2013, 2014 Nicira, Inc.
> + * Copyright (c) 2008, 2009, 2010, 2011, 2012, 2013, 2014, 2015 Nicira, Inc.
> *
> * Licensed under the Apache License, Version 2.0 (the "License");
> * you may not use this file except in compliance with the License.
> @@ -235,6 +235,14 @@ in_band_must_output_to_local_port(const struct flow
> *flow)
> && flow->tp_dst == htons(DHCP_CLIENT_PORT));
> }
>
> +/* Returns the number of in-band rules currently installed in the flow
> + * table. */
> +int
> +in_band_count_rules(const struct in_band *ib)
> +{
> + return hmap_count(&ib->rules);
> +}
> +
> static void
> add_rule(struct in_band *ib, const struct match *match, int priority)
> {
> diff --git a/ofproto/in-band.h b/ofproto/in-band.h
> index ad16dc2..6e0585a 100644
> --- a/ofproto/in-band.h
> +++ b/ofproto/in-band.h
> @@ -1,5 +1,5 @@
> /*
> - * Copyright (c) 2008, 2009, 2010, 2011, 2013 Nicira, Inc.
> + * Copyright (c) 2008, 2009, 2010, 2011, 2013, 2015 Nicira, Inc.
> *
> * Licensed under the Apache License, Version 2.0 (the "License");
> * you may not use this file except in compliance with the License.
> @@ -42,4 +42,6 @@ void in_band_wait(struct in_band *);
>
> bool in_band_must_output_to_local_port(const struct flow *);
>
> +int in_band_count_rules(const struct in_band *);
> +
> #endif /* in-band.h */
> diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
> index dc7b551..e4370e7 100644
> --- a/ofproto/ofproto.c
> +++ b/ofproto/ofproto.c
> @@ -1,5 +1,5 @@
> /*
> - * Copyright (c) 2009, 2010, 2011, 2012, 2013, 2014 Nicira, Inc.
> + * Copyright (c) 2009, 2010, 2011, 2012, 2013, 2014, 2015 Nicira, Inc.
> * Copyright (c) 2010 Jean Tourrilhes - HP-Labs.
> *
> * Licensed under the Apache License, Version 2.0 (the "License");
> @@ -2958,6 +2958,10 @@ query_tables(struct ofproto *ofproto,
>
> s->table_id = i;
> s->active_count = classifier_count(cls);
> + if (i == 0) {
> + s->active_count -= connmgr_count_hidden_rules(
> + ofproto->connmgr);
> + }
> }
> } else {
> stats = NULL;
> diff --git a/tests/ofproto.at b/tests/ofproto.at
> index 8cfecc6..a645795 100644
> --- a/tests/ofproto.at
> +++ b/tests/ofproto.at
> @@ -1306,6 +1306,58 @@ AT_CHECK([ovs-ofctl dump-tables br0], [0], [expout])
> OVS_VSWITCHD_STOP
> AT_CLEANUP
>
> +dnl In-band and fail-open add "hidden rules" to table 0. These rules
> shouldn't
> +dnl be visible to OpenFlow. This test checks that "dump-flows" and
> +dnl "dump-tables" don't make them visible.
> +AT_SETUP([ofproto - hidden rules not in table stats])
> +# Use an IP address for a controller that won't actually exist: we
> +# want to create in-band rules but we do not want to actually connect
> +# to a controller (because that could mess about with our test). The
> +# Class E range 240.0.0.0 - 255.255.255.255 seems like a good choice.
> +OVS_VSWITCHD_START([set-controller br0 tcp:240.0.0.1:6653])
> +for i in 1 2 3 4 5; do ovs-appctl time/warp 1000; done
> +
> +# Check that no hidden flows are visible in OpenFlow.
> +AT_CHECK([ovs-ofctl dump-flows br0], [0], [NXST_FLOW reply (xid=0x4):
> +])
> +
> +# Check that some hidden flows related to 240.0.0.1 are actually in table 0.
> +#
> +# We discard flows that mention table_id because we only want table 0 flows,
> +# which in OVS is implied by the absence of a table_id.
> +AT_CHECK([ovs-appctl bridge/dump-flows br0], [0], [stdout])
> +AT_CHECK([test `grep '240\.0\.0\.1' stdout | grep -v table_id= | wc -l` -gt
> 0])
> +
> +# Check that dump-tables doesn't count the hidden flows.
> +(printf "OFPST_TABLE reply (xid=0x2):"
> + x=0
> + name=classifier
> + while test $x -lt 254; do
> + printf "
> + table %d (\"%s\"):
> + active=0, lookup=0, matched=0
> + max_entries=1000000
> + matching:
> + in_port: exact match or wildcard
> + eth_src: exact match or wildcard
> + eth_dst: exact match or wildcard
> + eth_type: exact match or wildcard
> + vlan_vid: exact match or wildcard
> + vlan_pcp: exact match or wildcard
> + ip_src: exact match or wildcard
> + ip_dst: exact match or wildcard
> + nw_proto: exact match or wildcard
> + nw_tos: exact match or wildcard
> + tcp_src: exact match or wildcard
> + tcp_dst: exact match or wildcard
> +" $x $name
> + x=`expr $x + 1`
> + name=table$x
> + done) > expout
> +AT_CHECK([ovs-ofctl dump-tables br0], [0], [expout])
> +OVS_VSWITCHD_STOP(["/cannot find route for controller/d"])
> +AT_CLEANUP
> +
> AT_SETUP([ofproto - flow table configuration (OpenFlow 1.2)])
> OVS_VSWITCHD_START
> # Check the default configuration.
> --
> 2.1.3
>
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
