| Message ID | 1447968798-81879-2-git-send-email-jarno@ovn.org |
|---|---|
| State | Changes Requested |
| Headers | show |
On Thu, Nov 19, 2015 at 01:33:18PM -0800, Jarno Rajahalme wrote: > Currently ovs-ofctl replace-flows and diff-flows commands only support > flows in table 0. Extend this to cover all possible tables. > > Signed-off-by: Jarno Rajahalme <jarno@ovn.org> There's one oddity that may deserve consideration. It depends on how careful we want to be. OpenFlow 1.0 does not define a way to add a flow to a particular table. The switch is responsible for deciding which table is most appropriate for a given flow. For example, a switch might have one table that supports wildcards and another one that is exact-match (this is in fact specifically envisioned by OF1.0 through its insistence that exact-match flows have the highest priority). This means that when talking to an OF1.0 switch, "ovs-ofctl replace-flows" (and friends) should ignore the table number. If a flow on the switch is in table 1, but the input file says it is in table 0 (probably because it doesn't specify a table at all), ovs-ofctl should do nothing, because that's the desired state. However, for practically forever, OVS has had special extensions to allow control over the table in which a flow lives. This means that if ovs-ofctl is talking to OVS, even in OpenFlow 1.0, it should place flows where the user requested and should not ignore the table numbers. This distinction is reflected through ofputil_protocol values. If a switch supports OFPUTIL_P_OF10_STD_TID or OFPUTIL_P_OF10_NXM_TID, then ovs-ofctl can place flows arbitrarily; if it only supports OFPUTIL_P_OF10_STD (or, theoretically, only OFPUTIL_P_OF10_NXM), then it is just a plain OF1.0 switch and all of the tables should be treated alike. OF1.1+ all support placing flows where the user requests. It's probably not too hard to support this, and possibly it is worthwhile. What do you think?
> On Nov 24, 2015, at 9:25 AM, Ben Pfaff <blp@ovn.org> wrote: > > On Thu, Nov 19, 2015 at 01:33:18PM -0800, Jarno Rajahalme wrote: >> Currently ovs-ofctl replace-flows and diff-flows commands only support >> flows in table 0. Extend this to cover all possible tables. >> >> Signed-off-by: Jarno Rajahalme <jarno@ovn.org> > > There's one oddity that may deserve consideration. It depends on how > careful we want to be. > > OpenFlow 1.0 does not define a way to add a flow to a particular table. > The switch is responsible for deciding which table is most appropriate > for a given flow. For example, a switch might have one table that > supports wildcards and another one that is exact-match (this is in fact > specifically envisioned by OF1.0 through its insistence that exact-match > flows have the highest priority). > > This means that when talking to an OF1.0 switch, "ovs-ofctl > replace-flows" (and friends) should ignore the table number. If > a flow on the switch is in table 1, but the input file says it is in > table 0 (probably because it doesn't specify a table at all), ovs-ofctl > should do nothing, because that's the desired state. > So for an OF1.0 switch without the Table ID extension we should ignore table numbers both ways, when reading from the file and when reading from the switch, essentially pretend that there is only one table? > However, for practically forever, OVS has had special extensions to > allow control over the table in which a flow lives. This means that if > ovs-ofctl is talking to OVS, even in OpenFlow 1.0, it should place flows > where the user requested and should not ignore the table numbers. > > This distinction is reflected through ofputil_protocol values. If a > switch supports OFPUTIL_P_OF10_STD_TID or OFPUTIL_P_OF10_NXM_TID, then > ovs-ofctl can place flows arbitrarily; if it only supports > OFPUTIL_P_OF10_STD (or, theoretically, only OFPUTIL_P_OF10_NXM), then it > is just a plain OF1.0 switch and all of the tables should be treated > alike. > > OF1.1+ all support placing flows where the user requests. > > It's probably not too hard to support this, and possibly it is > worthwhile. > > What do you think? I’ll take a stab at it, assuming the above, Jarno
> On Nov 24, 2015, at 9:40 AM, Jarno Rajahalme <jarno@ovn.org> wrote: > > >> On Nov 24, 2015, at 9:25 AM, Ben Pfaff <blp@ovn.org> wrote: >> >> On Thu, Nov 19, 2015 at 01:33:18PM -0800, Jarno Rajahalme wrote: >>> Currently ovs-ofctl replace-flows and diff-flows commands only support >>> flows in table 0. Extend this to cover all possible tables. >>> >>> Signed-off-by: Jarno Rajahalme <jarno@ovn.org> >> >> There's one oddity that may deserve consideration. It depends on how >> careful we want to be. >> >> OpenFlow 1.0 does not define a way to add a flow to a particular table. >> The switch is responsible for deciding which table is most appropriate >> for a given flow. For example, a switch might have one table that >> supports wildcards and another one that is exact-match (this is in fact >> specifically envisioned by OF1.0 through its insistence that exact-match >> flows have the highest priority). >> >> This means that when talking to an OF1.0 switch, "ovs-ofctl >> replace-flows" (and friends) should ignore the table number. If >> a flow on the switch is in table 1, but the input file says it is in >> table 0 (probably because it doesn't specify a table at all), ovs-ofctl >> should do nothing, because that's the desired state. >> > > So for an OF1.0 switch without the Table ID extension we should ignore table numbers both ways, when reading from the file and when reading from the switch, essentially pretend that there is only one table? > >> However, for practically forever, OVS has had special extensions to >> allow control over the table in which a flow lives. This means that if >> ovs-ofctl is talking to OVS, even in OpenFlow 1.0, it should place flows >> where the user requested and should not ignore the table numbers. >> >> This distinction is reflected through ofputil_protocol values. If a >> switch supports OFPUTIL_P_OF10_STD_TID or OFPUTIL_P_OF10_NXM_TID, then >> ovs-ofctl can place flows arbitrarily; if it only supports >> OFPUTIL_P_OF10_STD (or, theoretically, only OFPUTIL_P_OF10_NXM), then it >> is just a plain OF1.0 switch and all of the tables should be treated >> alike. >> >> OF1.1+ all support placing flows where the user requests. >> >> It's probably not too hard to support this, and possibly it is >> worthwhile. >> IMO this could be cleaner if the choice of protocol is driven by the input file. If the file has any flow with a non-zero or non-all table number, then we restrict the choice of protocols to ones that support multiple tables. Sounds reasonable? Jarno >> What do you think? > > I’ll take a stab at it, assuming the above, > > Jarno >
> On Nov 24, 2015, at 10:15 AM, Jarno Rajahalme <jarno@ovn.org> wrote: > > >> On Nov 24, 2015, at 9:40 AM, Jarno Rajahalme <jarno@ovn.org> wrote: >> >> >>> On Nov 24, 2015, at 9:25 AM, Ben Pfaff <blp@ovn.org> wrote: >>> >>> On Thu, Nov 19, 2015 at 01:33:18PM -0800, Jarno Rajahalme wrote: >>>> Currently ovs-ofctl replace-flows and diff-flows commands only support >>>> flows in table 0. Extend this to cover all possible tables. >>>> >>>> Signed-off-by: Jarno Rajahalme <jarno@ovn.org> >>> >>> There's one oddity that may deserve consideration. It depends on how >>> careful we want to be. >>> >>> OpenFlow 1.0 does not define a way to add a flow to a particular table. >>> The switch is responsible for deciding which table is most appropriate >>> for a given flow. For example, a switch might have one table that >>> supports wildcards and another one that is exact-match (this is in fact >>> specifically envisioned by OF1.0 through its insistence that exact-match >>> flows have the highest priority). >>> >>> This means that when talking to an OF1.0 switch, "ovs-ofctl >>> replace-flows" (and friends) should ignore the table number. If >>> a flow on the switch is in table 1, but the input file says it is in >>> table 0 (probably because it doesn't specify a table at all), ovs-ofctl >>> should do nothing, because that's the desired state. >>> >> >> So for an OF1.0 switch without the Table ID extension we should ignore table numbers both ways, when reading from the file and when reading from the switch, essentially pretend that there is only one table? >> >>> However, for practically forever, OVS has had special extensions to >>> allow control over the table in which a flow lives. This means that if >>> ovs-ofctl is talking to OVS, even in OpenFlow 1.0, it should place flows >>> where the user requested and should not ignore the table numbers. >>> >>> This distinction is reflected through ofputil_protocol values. If a >>> switch supports OFPUTIL_P_OF10_STD_TID or OFPUTIL_P_OF10_NXM_TID, then >>> ovs-ofctl can place flows arbitrarily; if it only supports >>> OFPUTIL_P_OF10_STD (or, theoretically, only OFPUTIL_P_OF10_NXM), then it >>> is just a plain OF1.0 switch and all of the tables should be treated >>> alike. >>> >>> OF1.1+ all support placing flows where the user requests. >>> >>> It's probably not too hard to support this, and possibly it is >>> worthwhile. >>> > > IMO this could be cleaner if the choice of protocol is driven by the input file. If the file has any flow with a non-zero or non-all table number, then we restrict the choice of protocols to ones that support multiple tables. Sounds reasonable? > parse_ofp_str() already does this: if (!strcmp(name, "table")) { error = str_to_u8(value, "table", &fm->table_id); if (fm->table_id != 0xff) { *usable_protocols &= OFPUTIL_P_TID; } } Here even “table=0” restricts vanilla OF1.0 out, which I think is the right thing to do. So it turns out OF1.0 without table extension is already taken care of by restricting the choice of protocol. Jarno
On Tue, Nov 24, 2015 at 09:40:20AM -0800, Jarno Rajahalme wrote: > > > On Nov 24, 2015, at 9:25 AM, Ben Pfaff <blp@ovn.org> wrote: > > > > On Thu, Nov 19, 2015 at 01:33:18PM -0800, Jarno Rajahalme wrote: > >> Currently ovs-ofctl replace-flows and diff-flows commands only support > >> flows in table 0. Extend this to cover all possible tables. > >> > >> Signed-off-by: Jarno Rajahalme <jarno@ovn.org> > > > > There's one oddity that may deserve consideration. It depends on how > > careful we want to be. > > > > OpenFlow 1.0 does not define a way to add a flow to a particular table. > > The switch is responsible for deciding which table is most appropriate > > for a given flow. For example, a switch might have one table that > > supports wildcards and another one that is exact-match (this is in fact > > specifically envisioned by OF1.0 through its insistence that exact-match > > flows have the highest priority). > > > > This means that when talking to an OF1.0 switch, "ovs-ofctl > > replace-flows" (and friends) should ignore the table number. If > > a flow on the switch is in table 1, but the input file says it is in > > table 0 (probably because it doesn't specify a table at all), ovs-ofctl > > should do nothing, because that's the desired state. > > > > So for an OF1.0 switch without the Table ID extension we should ignore > table numbers both ways, when reading from the file and when reading > from the switch, essentially pretend that there is only one table? Yes.
On Tue, Nov 24, 2015 at 10:15:57AM -0800, Jarno Rajahalme wrote: > > > On Nov 24, 2015, at 9:40 AM, Jarno Rajahalme <jarno@ovn.org> wrote: > > > > > >> On Nov 24, 2015, at 9:25 AM, Ben Pfaff <blp@ovn.org> wrote: > >> However, for practically forever, OVS has had special extensions to > >> allow control over the table in which a flow lives. This means that if > >> ovs-ofctl is talking to OVS, even in OpenFlow 1.0, it should place flows > >> where the user requested and should not ignore the table numbers. > >> > >> This distinction is reflected through ofputil_protocol values. If a > >> switch supports OFPUTIL_P_OF10_STD_TID or OFPUTIL_P_OF10_NXM_TID, then > >> ovs-ofctl can place flows arbitrarily; if it only supports > >> OFPUTIL_P_OF10_STD (or, theoretically, only OFPUTIL_P_OF10_NXM), then it > >> is just a plain OF1.0 switch and all of the tables should be treated > >> alike. > >> > >> OF1.1+ all support placing flows where the user requests. > >> > >> It's probably not too hard to support this, and possibly it is > >> worthwhile. > > IMO this could be cleaner if the choice of protocol is driven by the > input file. If the file has any flow with a non-zero or non-all table > number, then we restrict the choice of protocols to ones that support > multiple tables. Sounds reasonable? Maybe. I think that "dump-flows" will still print the table numbers for OF1.0 without the table-id extension, though, so this would cause odd behavior for what I've considered a reasonable use case of roughly: ovs-ofctl dump-flows br0 > dump ... ovs-ofctl replace-flows br0 dump
On Tue, Nov 24, 2015 at 10:21:41AM -0800, Jarno Rajahalme wrote: > > > On Nov 24, 2015, at 10:15 AM, Jarno Rajahalme <jarno@ovn.org> wrote: > > > > > >> On Nov 24, 2015, at 9:40 AM, Jarno Rajahalme <jarno@ovn.org> wrote: > >> > >> > >>> On Nov 24, 2015, at 9:25 AM, Ben Pfaff <blp@ovn.org> wrote: > >>> > >>> On Thu, Nov 19, 2015 at 01:33:18PM -0800, Jarno Rajahalme wrote: > >>>> Currently ovs-ofctl replace-flows and diff-flows commands only support > >>>> flows in table 0. Extend this to cover all possible tables. > >>>> > >>>> Signed-off-by: Jarno Rajahalme <jarno@ovn.org> > >>> > >>> There's one oddity that may deserve consideration. It depends on how > >>> careful we want to be. > >>> > >>> OpenFlow 1.0 does not define a way to add a flow to a particular table. > >>> The switch is responsible for deciding which table is most appropriate > >>> for a given flow. For example, a switch might have one table that > >>> supports wildcards and another one that is exact-match (this is in fact > >>> specifically envisioned by OF1.0 through its insistence that exact-match > >>> flows have the highest priority). > >>> > >>> This means that when talking to an OF1.0 switch, "ovs-ofctl > >>> replace-flows" (and friends) should ignore the table number. If > >>> a flow on the switch is in table 1, but the input file says it is in > >>> table 0 (probably because it doesn't specify a table at all), ovs-ofctl > >>> should do nothing, because that's the desired state. > >>> > >> > >> So for an OF1.0 switch without the Table ID extension we should ignore table numbers both ways, when reading from the file and when reading from the switch, essentially pretend that there is only one table? > >> > >>> However, for practically forever, OVS has had special extensions to > >>> allow control over the table in which a flow lives. This means that if > >>> ovs-ofctl is talking to OVS, even in OpenFlow 1.0, it should place flows > >>> where the user requested and should not ignore the table numbers. > >>> > >>> This distinction is reflected through ofputil_protocol values. If a > >>> switch supports OFPUTIL_P_OF10_STD_TID or OFPUTIL_P_OF10_NXM_TID, then > >>> ovs-ofctl can place flows arbitrarily; if it only supports > >>> OFPUTIL_P_OF10_STD (or, theoretically, only OFPUTIL_P_OF10_NXM), then it > >>> is just a plain OF1.0 switch and all of the tables should be treated > >>> alike. > >>> > >>> OF1.1+ all support placing flows where the user requests. > >>> > >>> It's probably not too hard to support this, and possibly it is > >>> worthwhile. > >>> > > > > IMO this could be cleaner if the choice of protocol is driven by the input file. If the file has any flow with a non-zero or non-all table number, then we restrict the choice of protocols to ones that support multiple tables. Sounds reasonable? > > > > parse_ofp_str() already does this: > > if (!strcmp(name, "table")) { > error = str_to_u8(value, "table", &fm->table_id); > if (fm->table_id != 0xff) { > *usable_protocols &= OFPUTIL_P_TID; > } > } > > Here even “table=0” restricts vanilla OF1.0 out, which I think is the right thing to do. > > So it turns out OF1.0 without table extension is already taken care of by restricting the choice of protocol. Hmm. Well, OK, we're no more wrong than we were before then.
> On Nov 24, 2015, at 4:22 PM, Ben Pfaff <blp@ovn.org> wrote: > > On Tue, Nov 24, 2015 at 10:21:41AM -0800, Jarno Rajahalme wrote: >> >>> On Nov 24, 2015, at 10:15 AM, Jarno Rajahalme <jarno@ovn.org> wrote: >>> >>> >>>> On Nov 24, 2015, at 9:40 AM, Jarno Rajahalme <jarno@ovn.org> wrote: >>>> >>>> >>>>> On Nov 24, 2015, at 9:25 AM, Ben Pfaff <blp@ovn.org> wrote: >>>>> >>>>> On Thu, Nov 19, 2015 at 01:33:18PM -0800, Jarno Rajahalme wrote: >>>>>> Currently ovs-ofctl replace-flows and diff-flows commands only support >>>>>> flows in table 0. Extend this to cover all possible tables. >>>>>> >>>>>> Signed-off-by: Jarno Rajahalme <jarno@ovn.org> >>>>> >>>>> There's one oddity that may deserve consideration. It depends on how >>>>> careful we want to be. >>>>> >>>>> OpenFlow 1.0 does not define a way to add a flow to a particular table. >>>>> The switch is responsible for deciding which table is most appropriate >>>>> for a given flow. For example, a switch might have one table that >>>>> supports wildcards and another one that is exact-match (this is in fact >>>>> specifically envisioned by OF1.0 through its insistence that exact-match >>>>> flows have the highest priority). >>>>> >>>>> This means that when talking to an OF1.0 switch, "ovs-ofctl >>>>> replace-flows" (and friends) should ignore the table number. If >>>>> a flow on the switch is in table 1, but the input file says it is in >>>>> table 0 (probably because it doesn't specify a table at all), ovs-ofctl >>>>> should do nothing, because that's the desired state. >>>>> >>>> >>>> So for an OF1.0 switch without the Table ID extension we should ignore table numbers both ways, when reading from the file and when reading from the switch, essentially pretend that there is only one table? >>>> >>>>> However, for practically forever, OVS has had special extensions to >>>>> allow control over the table in which a flow lives. This means that if >>>>> ovs-ofctl is talking to OVS, even in OpenFlow 1.0, it should place flows >>>>> where the user requested and should not ignore the table numbers. >>>>> >>>>> This distinction is reflected through ofputil_protocol values. If a >>>>> switch supports OFPUTIL_P_OF10_STD_TID or OFPUTIL_P_OF10_NXM_TID, then >>>>> ovs-ofctl can place flows arbitrarily; if it only supports >>>>> OFPUTIL_P_OF10_STD (or, theoretically, only OFPUTIL_P_OF10_NXM), then it >>>>> is just a plain OF1.0 switch and all of the tables should be treated >>>>> alike. >>>>> >>>>> OF1.1+ all support placing flows where the user requests. >>>>> >>>>> It's probably not too hard to support this, and possibly it is >>>>> worthwhile. >>>>> >>> >>> IMO this could be cleaner if the choice of protocol is driven by the input file. If the file has any flow with a non-zero or non-all table number, then we restrict the choice of protocols to ones that support multiple tables. Sounds reasonable? >>> >> >> parse_ofp_str() already does this: >> >> if (!strcmp(name, "table")) { >> error = str_to_u8(value, "table", &fm->table_id); >> if (fm->table_id != 0xff) { >> *usable_protocols &= OFPUTIL_P_TID; >> } >> } >> >> Here even “table=0” restricts vanilla OF1.0 out, which I think is the right thing to do. >> >> So it turns out OF1.0 without table extension is already taken care of by restricting the choice of protocol. > > Hmm. Well, OK, we're no more wrong than we were before then. Sounds like the dump-flows printing out table=0 for OF1.0 should be fixed in a separate patch? If so, you think this patch is ready to go? Jarno
On Wed, Nov 25, 2015 at 04:19:40PM -0800, Jarno Rajahalme wrote: > > > On Nov 24, 2015, at 4:22 PM, Ben Pfaff <blp@ovn.org> wrote: > > > > On Tue, Nov 24, 2015 at 10:21:41AM -0800, Jarno Rajahalme wrote: > >> > >>> On Nov 24, 2015, at 10:15 AM, Jarno Rajahalme <jarno@ovn.org> wrote: > >>> > >>> > >>>> On Nov 24, 2015, at 9:40 AM, Jarno Rajahalme <jarno@ovn.org> wrote: > >>>> > >>>> > >>>>> On Nov 24, 2015, at 9:25 AM, Ben Pfaff <blp@ovn.org> wrote: > >>>>> > >>>>> On Thu, Nov 19, 2015 at 01:33:18PM -0800, Jarno Rajahalme wrote: > >>>>>> Currently ovs-ofctl replace-flows and diff-flows commands only support > >>>>>> flows in table 0. Extend this to cover all possible tables. > >>>>>> > >>>>>> Signed-off-by: Jarno Rajahalme <jarno@ovn.org> > >>>>> > >>>>> There's one oddity that may deserve consideration. It depends on how > >>>>> careful we want to be. > >>>>> > >>>>> OpenFlow 1.0 does not define a way to add a flow to a particular table. > >>>>> The switch is responsible for deciding which table is most appropriate > >>>>> for a given flow. For example, a switch might have one table that > >>>>> supports wildcards and another one that is exact-match (this is in fact > >>>>> specifically envisioned by OF1.0 through its insistence that exact-match > >>>>> flows have the highest priority). > >>>>> > >>>>> This means that when talking to an OF1.0 switch, "ovs-ofctl > >>>>> replace-flows" (and friends) should ignore the table number. If > >>>>> a flow on the switch is in table 1, but the input file says it is in > >>>>> table 0 (probably because it doesn't specify a table at all), ovs-ofctl > >>>>> should do nothing, because that's the desired state. > >>>>> > >>>> > >>>> So for an OF1.0 switch without the Table ID extension we should ignore table numbers both ways, when reading from the file and when reading from the switch, essentially pretend that there is only one table? > >>>> > >>>>> However, for practically forever, OVS has had special extensions to > >>>>> allow control over the table in which a flow lives. This means that if > >>>>> ovs-ofctl is talking to OVS, even in OpenFlow 1.0, it should place flows > >>>>> where the user requested and should not ignore the table numbers. > >>>>> > >>>>> This distinction is reflected through ofputil_protocol values. If a > >>>>> switch supports OFPUTIL_P_OF10_STD_TID or OFPUTIL_P_OF10_NXM_TID, then > >>>>> ovs-ofctl can place flows arbitrarily; if it only supports > >>>>> OFPUTIL_P_OF10_STD (or, theoretically, only OFPUTIL_P_OF10_NXM), then it > >>>>> is just a plain OF1.0 switch and all of the tables should be treated > >>>>> alike. > >>>>> > >>>>> OF1.1+ all support placing flows where the user requests. > >>>>> > >>>>> It's probably not too hard to support this, and possibly it is > >>>>> worthwhile. > >>>>> > >>> > >>> IMO this could be cleaner if the choice of protocol is driven by the input file. If the file has any flow with a non-zero or non-all table number, then we restrict the choice of protocols to ones that support multiple tables. Sounds reasonable? > >>> > >> > >> parse_ofp_str() already does this: > >> > >> if (!strcmp(name, "table")) { > >> error = str_to_u8(value, "table", &fm->table_id); > >> if (fm->table_id != 0xff) { > >> *usable_protocols &= OFPUTIL_P_TID; > >> } > >> } > >> > >> Here even “table=0” restricts vanilla OF1.0 out, which I think is the right thing to do. > >> > >> So it turns out OF1.0 without table extension is already taken care of by restricting the choice of protocol. > > > > Hmm. Well, OK, we're no more wrong than we were before then. > > Sounds like the dump-flows printing out table=0 for OF1.0 should be > fixed in a separate patch? If so, you think this patch is ready to go? I'm not sure that's the correct resolution but I do think it's reasonable to resolve it separately. Acked-by: Ben Pfaff <blp@ovn.org>
> On Nov 25, 2015, at 5:07 PM, Ben Pfaff <blp@ovn.org> wrote: > > On Wed, Nov 25, 2015 at 04:19:40PM -0800, Jarno Rajahalme wrote: >> >>> On Nov 24, 2015, at 4:22 PM, Ben Pfaff <blp@ovn.org> wrote: >>> >>> On Tue, Nov 24, 2015 at 10:21:41AM -0800, Jarno Rajahalme wrote: >>>> >>>>> On Nov 24, 2015, at 10:15 AM, Jarno Rajahalme <jarno@ovn.org> wrote: >>>>> >>>>> >>>>>> On Nov 24, 2015, at 9:40 AM, Jarno Rajahalme <jarno@ovn.org> wrote: >>>>>> >>>>>> >>>>>>> On Nov 24, 2015, at 9:25 AM, Ben Pfaff <blp@ovn.org> wrote: >>>>>>> >>>>>>> On Thu, Nov 19, 2015 at 01:33:18PM -0800, Jarno Rajahalme wrote: >>>>>>>> Currently ovs-ofctl replace-flows and diff-flows commands only support >>>>>>>> flows in table 0. Extend this to cover all possible tables. >>>>>>>> >>>>>>>> Signed-off-by: Jarno Rajahalme <jarno@ovn.org> >>>>>>> >>>>>>> There's one oddity that may deserve consideration. It depends on how >>>>>>> careful we want to be. >>>>>>> >>>>>>> OpenFlow 1.0 does not define a way to add a flow to a particular table. >>>>>>> The switch is responsible for deciding which table is most appropriate >>>>>>> for a given flow. For example, a switch might have one table that >>>>>>> supports wildcards and another one that is exact-match (this is in fact >>>>>>> specifically envisioned by OF1.0 through its insistence that exact-match >>>>>>> flows have the highest priority). >>>>>>> >>>>>>> This means that when talking to an OF1.0 switch, "ovs-ofctl >>>>>>> replace-flows" (and friends) should ignore the table number. If >>>>>>> a flow on the switch is in table 1, but the input file says it is in >>>>>>> table 0 (probably because it doesn't specify a table at all), ovs-ofctl >>>>>>> should do nothing, because that's the desired state. >>>>>>> >>>>>> >>>>>> So for an OF1.0 switch without the Table ID extension we should ignore table numbers both ways, when reading from the file and when reading from the switch, essentially pretend that there is only one table? >>>>>> >>>>>>> However, for practically forever, OVS has had special extensions to >>>>>>> allow control over the table in which a flow lives. This means that if >>>>>>> ovs-ofctl is talking to OVS, even in OpenFlow 1.0, it should place flows >>>>>>> where the user requested and should not ignore the table numbers. >>>>>>> >>>>>>> This distinction is reflected through ofputil_protocol values. If a >>>>>>> switch supports OFPUTIL_P_OF10_STD_TID or OFPUTIL_P_OF10_NXM_TID, then >>>>>>> ovs-ofctl can place flows arbitrarily; if it only supports >>>>>>> OFPUTIL_P_OF10_STD (or, theoretically, only OFPUTIL_P_OF10_NXM), then it >>>>>>> is just a plain OF1.0 switch and all of the tables should be treated >>>>>>> alike. >>>>>>> >>>>>>> OF1.1+ all support placing flows where the user requests. >>>>>>> >>>>>>> It's probably not too hard to support this, and possibly it is >>>>>>> worthwhile. >>>>>>> >>>>> >>>>> IMO this could be cleaner if the choice of protocol is driven by the input file. If the file has any flow with a non-zero or non-all table number, then we restrict the choice of protocols to ones that support multiple tables. Sounds reasonable? >>>>> >>>> >>>> parse_ofp_str() already does this: >>>> >>>> if (!strcmp(name, "table")) { >>>> error = str_to_u8(value, "table", &fm->table_id); >>>> if (fm->table_id != 0xff) { >>>> *usable_protocols &= OFPUTIL_P_TID; >>>> } >>>> } >>>> >>>> Here even “table=0” restricts vanilla OF1.0 out, which I think is the right thing to do. >>>> >>>> So it turns out OF1.0 without table extension is already taken care of by restricting the choice of protocol. >>> >>> Hmm. Well, OK, we're no more wrong than we were before then. >> >> Sounds like the dump-flows printing out table=0 for OF1.0 should be >> fixed in a separate patch? If so, you think this patch is ready to go? > > I'm not sure that's the correct resolution but I do think it's > reasonable to resolve it separately. > > Acked-by: Ben Pfaff <blp@ovn.org <mailto:blp@ovn.org>> Pushed to master, Jarno
diff --git a/tests/ovs-ofctl.at b/tests/ovs-ofctl.at index 7375cad..e52cbf9 100644 --- a/tests/ovs-ofctl.at +++ b/tests/ovs-ofctl.at @@ -2869,11 +2869,11 @@ OVS_VSWITCHD_START AT_CHECK([ovs-appctl vlog/set vconn:dbg]) dnl Add flows to br0 with importance via OF1.4+, using an OF1.4+ bundle. For more details refer "ovs-ofctl rule with importance" test case. -for i in 1 2 3 4 5 6 7 8; do echo "dl_vlan=$i,importance=$i,actions=drop"; done > add-flows.txt +for i in 1 2 3 4 5 6 7 8; do echo "table=$i,dl_vlan=$i,importance=$i,actions=drop"; done > add-flows.txt AT_CHECK([ovs-ofctl --bundle add-flows br0 add-flows.txt]) dnl Replace some flows in the bridge. -for i in 1 3 5 7; do echo " importance=`expr $i + 10`, dl_vlan=$i actions=drop"; done > replace-flows.txt +for i in 1 3 5 7; do echo " table=$i, importance=`expr $i + 10`, dl_vlan=$i actions=drop"; done > replace-flows.txt AT_CHECK([ovs-ofctl --bundle replace-flows br0 replace-flows.txt]) dnl Dump them and compare the dump flows output against the expected output. @@ -2897,28 +2897,28 @@ vconn|DBG|unix: sent (Success): OFPT_BUNDLE_CONTROL (OF1.4): bundle_id=0 type=OPEN_REPLY flags=0 vconn|DBG|unix: received: OFPT_BUNDLE_ADD_MESSAGE (OF1.4): bundle_id=0 flags=atomic ordered -OFPT_FLOW_MOD (OF1.4): ADD dl_vlan=1 importance:1 actions=drop +OFPT_FLOW_MOD (OF1.4): ADD table:1 dl_vlan=1 importance:1 actions=drop vconn|DBG|unix: received: OFPT_BUNDLE_ADD_MESSAGE (OF1.4): bundle_id=0 flags=atomic ordered -OFPT_FLOW_MOD (OF1.4): ADD dl_vlan=2 importance:2 actions=drop +OFPT_FLOW_MOD (OF1.4): ADD table:2 dl_vlan=2 importance:2 actions=drop vconn|DBG|unix: received: OFPT_BUNDLE_ADD_MESSAGE (OF1.4): bundle_id=0 flags=atomic ordered -OFPT_FLOW_MOD (OF1.4): ADD dl_vlan=3 importance:3 actions=drop +OFPT_FLOW_MOD (OF1.4): ADD table:3 dl_vlan=3 importance:3 actions=drop vconn|DBG|unix: received: OFPT_BUNDLE_ADD_MESSAGE (OF1.4): bundle_id=0 flags=atomic ordered -OFPT_FLOW_MOD (OF1.4): ADD dl_vlan=4 importance:4 actions=drop +OFPT_FLOW_MOD (OF1.4): ADD table:4 dl_vlan=4 importance:4 actions=drop vconn|DBG|unix: received: OFPT_BUNDLE_ADD_MESSAGE (OF1.4): bundle_id=0 flags=atomic ordered -OFPT_FLOW_MOD (OF1.4): ADD dl_vlan=5 importance:5 actions=drop +OFPT_FLOW_MOD (OF1.4): ADD table:5 dl_vlan=5 importance:5 actions=drop vconn|DBG|unix: received: OFPT_BUNDLE_ADD_MESSAGE (OF1.4): bundle_id=0 flags=atomic ordered -OFPT_FLOW_MOD (OF1.4): ADD dl_vlan=6 importance:6 actions=drop +OFPT_FLOW_MOD (OF1.4): ADD table:6 dl_vlan=6 importance:6 actions=drop vconn|DBG|unix: received: OFPT_BUNDLE_ADD_MESSAGE (OF1.4): bundle_id=0 flags=atomic ordered -OFPT_FLOW_MOD (OF1.4): ADD dl_vlan=7 importance:7 actions=drop +OFPT_FLOW_MOD (OF1.4): ADD table:7 dl_vlan=7 importance:7 actions=drop vconn|DBG|unix: received: OFPT_BUNDLE_ADD_MESSAGE (OF1.4): bundle_id=0 flags=atomic ordered -OFPT_FLOW_MOD (OF1.4): ADD dl_vlan=8 importance:8 actions=drop +OFPT_FLOW_MOD (OF1.4): ADD table:8 dl_vlan=8 importance:8 actions=drop vconn|DBG|unix: received: OFPT_BUNDLE_CONTROL (OF1.4): bundle_id=0 type=COMMIT_REQUEST flags=atomic ordered vconn|DBG|unix: sent (Success): OFPT_BUNDLE_CONTROL (OF1.4): @@ -2930,42 +2930,42 @@ vconn|DBG|unix: received: OFPT_HELLO (OF1.4): vconn|DBG|unix: negotiated OpenFlow version 0x05 (we support version 0x06 and earlier, peer supports version 0x05) vconn|DBG|unix: received: OFPST_FLOW request (OF1.4): vconn|DBG|unix: sent (Success): OFPST_FLOW reply (OF1.4): - importance=1, dl_vlan=1 actions=drop - importance=2, dl_vlan=2 actions=drop - importance=3, dl_vlan=3 actions=drop - importance=4, dl_vlan=4 actions=drop - importance=5, dl_vlan=5 actions=drop - importance=6, dl_vlan=6 actions=drop - importance=7, dl_vlan=7 actions=drop - importance=8, dl_vlan=8 actions=drop + table=1, importance=1, dl_vlan=1 actions=drop + table=2, importance=2, dl_vlan=2 actions=drop + table=3, importance=3, dl_vlan=3 actions=drop + table=4, importance=4, dl_vlan=4 actions=drop + table=5, importance=5, dl_vlan=5 actions=drop + table=6, importance=6, dl_vlan=6 actions=drop + table=7, importance=7, dl_vlan=7 actions=drop + table=8, importance=8, dl_vlan=8 actions=drop vconn|DBG|unix: received: OFPT_BUNDLE_CONTROL (OF1.4): bundle_id=0 type=OPEN_REQUEST flags=atomic ordered vconn|DBG|unix: sent (Success): OFPT_BUNDLE_CONTROL (OF1.4): bundle_id=0 type=OPEN_REPLY flags=0 vconn|DBG|unix: received: OFPT_BUNDLE_ADD_MESSAGE (OF1.4): bundle_id=0 flags=atomic ordered -OFPT_FLOW_MOD (OF1.4): DEL_STRICT table:255 dl_vlan=2 actions=drop +OFPT_FLOW_MOD (OF1.4): ADD table:1 dl_vlan=1 importance:11 actions=drop vconn|DBG|unix: received: OFPT_BUNDLE_ADD_MESSAGE (OF1.4): bundle_id=0 flags=atomic ordered -OFPT_FLOW_MOD (OF1.4): DEL_STRICT table:255 dl_vlan=4 actions=drop +OFPT_FLOW_MOD (OF1.4): DEL_STRICT table:2 dl_vlan=2 actions=drop vconn|DBG|unix: received: OFPT_BUNDLE_ADD_MESSAGE (OF1.4): bundle_id=0 flags=atomic ordered -OFPT_FLOW_MOD (OF1.4): DEL_STRICT table:255 dl_vlan=6 actions=drop +OFPT_FLOW_MOD (OF1.4): ADD table:3 dl_vlan=3 importance:13 actions=drop vconn|DBG|unix: received: OFPT_BUNDLE_ADD_MESSAGE (OF1.4): bundle_id=0 flags=atomic ordered -OFPT_FLOW_MOD (OF1.4): DEL_STRICT table:255 dl_vlan=8 actions=drop +OFPT_FLOW_MOD (OF1.4): DEL_STRICT table:4 dl_vlan=4 actions=drop vconn|DBG|unix: received: OFPT_BUNDLE_ADD_MESSAGE (OF1.4): bundle_id=0 flags=atomic ordered -OFPT_FLOW_MOD (OF1.4): ADD dl_vlan=1 importance:11 actions=drop +OFPT_FLOW_MOD (OF1.4): ADD table:5 dl_vlan=5 importance:15 actions=drop vconn|DBG|unix: received: OFPT_BUNDLE_ADD_MESSAGE (OF1.4): bundle_id=0 flags=atomic ordered -OFPT_FLOW_MOD (OF1.4): ADD dl_vlan=3 importance:13 actions=drop +OFPT_FLOW_MOD (OF1.4): DEL_STRICT table:6 dl_vlan=6 actions=drop vconn|DBG|unix: received: OFPT_BUNDLE_ADD_MESSAGE (OF1.4): bundle_id=0 flags=atomic ordered -OFPT_FLOW_MOD (OF1.4): ADD dl_vlan=5 importance:15 actions=drop +OFPT_FLOW_MOD (OF1.4): ADD table:7 dl_vlan=7 importance:17 actions=drop vconn|DBG|unix: received: OFPT_BUNDLE_ADD_MESSAGE (OF1.4): bundle_id=0 flags=atomic ordered -OFPT_FLOW_MOD (OF1.4): ADD dl_vlan=7 importance:17 actions=drop +OFPT_FLOW_MOD (OF1.4): DEL_STRICT table:8 dl_vlan=8 actions=drop vconn|DBG|unix: received: OFPT_BUNDLE_CONTROL (OF1.4): bundle_id=0 type=COMMIT_REQUEST flags=atomic ordered vconn|DBG|unix: sent (Success): OFPT_BUNDLE_CONTROL (OF1.4): @@ -2977,10 +2977,10 @@ vconn|DBG|unix: received: OFPT_HELLO (OF1.4): vconn|DBG|unix: negotiated OpenFlow version 0x05 (we support version 0x06 and earlier, peer supports version 0x05) vconn|DBG|unix: received: OFPST_FLOW request (OF1.4): vconn|DBG|unix: sent (Success): OFPST_FLOW reply (OF1.4): - importance=11, dl_vlan=1 actions=drop - importance=13, dl_vlan=3 actions=drop - importance=15, dl_vlan=5 actions=drop - importance=17, dl_vlan=7 actions=drop + table=1, importance=11, dl_vlan=1 actions=drop + table=3, importance=13, dl_vlan=3 actions=drop + table=5, importance=15, dl_vlan=5 actions=drop + table=7, importance=17, dl_vlan=7 actions=drop ]) OVS_VSWITCHD_STOP diff --git a/utilities/ovs-ofctl.c b/utilities/ovs-ofctl.c index ece62d5..23a9557 100644 --- a/utilities/ovs-ofctl.c +++ b/utilities/ovs-ofctl.c @@ -2472,6 +2472,45 @@ ofctl_list_commands(struct ovs_cmdl_context *ctx OVS_UNUSED) /* replace-flows and diff-flows commands. */ +struct flow_tables { + struct classifier tables[OFPTT_MAX + 1]; +}; + +#define FOR_EACH_TABLE(CLS, TABLES) \ + for ((CLS) = (TABLES)->tables; \ + (CLS) < &(TABLES)->tables[ARRAY_SIZE((TABLES)->tables)]; \ + (CLS)++) + +static void +flow_tables_init(struct flow_tables *tables) +{ + struct classifier *cls; + + FOR_EACH_TABLE (cls, tables) { + classifier_init(cls, NULL); + } +} + +static void +flow_tables_defer(struct flow_tables *tables) +{ + struct classifier *cls; + + FOR_EACH_TABLE (cls, tables) { + classifier_defer(cls); + } +} + +static void +flow_tables_publish(struct flow_tables *tables) +{ + struct classifier *cls; + + FOR_EACH_TABLE (cls, tables) { + classifier_publish(cls); + } +} + /* A flow table entry, possibly with two different versions. */ struct fte { struct cls_rule rule; /* Within a "struct classifier". */ @@ -2487,6 +2526,7 @@ struct fte_version { uint16_t flags; struct ofpact *ofpacts; size_t ofpacts_len; + uint8_t table_id; }; /* Frees 'version' and the data that it owns. */ @@ -2510,6 +2550,7 @@ fte_version_equals(const struct fte_version *a, const struct fte_version *b) && a->idle_timeout == b->idle_timeout && a->hard_timeout == b->hard_timeout && a->importance == b->importance + && a->table_id == b->table_id && ofpacts_equal(a->ofpacts, a->ofpacts_len, b->ofpacts, b->ofpacts_len)); } @@ -2526,6 +2567,9 @@ fte_version_format(const struct fte *fte, int index, struct ds *s) return; } + if (version->table_id) { + ds_put_format(s, "table=%"PRIu8" ", version->table_id); + } cls_rule_format(&fte->rule, s); if (version->cookie != htonll(0)) { ds_put_format(s, " cookie=0x%"PRIx64, ntohll(version->cookie)); @@ -2564,29 +2608,34 @@ fte_free(struct fte *fte) } } -/* Frees all of the FTEs within 'cls'. */ +/* Frees all of the FTEs within 'tables'. */ static void -fte_free_all(struct classifier *cls) +fte_free_all(struct flow_tables *tables) { - struct fte *fte; + struct classifier *cls; + + FOR_EACH_TABLE (cls, tables) { + struct fte *fte; - classifier_defer(cls); - CLS_FOR_EACH (fte, rule, cls) { - classifier_remove(cls, &fte->rule); - ovsrcu_postpone(fte_free, fte); + classifier_defer(cls); + CLS_FOR_EACH (fte, rule, cls) { + classifier_remove(cls, &fte->rule); + ovsrcu_postpone(fte_free, fte); + } + classifier_destroy(cls); } - classifier_destroy(cls); } -/* Searches 'cls' for an FTE matching 'rule', inserting a new one if +/* Searches 'tables' for an FTE matching 'rule', inserting a new one if * necessary. Sets 'version' as the version of that rule with the given * 'index', replacing any existing version, if any. * * Takes ownership of 'version'. */ static void -fte_insert(struct classifier *cls, const struct match *match, +fte_insert(struct flow_tables *tables, const struct match *match, int priority, struct fte_version *version, int index) { + struct classifier *cls = &tables->tables[version->table_id]; struct fte *old, *fte; fte = xzalloc(sizeof *fte); @@ -2603,11 +2652,12 @@ fte_insert(struct classifier *cls, const struct match *match, } } -/* Reads the flows in 'filename' as flow table entries in 'cls' for the version - * with the specified 'index'. Returns the flow formats able to represent the - * flows that were read. */ +/* Reads the flows in 'filename' as flow table entries in 'tables' for the + * version with the specified 'index'. Returns the flow formats able to + * represent the flows that were read. */ static enum ofputil_protocol -read_flows_from_file(const char *filename, struct classifier *cls, int index) +read_flows_from_file(const char *filename, struct flow_tables *tables, + int index) { enum ofputil_protocol usable_protocols; int line_number; @@ -2622,7 +2672,7 @@ read_flows_from_file(const char *filename, struct classifier *cls, int index) ds_init(&s); usable_protocols = OFPUTIL_P_ANY; line_number = 0; - classifier_defer(cls); + flow_tables_defer(tables); while (!ds_get_preprocessed_line(&s, file, &line_number)) { struct fte_version *version; struct ofputil_flow_mod fm; @@ -2644,10 +2694,11 @@ read_flows_from_file(const char *filename, struct classifier *cls, int index) | OFPUTIL_FF_EMERG); version->ofpacts = fm.ofpacts; version->ofpacts_len = fm.ofpacts_len; + version->table_id = fm.table_id != OFPTT_ALL ? fm.table_id : 0; - fte_insert(cls, &fm.match, fm.priority, version, index); + fte_insert(tables, &fm.match, fm.priority, version, index); } - classifier_publish(cls); + flow_tables_publish(tables); ds_destroy(&s); if (file != stdin) { @@ -2711,12 +2762,12 @@ recv_flow_stats_reply(struct vconn *vconn, ovs_be32 send_xid, } /* Reads the OpenFlow flow table from 'vconn', which has currently active flow - * format 'protocol', and adds them as flow table entries in 'cls' for the + * format 'protocol', and adds them as flow table entries in 'tables' for the * version with the specified 'index'. */ static void read_flows_from_switch(struct vconn *vconn, enum ofputil_protocol protocol, - struct classifier *cls, int index) + struct flow_tables *tables, int index) { struct ofputil_flow_stats_request fsr; struct ofputil_flow_stats fs; @@ -2737,7 +2788,7 @@ read_flows_from_switch(struct vconn *vconn, reply = NULL; ofpbuf_init(&ofpacts, 0); - classifier_defer(cls); + flow_tables_defer(tables); while (recv_flow_stats_reply(vconn, send_xid, &reply, &fs, &ofpacts)) { struct fte_version *version; @@ -2749,10 +2800,11 @@ read_flows_from_switch(struct vconn *vconn, version->flags = 0; version->ofpacts_len = fs.ofpacts_len; version->ofpacts = xmemdup(fs.ofpacts, fs.ofpacts_len); + version->table_id = fs.table_id; - fte_insert(cls, &fs.match, fs.priority, version, index); + fte_insert(tables, &fs.match, fs.priority, version, index); } - classifier_publish(cls); + flow_tables_publish(tables); ofpbuf_uninit(&ofpacts); } @@ -2770,7 +2822,7 @@ fte_make_flow_mod(const struct fte *fte, int index, uint16_t command, fm.cookie_mask = htonll(0); fm.new_cookie = version->cookie; fm.modify_cookie = true; - fm.table_id = 0xff; + fm.table_id = version->table_id; fm.command = command; fm.idle_timeout = version->idle_timeout; fm.hard_timeout = version->hard_timeout; @@ -2798,41 +2850,45 @@ ofctl_replace_flows(struct ovs_cmdl_context *ctx) { enum { FILE_IDX = 0, SWITCH_IDX = 1 }; enum ofputil_protocol usable_protocols, protocol; - struct classifier cls; + struct flow_tables tables; + struct classifier *cls; struct ovs_list requests; struct vconn *vconn; struct fte *fte; - classifier_init(&cls, NULL); - usable_protocols = read_flows_from_file(ctx->argv[2], &cls, FILE_IDX); + flow_tables_init(&tables); + usable_protocols = read_flows_from_file(ctx->argv[2], &tables, FILE_IDX); protocol = open_vconn(ctx->argv[1], &vconn); protocol = set_protocol_for_flow_dump(vconn, protocol, usable_protocols); - read_flows_from_switch(vconn, protocol, &cls, SWITCH_IDX); + read_flows_from_switch(vconn, protocol, &tables, SWITCH_IDX); list_init(&requests); - /* Delete flows that exist on the switch but not in the file. */ - CLS_FOR_EACH (fte, rule, &cls) { - struct fte_version *file_ver = fte->versions[FILE_IDX]; - struct fte_version *sw_ver = fte->versions[SWITCH_IDX]; + FOR_EACH_TABLE (cls, &tables) { + /* Delete flows that exist on the switch but not in the file. */ + CLS_FOR_EACH (fte, rule, cls) { + struct fte_version *file_ver = fte->versions[FILE_IDX]; + struct fte_version *sw_ver = fte->versions[SWITCH_IDX]; - if (sw_ver && !file_ver) { - fte_make_flow_mod(fte, SWITCH_IDX, OFPFC_DELETE_STRICT, - protocol, &requests); + if (sw_ver && !file_ver) { + fte_make_flow_mod(fte, SWITCH_IDX, OFPFC_DELETE_STRICT, + protocol, &requests); + } } - } - /* Add flows that exist in the file but not on the switch. - * Update flows that exist in both places but differ. */ - CLS_FOR_EACH (fte, rule, &cls) { - struct fte_version *file_ver = fte->versions[FILE_IDX]; - struct fte_version *sw_ver = fte->versions[SWITCH_IDX]; + /* Add flows that exist in the file but not on the switch. + * Update flows that exist in both places but differ. */ + CLS_FOR_EACH (fte, rule, cls) { + struct fte_version *file_ver = fte->versions[FILE_IDX]; + struct fte_version *sw_ver = fte->versions[SWITCH_IDX]; - if (file_ver - && (readd || !sw_ver || !fte_version_equals(sw_ver, file_ver))) { - fte_make_flow_mod(fte, FILE_IDX, OFPFC_ADD, protocol, &requests); + if (file_ver && + (readd || !sw_ver || !fte_version_equals(sw_ver, file_ver))) { + fte_make_flow_mod(fte, FILE_IDX, OFPFC_ADD, protocol, + &requests); + } } } if (bundle) { @@ -2842,24 +2898,25 @@ ofctl_replace_flows(struct ovs_cmdl_context *ctx) } vconn_close(vconn); - fte_free_all(&cls); + fte_free_all(&tables); } static void -read_flows_from_source(const char *source, struct classifier *cls, int index) +read_flows_from_source(const char *source, struct flow_tables *tables, + int index) { struct stat s; if (source[0] == '/' || source[0] == '.' || (!strchr(source, ':') && !stat(source, &s))) { - read_flows_from_file(source, cls, index); + read_flows_from_file(source, tables, index); } else { enum ofputil_protocol protocol; struct vconn *vconn; protocol = open_vconn(source, &vconn); protocol = set_protocol_for_flow_dump(vconn, protocol, OFPUTIL_P_ANY); - read_flows_from_switch(vconn, protocol, cls, index); + read_flows_from_switch(vconn, protocol, tables, index); vconn_close(vconn); } } @@ -2868,32 +2925,35 @@ static void ofctl_diff_flows(struct ovs_cmdl_context *ctx) { bool differences = false; - struct classifier cls; + struct flow_tables tables; + struct classifier *cls; struct ds a_s, b_s; struct fte *fte; - classifier_init(&cls, NULL); - read_flows_from_source(ctx->argv[1], &cls, 0); - read_flows_from_source(ctx->argv[2], &cls, 1); + flow_tables_init(&tables); + read_flows_from_source(ctx->argv[1], &tables, 0); + read_flows_from_source(ctx->argv[2], &tables, 1); ds_init(&a_s); ds_init(&b_s); - CLS_FOR_EACH (fte, rule, &cls) { - struct fte_version *a = fte->versions[0]; - struct fte_version *b = fte->versions[1]; - - if (!a || !b || !fte_version_equals(a, b)) { - fte_version_format(fte, 0, &a_s); - fte_version_format(fte, 1, &b_s); - if (strcmp(ds_cstr(&a_s), ds_cstr(&b_s))) { - if (a_s.length) { - printf("-%s", ds_cstr(&a_s)); - } - if (b_s.length) { - printf("+%s", ds_cstr(&b_s)); + FOR_EACH_TABLE (cls, &tables) { + CLS_FOR_EACH (fte, rule, cls) { + struct fte_version *a = fte->versions[0]; + struct fte_version *b = fte->versions[1]; + + if (!a || !b || !fte_version_equals(a, b)) { + fte_version_format(fte, 0, &a_s); + fte_version_format(fte, 1, &b_s); + if (strcmp(ds_cstr(&a_s), ds_cstr(&b_s))) { + if (a_s.length) { + printf("-%s", ds_cstr(&a_s)); + } + if (b_s.length) { + printf("+%s", ds_cstr(&b_s)); + } + differences = true; } - differences = true; } } } @@ -2901,7 +2961,7 @@ ofctl_diff_flows(struct ovs_cmdl_context *ctx) ds_destroy(&a_s); ds_destroy(&b_s); - fte_free_all(&cls); + fte_free_all(&tables); if (differences) { exit(2); @@ -3248,7 +3308,7 @@ ofctl_parse_actions__(const char *version_s, bool instructions) error = ofpacts_check_consistency(ofpacts.data, ofpacts.size, &flow, OFPP_MAX, table_id ? atoi(table_id) : 0, - 255, protocol); + OFPTT_MAX + 1, protocol); } if (error) { printf("bad %s %s: %s\n\n",
Currently ovs-ofctl replace-flows and diff-flows commands only support flows in table 0. Extend this to cover all possible tables. Signed-off-by: Jarno Rajahalme <jarno@ovn.org> --- tests/ovs-ofctl.at | 60 ++++++++-------- utilities/ovs-ofctl.c | 194 +++++++++++++++++++++++++++++++++----------------- 2 files changed, 157 insertions(+), 97 deletions(-)