| Message ID | 1447270794-21103-6-git-send-email-joestringer@nicira.com |
|---|---|
| State | Awaiting Upstream |
| Headers | show |
With a comment below, Acked-by: Jarno Rajahalme <jarno@ovn.org> > On Nov 11, 2015, at 11:39 AM, Joe Stringer <joestringer@nicira.com> wrote: > > Add an ofproto-level function to allow implementations to reject > specific action types based on internal implementation details. The > first user will be the next patch, which checks for datapath (kernel) > support for various aspects of connection tracking and uses this to > allow or reject ct() actions. > > Signed-off-by: Joe Stringer <joestringer@nicira.com> > CC: Jarno Rajahalme <jarno@ovn.org> > --- > ofproto/ofproto-dpif.c | 1 + > ofproto/ofproto-provider.h | 14 ++++++++++++++ > ofproto/ofproto.c | 9 +++++++++ > 3 files changed, 24 insertions(+) > > diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c > index f0a2ca59e2e8..8b1760c95409 100644 > --- a/ofproto/ofproto-dpif.c > +++ b/ofproto/ofproto-dpif.c > @@ -5689,6 +5689,7 @@ const struct ofproto_class ofproto_dpif_class = { > port_poll_wait, > port_is_lacp_current, > port_get_lacp_stats, > + NULL, > NULL, /* rule_choose_table */ > rule_alloc, > rule_construct, > diff --git a/ofproto/ofproto-provider.h b/ofproto/ofproto-provider.h > index 117cd1fcc02e..da6cb37c0b60 100644 > --- a/ofproto/ofproto-provider.h > +++ b/ofproto/ofproto-provider.h > @@ -1077,6 +1077,20 @@ struct ofproto_class { > /* ## OpenFlow Rule Functions ## */ > /* ## ----------------------- ## */ > > + /* Checks whether the action 'ofpact' is supported by 'ofproto'. On > + * success, returns 0. On failure, returns an OpenFlow error code. > + * > + * Some actions are marked as optional by the OpenFlow specification. > + * Furthermore, OVS includes support for several vendor extensions which > + * may not be supported by all ofproto implementations. This function > + * allows specific actions to be rejected based on internal datapath > + * support. Failure implies that an OpenFlow rule that includes 'ofpact' > + * in its actions can never be inserted into 'ofproto'. > + * > + * If this function is NULL then all actions are supported. */ > + enum ofperr (*rule_check_action)(const struct ofproto *ofproto, > + const struct ofpact *ofpact); > + > /* Chooses an appropriate table for 'match' within 'ofproto'. On > * success, stores the table ID into '*table_idp' and returns 0. On > * failure, returns an OpenFlow error code. > diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c > index c7dd8a2c991b..338330108df1 100644 > --- a/ofproto/ofproto.c > +++ b/ofproto/ofproto.c > @@ -3346,10 +3346,19 @@ ofproto_check_ofpacts(struct ofproto *ofproto, > } > > OFPACT_FOR_EACH (a, ofpacts, ofpacts_len) { > + enum ofperr error; > + > if (a->type == OFPACT_GROUP > && !ofproto_group_exists(ofproto, ofpact_get_GROUP(a)->group_id)) { > return OFPERR_OFPBAC_BAD_OUT_GROUP; > } > + > + if (ofproto->ofproto_class->rule_check_action) { > + error = ofproto->ofproto_class->rule_check_action(ofproto, a); > + if (error) { > + return error; > + } > + } I would make this call for ‘known to possibly not be supported’ actions, in the spirit of the group check above. > } > > return 0; > -- > 2.1.4 >
I would urge Ben to check this up as well, though. Jarno > On Nov 11, 2015, at 2:23 PM, Jarno Rajahalme <jarno@ovn.org> wrote: > > With a comment below, > > Acked-by: Jarno Rajahalme <jarno@ovn.org> > >> On Nov 11, 2015, at 11:39 AM, Joe Stringer <joestringer@nicira.com> wrote: >> >> Add an ofproto-level function to allow implementations to reject >> specific action types based on internal implementation details. The >> first user will be the next patch, which checks for datapath (kernel) >> support for various aspects of connection tracking and uses this to >> allow or reject ct() actions. >> >> Signed-off-by: Joe Stringer <joestringer@nicira.com> >> CC: Jarno Rajahalme <jarno@ovn.org> >> --- >> ofproto/ofproto-dpif.c | 1 + >> ofproto/ofproto-provider.h | 14 ++++++++++++++ >> ofproto/ofproto.c | 9 +++++++++ >> 3 files changed, 24 insertions(+) >> >> diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c >> index f0a2ca59e2e8..8b1760c95409 100644 >> --- a/ofproto/ofproto-dpif.c >> +++ b/ofproto/ofproto-dpif.c >> @@ -5689,6 +5689,7 @@ const struct ofproto_class ofproto_dpif_class = { >> port_poll_wait, >> port_is_lacp_current, >> port_get_lacp_stats, >> + NULL, >> NULL, /* rule_choose_table */ >> rule_alloc, >> rule_construct, >> diff --git a/ofproto/ofproto-provider.h b/ofproto/ofproto-provider.h >> index 117cd1fcc02e..da6cb37c0b60 100644 >> --- a/ofproto/ofproto-provider.h >> +++ b/ofproto/ofproto-provider.h >> @@ -1077,6 +1077,20 @@ struct ofproto_class { >> /* ## OpenFlow Rule Functions ## */ >> /* ## ----------------------- ## */ >> >> + /* Checks whether the action 'ofpact' is supported by 'ofproto'. On >> + * success, returns 0. On failure, returns an OpenFlow error code. >> + * >> + * Some actions are marked as optional by the OpenFlow specification. >> + * Furthermore, OVS includes support for several vendor extensions which >> + * may not be supported by all ofproto implementations. This function >> + * allows specific actions to be rejected based on internal datapath >> + * support. Failure implies that an OpenFlow rule that includes 'ofpact' >> + * in its actions can never be inserted into 'ofproto'. >> + * >> + * If this function is NULL then all actions are supported. */ >> + enum ofperr (*rule_check_action)(const struct ofproto *ofproto, >> + const struct ofpact *ofpact); >> + >> /* Chooses an appropriate table for 'match' within 'ofproto'. On >> * success, stores the table ID into '*table_idp' and returns 0. On >> * failure, returns an OpenFlow error code. >> diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c >> index c7dd8a2c991b..338330108df1 100644 >> --- a/ofproto/ofproto.c >> +++ b/ofproto/ofproto.c >> @@ -3346,10 +3346,19 @@ ofproto_check_ofpacts(struct ofproto *ofproto, >> } >> >> OFPACT_FOR_EACH (a, ofpacts, ofpacts_len) { >> + enum ofperr error; >> + >> if (a->type == OFPACT_GROUP >> && !ofproto_group_exists(ofproto, ofpact_get_GROUP(a)->group_id)) { >> return OFPERR_OFPBAC_BAD_OUT_GROUP; >> } >> + >> + if (ofproto->ofproto_class->rule_check_action) { >> + error = ofproto->ofproto_class->rule_check_action(ofproto, a); >> + if (error) { >> + return error; >> + } >> + } > > I would make this call for ‘known to possibly not be supported’ actions, in the spirit of the group check above. > >> } >> >> return 0; >> -- >> 2.1.4 >> >
On 11 November 2015 at 14:23, Jarno Rajahalme <jarno@ovn.org> wrote: > With a comment below, > > Acked-by: Jarno Rajahalme <jarno@ovn.org> > >> On Nov 11, 2015, at 11:39 AM, Joe Stringer <joestringer@nicira.com> wrote: >> >> Add an ofproto-level function to allow implementations to reject >> specific action types based on internal implementation details. The >> first user will be the next patch, which checks for datapath (kernel) >> support for various aspects of connection tracking and uses this to >> allow or reject ct() actions. >> >> Signed-off-by: Joe Stringer <joestringer@nicira.com> >> CC: Jarno Rajahalme <jarno@ovn.org> >> --- >> ofproto/ofproto-dpif.c | 1 + >> ofproto/ofproto-provider.h | 14 ++++++++++++++ >> ofproto/ofproto.c | 9 +++++++++ >> 3 files changed, 24 insertions(+) >> >> diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c >> index f0a2ca59e2e8..8b1760c95409 100644 >> --- a/ofproto/ofproto-dpif.c >> +++ b/ofproto/ofproto-dpif.c >> @@ -5689,6 +5689,7 @@ const struct ofproto_class ofproto_dpif_class = { >> port_poll_wait, >> port_is_lacp_current, >> port_get_lacp_stats, >> + NULL, >> NULL, /* rule_choose_table */ >> rule_alloc, >> rule_construct, >> diff --git a/ofproto/ofproto-provider.h b/ofproto/ofproto-provider.h >> index 117cd1fcc02e..da6cb37c0b60 100644 >> --- a/ofproto/ofproto-provider.h >> +++ b/ofproto/ofproto-provider.h >> @@ -1077,6 +1077,20 @@ struct ofproto_class { >> /* ## OpenFlow Rule Functions ## */ >> /* ## ----------------------- ## */ >> >> + /* Checks whether the action 'ofpact' is supported by 'ofproto'. On >> + * success, returns 0. On failure, returns an OpenFlow error code. >> + * >> + * Some actions are marked as optional by the OpenFlow specification. >> + * Furthermore, OVS includes support for several vendor extensions which >> + * may not be supported by all ofproto implementations. This function >> + * allows specific actions to be rejected based on internal datapath >> + * support. Failure implies that an OpenFlow rule that includes 'ofpact' >> + * in its actions can never be inserted into 'ofproto'. >> + * >> + * If this function is NULL then all actions are supported. */ >> + enum ofperr (*rule_check_action)(const struct ofproto *ofproto, >> + const struct ofpact *ofpact); >> + >> /* Chooses an appropriate table for 'match' within 'ofproto'. On >> * success, stores the table ID into '*table_idp' and returns 0. On >> * failure, returns an OpenFlow error code. >> diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c >> index c7dd8a2c991b..338330108df1 100644 >> --- a/ofproto/ofproto.c >> +++ b/ofproto/ofproto.c >> @@ -3346,10 +3346,19 @@ ofproto_check_ofpacts(struct ofproto *ofproto, >> } >> >> OFPACT_FOR_EACH (a, ofpacts, ofpacts_len) { >> + enum ofperr error; >> + >> if (a->type == OFPACT_GROUP >> && !ofproto_group_exists(ofproto, ofpact_get_GROUP(a)->group_id)) { >> return OFPERR_OFPBAC_BAD_OUT_GROUP; >> } >> + >> + if (ofproto->ofproto_class->rule_check_action) { >> + error = ofproto->ofproto_class->rule_check_action(ofproto, a); >> + if (error) { >> + return error; >> + } >> + } > > I would make this call for ‘known to possibly not be supported’ actions, in the spirit of the group check above. I agree, this would make it more efficient.
On Wed, Nov 11, 2015 at 11:39:53AM -0800, Joe Stringer wrote: > Add an ofproto-level function to allow implementations to reject > specific action types based on internal implementation details. The > first user will be the next patch, which checks for datapath (kernel) > support for various aspects of connection tracking and uses this to > allow or reject ct() actions. > > Signed-off-by: Joe Stringer <joestringer@nicira.com> > CC: Jarno Rajahalme <jarno@ovn.org> > --- > ofproto/ofproto-dpif.c | 1 + > ofproto/ofproto-provider.h | 14 ++++++++++++++ > ofproto/ofproto.c | 9 +++++++++ > 3 files changed, 24 insertions(+) > > diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c > index f0a2ca59e2e8..8b1760c95409 100644 > --- a/ofproto/ofproto-dpif.c > +++ b/ofproto/ofproto-dpif.c > @@ -5689,6 +5689,7 @@ const struct ofproto_class ofproto_dpif_class = { > port_poll_wait, > port_is_lacp_current, > port_get_lacp_stats, > + NULL, > NULL, /* rule_choose_table */ > rule_alloc, > rule_construct, > diff --git a/ofproto/ofproto-provider.h b/ofproto/ofproto-provider.h > index 117cd1fcc02e..da6cb37c0b60 100644 > --- a/ofproto/ofproto-provider.h > +++ b/ofproto/ofproto-provider.h > @@ -1077,6 +1077,20 @@ struct ofproto_class { > /* ## OpenFlow Rule Functions ## */ > /* ## ----------------------- ## */ > > + /* Checks whether the action 'ofpact' is supported by 'ofproto'. On > + * success, returns 0. On failure, returns an OpenFlow error code. > + * > + * Some actions are marked as optional by the OpenFlow specification. > + * Furthermore, OVS includes support for several vendor extensions which > + * may not be supported by all ofproto implementations. This function > + * allows specific actions to be rejected based on internal datapath > + * support. Failure implies that an OpenFlow rule that includes 'ofpact' > + * in its actions can never be inserted into 'ofproto'. > + * > + * If this function is NULL then all actions are supported. */ > + enum ofperr (*rule_check_action)(const struct ofproto *ofproto, > + const struct ofpact *ofpact); Checking that the datapath can implement the actions is supposed to be part of the job of ->rule_construct(), see this comment on ofproto-provider.h: * ->rule_construct() must also: * * - Validate that the datapath supports the matching rule in 'rule->cr' * datapath. For example, if the rule's table does not support * registers, then it is an error if 'rule->cr' does not wildcard all * registers. * * - Validate that the datapath can correctly implement 'rule->ofpacts'.
On 24 November 2015 at 21:45, Ben Pfaff <blp@ovn.org> wrote: > On Wed, Nov 11, 2015 at 11:39:53AM -0800, Joe Stringer wrote: >> Add an ofproto-level function to allow implementations to reject >> specific action types based on internal implementation details. The >> first user will be the next patch, which checks for datapath (kernel) >> support for various aspects of connection tracking and uses this to >> allow or reject ct() actions. >> >> Signed-off-by: Joe Stringer <joestringer@nicira.com> >> CC: Jarno Rajahalme <jarno@ovn.org> >> --- >> ofproto/ofproto-dpif.c | 1 + >> ofproto/ofproto-provider.h | 14 ++++++++++++++ >> ofproto/ofproto.c | 9 +++++++++ >> 3 files changed, 24 insertions(+) >> >> diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c >> index f0a2ca59e2e8..8b1760c95409 100644 >> --- a/ofproto/ofproto-dpif.c >> +++ b/ofproto/ofproto-dpif.c >> @@ -5689,6 +5689,7 @@ const struct ofproto_class ofproto_dpif_class = { >> port_poll_wait, >> port_is_lacp_current, >> port_get_lacp_stats, >> + NULL, >> NULL, /* rule_choose_table */ >> rule_alloc, >> rule_construct, >> diff --git a/ofproto/ofproto-provider.h b/ofproto/ofproto-provider.h >> index 117cd1fcc02e..da6cb37c0b60 100644 >> --- a/ofproto/ofproto-provider.h >> +++ b/ofproto/ofproto-provider.h >> @@ -1077,6 +1077,20 @@ struct ofproto_class { >> /* ## OpenFlow Rule Functions ## */ >> /* ## ----------------------- ## */ >> >> + /* Checks whether the action 'ofpact' is supported by 'ofproto'. On >> + * success, returns 0. On failure, returns an OpenFlow error code. >> + * >> + * Some actions are marked as optional by the OpenFlow specification. >> + * Furthermore, OVS includes support for several vendor extensions which >> + * may not be supported by all ofproto implementations. This function >> + * allows specific actions to be rejected based on internal datapath >> + * support. Failure implies that an OpenFlow rule that includes 'ofpact' >> + * in its actions can never be inserted into 'ofproto'. >> + * >> + * If this function is NULL then all actions are supported. */ >> + enum ofperr (*rule_check_action)(const struct ofproto *ofproto, >> + const struct ofpact *ofpact); > > Checking that the datapath can implement the actions is supposed to be > part of the job of ->rule_construct(), see this comment on > ofproto-provider.h: > > * ->rule_construct() must also: > * > * - Validate that the datapath supports the matching rule in 'rule->cr' > * datapath. For example, if the rule's table does not support > * registers, then it is an error if 'rule->cr' does not wildcard all > * registers. > * > * - Validate that the datapath can correctly implement 'rule->ofpacts'. OK, that makes sense to me. I will resend soon.
diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c index f0a2ca59e2e8..8b1760c95409 100644 --- a/ofproto/ofproto-dpif.c +++ b/ofproto/ofproto-dpif.c @@ -5689,6 +5689,7 @@ const struct ofproto_class ofproto_dpif_class = { port_poll_wait, port_is_lacp_current, port_get_lacp_stats, + NULL, NULL, /* rule_choose_table */ rule_alloc, rule_construct, diff --git a/ofproto/ofproto-provider.h b/ofproto/ofproto-provider.h index 117cd1fcc02e..da6cb37c0b60 100644 --- a/ofproto/ofproto-provider.h +++ b/ofproto/ofproto-provider.h @@ -1077,6 +1077,20 @@ struct ofproto_class { /* ## OpenFlow Rule Functions ## */ /* ## ----------------------- ## */ + /* Checks whether the action 'ofpact' is supported by 'ofproto'. On + * success, returns 0. On failure, returns an OpenFlow error code. + * + * Some actions are marked as optional by the OpenFlow specification. + * Furthermore, OVS includes support for several vendor extensions which + * may not be supported by all ofproto implementations. This function + * allows specific actions to be rejected based on internal datapath + * support. Failure implies that an OpenFlow rule that includes 'ofpact' + * in its actions can never be inserted into 'ofproto'. + * + * If this function is NULL then all actions are supported. */ + enum ofperr (*rule_check_action)(const struct ofproto *ofproto, + const struct ofpact *ofpact); + /* Chooses an appropriate table for 'match' within 'ofproto'. On * success, stores the table ID into '*table_idp' and returns 0. On * failure, returns an OpenFlow error code. diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c index c7dd8a2c991b..338330108df1 100644 --- a/ofproto/ofproto.c +++ b/ofproto/ofproto.c @@ -3346,10 +3346,19 @@ ofproto_check_ofpacts(struct ofproto *ofproto, } OFPACT_FOR_EACH (a, ofpacts, ofpacts_len) { + enum ofperr error; + if (a->type == OFPACT_GROUP && !ofproto_group_exists(ofproto, ofpact_get_GROUP(a)->group_id)) { return OFPERR_OFPBAC_BAD_OUT_GROUP; } + + if (ofproto->ofproto_class->rule_check_action) { + error = ofproto->ofproto_class->rule_check_action(ofproto, a); + if (error) { + return error; + } + } } return 0;
Add an ofproto-level function to allow implementations to reject specific action types based on internal implementation details. The first user will be the next patch, which checks for datapath (kernel) support for various aspects of connection tracking and uses this to allow or reject ct() actions. Signed-off-by: Joe Stringer <joestringer@nicira.com> CC: Jarno Rajahalme <jarno@ovn.org> --- ofproto/ofproto-dpif.c | 1 + ofproto/ofproto-provider.h | 14 ++++++++++++++ ofproto/ofproto.c | 9 +++++++++ 3 files changed, 24 insertions(+)