diff mbox

[ovs-dev,RFC,v2] lib/table: add new flexible formatting

Message ID 1481126154-6515-1-git-send-email-aconole@redhat.com
State Changes Requested
Headers show

Commit Message

Aaron Conole Dec. 7, 2016, 3:55 p.m. UTC
Adds a new --format="xx={0},..." to the table library, which allows
users to make table output formats to match their own needs.

Signed-off-by: Aaron Conole <aconole@redhat.com>
---
v1 url:
    https://mail.openvswitch.org/pipermail/ovs-dev/2016-November/325235.html

TODO: Still needs unit tests to check the format strings
      One thing I want to do is put in the idea of string length to the
      grammar.  Probably will be a few other additions (ex: text which is
      omitted when the corresponding column value is empty).

NOTE: Submitted for early feedback

 lib/table.c   | 192 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 lib/table.h   |   6 +-
 lib/table.man |  17 ++++++
 3 files changed, 213 insertions(+), 2 deletions(-)

Comments

Ben Pfaff Dec. 7, 2016, 5:06 p.m. UTC | #1
On Wed, Dec 07, 2016 at 10:55:54AM -0500, Aaron Conole wrote:
> Adds a new --format="xx={0},..." to the table library, which allows
> users to make table output formats to match their own needs.
> 
> Signed-off-by: Aaron Conole <aconole@redhat.com>
> ---
> v1 url:
>     https://mail.openvswitch.org/pipermail/ovs-dev/2016-November/325235.html
> 
> TODO: Still needs unit tests to check the format strings
>       One thing I want to do is put in the idea of string length to the
>       grammar.  Probably will be a few other additions (ex: text which is
>       omitted when the corresponding column value is empty).
> 
> NOTE: Submitted for early feedback

Can you give an example of a use case?  If we're going to add this
feature for formatting, I'd like to know that it's going to be genuinely
useful to users.
Aaron Conole Dec. 7, 2016, 6:08 p.m. UTC | #2
Ben Pfaff <blp@ovn.org> writes:

> On Wed, Dec 07, 2016 at 10:55:54AM -0500, Aaron Conole wrote:
>> Adds a new --format="xx={0},..." to the table library, which allows
>> users to make table output formats to match their own needs.
>> 
>> Signed-off-by: Aaron Conole <aconole@redhat.com>
>> ---
>> v1 url:
>>     https://mail.openvswitch.org/pipermail/ovs-dev/2016-November/325235.html
>> 
>> TODO: Still needs unit tests to check the format strings
>>       One thing I want to do is put in the idea of string length to the
>>       grammar.  Probably will be a few other additions (ex: text which is
>>       omitted when the corresponding column value is empty).
>> 
>> NOTE: Submitted for early feedback
>
> Can you give an example of a use case?  If we're going to add this
> feature for formatting, I'd like to know that it's going to be genuinely
> useful to users.

Yes, sorry - I should have re-included my previous cover letter;

I am currently asked by some openstack folks to aid with 'debugability'
- specifically, they want a change to the openflow printing such that
they have data like:

COL1    COL2    COL3
====    ====    ====

They want to choose the columns 'as-needed', and I think they also want
to change the formats.  Rather than code up multiple custom styles, I'd
like to just give them the ability to change output formats as they
wish.

To accomplish this, I'm going down the following route:

1. Add formatting option to the lib/table.{c,h}, such that we can
   still express the existing output.

2. Change ofp-print to use a table for flow dumping, with a default
   format that preserves the current format, and the option of changing
   the format.

Hopefully, I've explained it.

Thanks,
-Aaron
Ben Pfaff Dec. 7, 2016, 6:41 p.m. UTC | #3
On Wed, Dec 07, 2016 at 01:08:43PM -0500, Aaron Conole wrote:
> Ben Pfaff <blp@ovn.org> writes:
> 
> > On Wed, Dec 07, 2016 at 10:55:54AM -0500, Aaron Conole wrote:
> >> Adds a new --format="xx={0},..." to the table library, which allows
> >> users to make table output formats to match their own needs.
> >> 
> >> Signed-off-by: Aaron Conole <aconole@redhat.com>
> >> ---
> >> v1 url:
> >>     https://mail.openvswitch.org/pipermail/ovs-dev/2016-November/325235.html
> >> 
> >> TODO: Still needs unit tests to check the format strings
> >>       One thing I want to do is put in the idea of string length to the
> >>       grammar.  Probably will be a few other additions (ex: text which is
> >>       omitted when the corresponding column value is empty).
> >> 
> >> NOTE: Submitted for early feedback
> >
> > Can you give an example of a use case?  If we're going to add this
> > feature for formatting, I'd like to know that it's going to be genuinely
> > useful to users.
> 
> Yes, sorry - I should have re-included my previous cover letter;
> 
> I am currently asked by some openstack folks to aid with 'debugability'
> - specifically, they want a change to the openflow printing such that
> they have data like:
> 
> COL1    COL2    COL3
> ====    ====    ====
> 
> They want to choose the columns 'as-needed', and I think they also want
> to change the formats.  Rather than code up multiple custom styles, I'd
> like to just give them the ability to change output formats as they
> wish.

I guess it just sounds very similar to --format=table.

> To accomplish this, I'm going down the following route:
> 
> 1. Add formatting option to the lib/table.{c,h}, such that we can
>    still express the existing output.
> 
> 2. Change ofp-print to use a table for flow dumping, with a default
>    format that preserves the current format, and the option of changing
>    the format.

#2 is interesting.
Aaron Conole Dec. 7, 2016, 7:15 p.m. UTC | #4
Ben Pfaff <blp@ovn.org> writes:

> On Wed, Dec 07, 2016 at 01:08:43PM -0500, Aaron Conole wrote:
>> Ben Pfaff <blp@ovn.org> writes:
>> 
>> > On Wed, Dec 07, 2016 at 10:55:54AM -0500, Aaron Conole wrote:
>> >> Adds a new --format="xx={0},..." to the table library, which allows
>> >> users to make table output formats to match their own needs.
>> >> 
>> >> Signed-off-by: Aaron Conole <aconole@redhat.com>
>> >> ---
>> >> v1 url:
>> >>     https://mail.openvswitch.org/pipermail/ovs-dev/2016-November/325235.html
>> >> 
>> >> TODO: Still needs unit tests to check the format strings
>> >>       One thing I want to do is put in the idea of string length to the
>> >>       grammar.  Probably will be a few other additions (ex: text which is
>> >>       omitted when the corresponding column value is empty).
>> >> 
>> >> NOTE: Submitted for early feedback
>> >
>> > Can you give an example of a use case?  If we're going to add this
>> > feature for formatting, I'd like to know that it's going to be genuinely
>> > useful to users.
>> 
>> Yes, sorry - I should have re-included my previous cover letter;
>> 
>> I am currently asked by some openstack folks to aid with 'debugability'
>> - specifically, they want a change to the openflow printing such that
>> they have data like:
>> 
>> COL1    COL2    COL3
>> ====    ====    ====
>> 
>> They want to choose the columns 'as-needed', and I think they also want
>> to change the formats.  Rather than code up multiple custom styles, I'd
>> like to just give them the ability to change output formats as they
>> wish.
>
> I guess it just sounds very similar to --format=table.

Agreed, this specific format is.

>> To accomplish this, I'm going down the following route:
>> 
>> 1. Add formatting option to the lib/table.{c,h}, such that we can
>>    still express the existing output.
>> 
>> 2. Change ofp-print to use a table for flow dumping, with a default
>>    format that preserves the current format, and the option of changing
>>    the format.
>
> #2 is interesting.

I don't currently see how to implement #2, while preserving the existing
output, unless I implement #1.  Is there a way to accomplish this that
I'm missing?  If not, is #2 compelling enough to accept #1?

Thanks for your time and for looking at this, Ben.
Ben Pfaff Dec. 13, 2016, 12:34 a.m. UTC | #5
On Wed, Dec 07, 2016 at 02:15:12PM -0500, Aaron Conole wrote:
> Ben Pfaff <blp@ovn.org> writes:
> > On Wed, Dec 07, 2016 at 01:08:43PM -0500, Aaron Conole wrote:
> >> Ben Pfaff <blp@ovn.org> writes:
> >> To accomplish this, I'm going down the following route:
> >> 
> >> 1. Add formatting option to the lib/table.{c,h}, such that we can
> >>    still express the existing output.
> >> 
> >> 2. Change ofp-print to use a table for flow dumping, with a default
> >>    format that preserves the current format, and the option of changing
> >>    the format.
> >
> > #2 is interesting.
> 
> I don't currently see how to implement #2, while preserving the existing
> output, unless I implement #1.  Is there a way to accomplish this that
> I'm missing?  If not, is #2 compelling enough to accept #1?

I don't understand how you are going to preserve the default format even
with #1.  I assumed you were going to need to write a separate code path
to do that.  Can you explain your plan?
Aaron Conole Dec. 13, 2016, 1:11 a.m. UTC | #6
Ben Pfaff <blp@ovn.org> writes:

> On Wed, Dec 07, 2016 at 02:15:12PM -0500, Aaron Conole wrote:
>> Ben Pfaff <blp@ovn.org> writes:
>> > On Wed, Dec 07, 2016 at 01:08:43PM -0500, Aaron Conole wrote:
>> >> Ben Pfaff <blp@ovn.org> writes:
>> >> To accomplish this, I'm going down the following route:
>> >> 
>> >> 1. Add formatting option to the lib/table.{c,h}, such that we can
>> >>    still express the existing output.
>> >> 
>> >> 2. Change ofp-print to use a table for flow dumping, with a default
>> >>    format that preserves the current format, and the option of changing
>> >>    the format.
>> >
>> > #2 is interesting.
>> 
>> I don't currently see how to implement #2, while preserving the existing
>> output, unless I implement #1.  Is there a way to accomplish this that
>> I'm missing?  If not, is #2 compelling enough to accept #1?
>
> I don't understand how you are going to preserve the default format even
> with #1.  I assumed you were going to need to write a separate code path
> to do that.

No - that's my last resort.

> Can you explain your plan?

I was thinking of creating a table which had column headings that were,
example:

  actions, in_port, priority, table number, etc.

Then a format string such as:

  [priority={priority},][table={tableno},]...

where things in the [] would only be printed if all elements in the row
were filled in, could preserve the original output (without the full
working code set it might be difficult to see what I mean), while also
offering the existing --table=XXX output.

In that sense, we could switch from dynamic string in all of the
ofp-print (and sub-calls) to just filling in column data - then have
pre-set table format strings that can be switched between.  That also
means any future manupulation (ex: changing the colors, or something
else) could just be handled the same way existing prints are handled.

-Aaron
Ben Pfaff Dec. 13, 2016, 9:07 p.m. UTC | #7
On Mon, Dec 12, 2016 at 08:11:16PM -0500, Aaron Conole wrote:
> Ben Pfaff <blp@ovn.org> writes:
> 
> > On Wed, Dec 07, 2016 at 02:15:12PM -0500, Aaron Conole wrote:
> >> Ben Pfaff <blp@ovn.org> writes:
> >> > On Wed, Dec 07, 2016 at 01:08:43PM -0500, Aaron Conole wrote:
> >> >> Ben Pfaff <blp@ovn.org> writes:
> >> >> To accomplish this, I'm going down the following route:
> >> >> 
> >> >> 1. Add formatting option to the lib/table.{c,h}, such that we can
> >> >>    still express the existing output.
> >> >> 
> >> >> 2. Change ofp-print to use a table for flow dumping, with a default
> >> >>    format that preserves the current format, and the option of changing
> >> >>    the format.
> >> >
> >> > #2 is interesting.
> >> 
> >> I don't currently see how to implement #2, while preserving the existing
> >> output, unless I implement #1.  Is there a way to accomplish this that
> >> I'm missing?  If not, is #2 compelling enough to accept #1?
> >
> > I don't understand how you are going to preserve the default format even
> > with #1.  I assumed you were going to need to write a separate code path
> > to do that.
> 
> No - that's my last resort.
> 
> > Can you explain your plan?
> 
> I was thinking of creating a table which had column headings that were,
> example:
> 
>   actions, in_port, priority, table number, etc.
> 
> Then a format string such as:
> 
>   [priority={priority},][table={tableno},]...
> 
> where things in the [] would only be printed if all elements in the row
> were filled in, could preserve the original output (without the full
> working code set it might be difficult to see what I mean), while also
> offering the existing --table=XXX output.

This may be challenging because of the number of columns.  in_port is
easy enough, but OVS has dozens (at least) of possible fields.  It also
has a nonuniform way to display fields.  For example, it shows the
Ethernet type in a general form as "dl_type=0x1234", but if it's an IP
packet then it just says "ip" instead, and if it's TCP then it just says
"tcp" instead of "dl_type=0x0800,nw_proto=6", and so on.
Aaron Conole Dec. 13, 2016, 9:43 p.m. UTC | #8
Ben Pfaff <blp@ovn.org> writes:

> On Mon, Dec 12, 2016 at 08:11:16PM -0500, Aaron Conole wrote:
>> Ben Pfaff <blp@ovn.org> writes:
>> 
>> > On Wed, Dec 07, 2016 at 02:15:12PM -0500, Aaron Conole wrote:
>> >> Ben Pfaff <blp@ovn.org> writes:
>> >> > On Wed, Dec 07, 2016 at 01:08:43PM -0500, Aaron Conole wrote:
>> >> >> Ben Pfaff <blp@ovn.org> writes:
>> >> >> To accomplish this, I'm going down the following route:
>> >> >> 
>> >> >> 1. Add formatting option to the lib/table.{c,h}, such that we can
>> >> >>    still express the existing output.
>> >> >> 
>> >> >> 2. Change ofp-print to use a table for flow dumping, with a default
>> >> >>    format that preserves the current format, and the option of changing
>> >> >>    the format.
>> >> >
>> >> > #2 is interesting.
>> >> 
>> >> I don't currently see how to implement #2, while preserving the existing
>> >> output, unless I implement #1.  Is there a way to accomplish this that
>> >> I'm missing?  If not, is #2 compelling enough to accept #1?
>> >
>> > I don't understand how you are going to preserve the default format even
>> > with #1.  I assumed you were going to need to write a separate code path
>> > to do that.
>> 
>> No - that's my last resort.
>> 
>> > Can you explain your plan?
>> 
>> I was thinking of creating a table which had column headings that were,
>> example:
>> 
>>   actions, in_port, priority, table number, etc.
>> 
>> Then a format string such as:
>> 
>>   [priority={priority},][table={tableno},]...
>> 
>> where things in the [] would only be printed if all elements in the row
>> were filled in, could preserve the original output (without the full
>> working code set it might be difficult to see what I mean), while also
>> offering the existing --table=XXX output.
>
> This may be challenging because of the number of columns.  in_port is
> easy enough, but OVS has dozens (at least) of possible fields.  It also
> has a nonuniform way to display fields.  For example, it shows the
> Ethernet type in a general form as "dl_type=0x1234", but if it's an IP
> packet then it just says "ip" instead, and if it's TCP then it just says
> "tcp" instead of "dl_type=0x0800,nw_proto=6", and so on.

I enjoy a challenge from time to time ;-)

Moreover, I'm willing to do the work.  Agreed, there's lots of it
(dozens is right), but I think it's an elegant solution.
Ben Pfaff Dec. 20, 2016, 1:54 a.m. UTC | #9
On Tue, Dec 13, 2016 at 04:43:32PM -0500, Aaron Conole wrote:
> Ben Pfaff <blp@ovn.org> writes:
> 
> > On Mon, Dec 12, 2016 at 08:11:16PM -0500, Aaron Conole wrote:
> >> Ben Pfaff <blp@ovn.org> writes:
> >> 
> >> > On Wed, Dec 07, 2016 at 02:15:12PM -0500, Aaron Conole wrote:
> >> >> Ben Pfaff <blp@ovn.org> writes:
> >> >> > On Wed, Dec 07, 2016 at 01:08:43PM -0500, Aaron Conole wrote:
> >> >> >> Ben Pfaff <blp@ovn.org> writes:
> >> >> >> To accomplish this, I'm going down the following route:
> >> >> >> 
> >> >> >> 1. Add formatting option to the lib/table.{c,h}, such that we can
> >> >> >>    still express the existing output.
> >> >> >> 
> >> >> >> 2. Change ofp-print to use a table for flow dumping, with a default
> >> >> >>    format that preserves the current format, and the option of changing
> >> >> >>    the format.
> >> >> >
> >> >> > #2 is interesting.
> >> >> 
> >> >> I don't currently see how to implement #2, while preserving the existing
> >> >> output, unless I implement #1.  Is there a way to accomplish this that
> >> >> I'm missing?  If not, is #2 compelling enough to accept #1?
> >> >
> >> > I don't understand how you are going to preserve the default format even
> >> > with #1.  I assumed you were going to need to write a separate code path
> >> > to do that.
> >> 
> >> No - that's my last resort.
> >> 
> >> > Can you explain your plan?
> >> 
> >> I was thinking of creating a table which had column headings that were,
> >> example:
> >> 
> >>   actions, in_port, priority, table number, etc.
> >> 
> >> Then a format string such as:
> >> 
> >>   [priority={priority},][table={tableno},]...
> >> 
> >> where things in the [] would only be printed if all elements in the row
> >> were filled in, could preserve the original output (without the full
> >> working code set it might be difficult to see what I mean), while also
> >> offering the existing --table=XXX output.
> >
> > This may be challenging because of the number of columns.  in_port is
> > easy enough, but OVS has dozens (at least) of possible fields.  It also
> > has a nonuniform way to display fields.  For example, it shows the
> > Ethernet type in a general form as "dl_type=0x1234", but if it's an IP
> > packet then it just says "ip" instead, and if it's TCP then it just says
> > "tcp" instead of "dl_type=0x0800,nw_proto=6", and so on.
> 
> I enjoy a challenge from time to time ;-)
> 
> Moreover, I'm willing to do the work.  Agreed, there's lots of it
> (dozens is right), but I think it's an elegant solution.

Do you have a basic plan for how to do it?  There's definitely room for
better display here, but I want the code to be clean and maintainable.
Aaron Conole Dec. 21, 2016, 3:07 p.m. UTC | #10
Ben Pfaff <blp@ovn.org> writes:

> On Tue, Dec 13, 2016 at 04:43:32PM -0500, Aaron Conole wrote:
>> Ben Pfaff <blp@ovn.org> writes:
>> 
>> > On Mon, Dec 12, 2016 at 08:11:16PM -0500, Aaron Conole wrote:
>> >> Ben Pfaff <blp@ovn.org> writes:
>> >> 
>> >> > On Wed, Dec 07, 2016 at 02:15:12PM -0500, Aaron Conole wrote:
>> >> >> Ben Pfaff <blp@ovn.org> writes:
>> >> >> > On Wed, Dec 07, 2016 at 01:08:43PM -0500, Aaron Conole wrote:
>> >> >> >> Ben Pfaff <blp@ovn.org> writes:
>> >> >> >> To accomplish this, I'm going down the following route:
>> >> >> >> 
>> >> >> >> 1. Add formatting option to the lib/table.{c,h}, such that we can
>> >> >> >>    still express the existing output.
>> >> >> >> 
>> >> >> >> 2. Change ofp-print to use a table for flow dumping, with a default
>> >> >> >>    format that preserves the current format, and the option of changing
>> >> >> >>    the format.
>> >> >> >
>> >> >> > #2 is interesting.
>> >> >> 
>> >> >> I don't currently see how to implement #2, while preserving the existing
>> >> >> output, unless I implement #1.  Is there a way to accomplish this that
>> >> >> I'm missing?  If not, is #2 compelling enough to accept #1?
>> >> >
>> >> > I don't understand how you are going to preserve the default format even
>> >> > with #1.  I assumed you were going to need to write a separate code path
>> >> > to do that.
>> >> 
>> >> No - that's my last resort.
>> >> 
>> >> > Can you explain your plan?
>> >> 
>> >> I was thinking of creating a table which had column headings that were,
>> >> example:
>> >> 
>> >>   actions, in_port, priority, table number, etc.
>> >> 
>> >> Then a format string such as:
>> >> 
>> >>   [priority={priority},][table={tableno},]...
>> >> 
>> >> where things in the [] would only be printed if all elements in the row
>> >> were filled in, could preserve the original output (without the full
>> >> working code set it might be difficult to see what I mean), while also
>> >> offering the existing --table=XXX output.
>> >
>> > This may be challenging because of the number of columns.  in_port is
>> > easy enough, but OVS has dozens (at least) of possible fields.  It also
>> > has a nonuniform way to display fields.  For example, it shows the
>> > Ethernet type in a general form as "dl_type=0x1234", but if it's an IP
>> > packet then it just says "ip" instead, and if it's TCP then it just says
>> > "tcp" instead of "dl_type=0x0800,nw_proto=6", and so on.
>> 
>> I enjoy a challenge from time to time ;-)
>> 
>> Moreover, I'm willing to do the work.  Agreed, there's lots of it
>> (dozens is right), but I think it's an elegant solution.
>
> Do you have a basic plan for how to do it?  There's definitely room for
> better display here, but I want the code to be clean and maintainable.

So, every plan that I've gone through starts with a separate code path
for the table approach if I want to stage it.

It probably makes sense to start there, and add the fields I've been
requested to add, first, and as part of the same series hook up
ovs-ofctl dump-flows to call that new code path.

Does that make sense?
Ben Pfaff Dec. 21, 2016, 3:58 p.m. UTC | #11
On Wed, Dec 21, 2016 at 10:07:51AM -0500, Aaron Conole wrote:
> Ben Pfaff <blp@ovn.org> writes:
> 
> > On Tue, Dec 13, 2016 at 04:43:32PM -0500, Aaron Conole wrote:
> >> Ben Pfaff <blp@ovn.org> writes:
> >> 
> >> > On Mon, Dec 12, 2016 at 08:11:16PM -0500, Aaron Conole wrote:
> >> >> Ben Pfaff <blp@ovn.org> writes:
> >> >> 
> >> >> > On Wed, Dec 07, 2016 at 02:15:12PM -0500, Aaron Conole wrote:
> >> >> >> Ben Pfaff <blp@ovn.org> writes:
> >> >> >> > On Wed, Dec 07, 2016 at 01:08:43PM -0500, Aaron Conole wrote:
> >> >> >> >> Ben Pfaff <blp@ovn.org> writes:
> >> >> >> >> To accomplish this, I'm going down the following route:
> >> >> >> >> 
> >> >> >> >> 1. Add formatting option to the lib/table.{c,h}, such that we can
> >> >> >> >>    still express the existing output.
> >> >> >> >> 
> >> >> >> >> 2. Change ofp-print to use a table for flow dumping, with a default
> >> >> >> >>    format that preserves the current format, and the option of changing
> >> >> >> >>    the format.
> >> >> >> >
> >> >> >> > #2 is interesting.
> >> >> >> 
> >> >> >> I don't currently see how to implement #2, while preserving the existing
> >> >> >> output, unless I implement #1.  Is there a way to accomplish this that
> >> >> >> I'm missing?  If not, is #2 compelling enough to accept #1?
> >> >> >
> >> >> > I don't understand how you are going to preserve the default format even
> >> >> > with #1.  I assumed you were going to need to write a separate code path
> >> >> > to do that.
> >> >> 
> >> >> No - that's my last resort.
> >> >> 
> >> >> > Can you explain your plan?
> >> >> 
> >> >> I was thinking of creating a table which had column headings that were,
> >> >> example:
> >> >> 
> >> >>   actions, in_port, priority, table number, etc.
> >> >> 
> >> >> Then a format string such as:
> >> >> 
> >> >>   [priority={priority},][table={tableno},]...
> >> >> 
> >> >> where things in the [] would only be printed if all elements in the row
> >> >> were filled in, could preserve the original output (without the full
> >> >> working code set it might be difficult to see what I mean), while also
> >> >> offering the existing --table=XXX output.
> >> >
> >> > This may be challenging because of the number of columns.  in_port is
> >> > easy enough, but OVS has dozens (at least) of possible fields.  It also
> >> > has a nonuniform way to display fields.  For example, it shows the
> >> > Ethernet type in a general form as "dl_type=0x1234", but if it's an IP
> >> > packet then it just says "ip" instead, and if it's TCP then it just says
> >> > "tcp" instead of "dl_type=0x0800,nw_proto=6", and so on.
> >> 
> >> I enjoy a challenge from time to time ;-)
> >> 
> >> Moreover, I'm willing to do the work.  Agreed, there's lots of it
> >> (dozens is right), but I think it's an elegant solution.
> >
> > Do you have a basic plan for how to do it?  There's definitely room for
> > better display here, but I want the code to be clean and maintainable.
> 
> So, every plan that I've gone through starts with a separate code path
> for the table approach if I want to stage it.
> 
> It probably makes sense to start there, and add the fields I've been
> requested to add, first, and as part of the same series hook up
> ovs-ofctl dump-flows to call that new code path.
> 
> Does that make sense?

Sure.  I'd prefer to make the whole thing a single series because in
this case I don't want to add library code that won't be used, but it
sounds like you already plan to do it that way.
diff mbox

Patch

diff --git a/lib/table.c b/lib/table.c
index 9158499..ca92848 100644
--- a/lib/table.c
+++ b/lib/table.c
@@ -532,6 +532,191 @@  table_print_json__(const struct table *table, const struct table_style *style)
     free(s);
 }
 
+/* Dynamic format parser. */
+
+struct table_output_action {
+    struct ds *print_value;       /* When !NULL, the text here is output. */
+    size_t column_id;             /* When print_value is NULL, this is the
+                                   * column to print, instead. */
+    enum table_format cell_style; /* TF_*. */
+};
+
+/* This compiles an input string into a series out output actions for the
+ * table formatting routines. */
+static size_t
+table_print_compile_format(const char *format,
+                           struct table_output_action **output,
+                           const struct table *table)
+{
+    struct table_output_action *actions;
+    char escape_vals[UCHAR_MAX] = {0};
+    bool escape_on_br = false;
+    struct ds *action = NULL;
+    const char *c = format;
+    bool escape = false;
+    size_t i = 0;
+
+    if (!format || !table) {
+        return 0;
+    }
+
+    actions = xmalloc(sizeof(struct table_output_action));
+
+    memset(escape_vals, '?', sizeof(escape_vals));
+    escape_vals['a'] = '\a';
+    escape_vals['b'] = '\b';
+    escape_vals['n'] = '\n';
+    escape_vals['r'] = '\r';
+    escape_vals['t'] = '\t';
+
+    for (; c; c++) {
+        if (!action) {
+            action = xmalloc(sizeof(struct ds));
+            ds_init(action);
+        }
+
+        switch (*c) {
+        /* default case - accumulate more in the action buffer */
+        default:
+            {
+                size_t ch = *c;
+                if (escape) {
+                    if (ch >= sizeof(escape_vals)) {
+                        ch = 0;
+                    }
+                    ch = escape_vals[ch];
+                    escape = false;
+                }
+
+                ds_put_char(action, ch);
+            }
+            break;
+
+        /* This is a pretty standard 'escape' character. */
+        case '\\':
+            if (!escape) {
+                escape = true;
+            } else {
+                ds_put_char(action, *c);
+                escape = false;
+            }
+            break;
+
+        /* The special state - start a new action block.  This printing
+         * state machine will try to have as few special states as possible,
+         * so the 'start' state is really just 'complete the previous block'
+         * state.  Unlike another block completion state, this state will
+         * not create a column lookup action.  */
+        case '{':
+            if (escape) {
+                ds_put_char(action, *c);
+                escape = false;
+                escape_on_br = true;
+            } else {
+                actions[i++].print_value = action;
+                action = NULL;
+                actions = xrealloc(actions, (i+1) * sizeof(actions[0]));
+            }
+            break;
+
+        /* The other special state - create a column lookup.  See the
+         * previous comments on the '{' case.  This is similar, except it
+         * does create a column lookup action. */
+        case '}':
+            if (escape || escape_on_br) {
+                ds_put_char(action, *c);
+                escape = false;
+                escape_on_br = false;
+            } else {
+                /* allow for ':' to be a special character. */
+                struct table_output_action *action_to_modify;
+                char *ch = strchr(ds_cstr(action), ':');
+                enum table_format fmt = TF_TABLE;
+
+                action_to_modify = &actions[i++];
+                if (ch) {
+                    struct table_style format_extractor;
+                    *ch++ = 0;
+                    table_parse_format(&format_extractor, ch);
+                    fmt = format_extractor.format;
+                }
+
+                /* find the header.  If not found, we fail. */
+                size_t hdrid;
+                for (hdrid = 0; hdrid < table->n_columns
+                         && strcmp(ds_cstr(action),
+                                   table->columns[hdrid].heading);
+                     ++hdrid) {
+                    /* skip forward until we find the header. */
+                }
+
+                if (hdrid >= table->n_columns) {
+                    char *end;
+                    hdrid = strtoul(ds_cstr(action), &end, 10);
+                    if (*end != '\0') {
+                        ovs_fatal(0, "Unknown column specified \"%s\"",
+                                  ds_cstr(action));
+                    }
+                }
+
+                action_to_modify->print_value = NULL;
+                action_to_modify->cell_style = fmt;
+                action_to_modify->column_id = hdrid;
+                actions = xrealloc(actions, (i+1) * sizeof(actions[0]));
+                ds_clear(action);
+                break;
+            }
+        }
+        if (!(*c)) {
+            actions[i++].print_value = action;
+            break;
+        }
+    }
+
+    *output = actions;
+    return i;
+}
+
+static void
+table_print_by_fmt__(const struct table *table,
+                     const struct table_style *style)
+{
+    struct table_output_action *actions;
+    size_t n_actions;
+
+    n_actions = table_print_compile_format(style->format_string, &actions,
+                                           table);
+
+    for (size_t row = 0; row < table->n_rows; ++row) {
+        for (size_t action = 0; action < n_actions; ++action) {
+            if (actions[action].print_value) {
+                fputs(ds_cstr(actions[action].print_value), stdout);
+            } else {
+                struct cell *cell = table_cell__(table, row,
+                                                 actions[action].column_id);
+                const char *str = cell_to_text(cell, style);
+
+                switch (actions[action].cell_style)
+                {
+                case TF_TABLE:
+                case TF_LIST:
+                case TF_JSON:
+                case TF_FORMATTED:
+                default:
+                    fputs(str, stdout);
+                    break;
+                case TF_CSV:
+                    table_print_csv_cell__(str);
+                    break;
+                case TF_HTML:
+                    table_escape_html_text__(str, strlen(str));
+                    break;
+                }
+            }
+        }
+    }
+}
+
 /* Parses 'format' as the argument to a --format command line option, updating
  * 'style->format'. */
 void
@@ -547,6 +732,9 @@  table_parse_format(struct table_style *style, const char *format)
         style->format = TF_CSV;
     } else if (!strcmp(format, "json")) {
         style->format = TF_JSON;
+    } else if (strchr(format, '{')) {
+        style->format = TF_FORMATTED;
+        style->format_string = xstrdup(format);
     } else {
         ovs_fatal(0, "unknown output format \"%s\"", format);
     }
@@ -592,5 +780,9 @@  table_print(const struct table *table, const struct table_style *style)
     case TF_JSON:
         table_print_json__(table, style);
         break;
+
+    case TF_FORMATTED:
+        table_print_by_fmt__(table, style);
+        break;
     }
 }
diff --git a/lib/table.h b/lib/table.h
index 85b8156..0880273 100644
--- a/lib/table.h
+++ b/lib/table.h
@@ -64,7 +64,8 @@  enum table_format {
     TF_LIST,                    /* One cell per line, one row per paragraph. */
     TF_HTML,                    /* HTML table. */
     TF_CSV,                     /* Comma-separated lines. */
-    TF_JSON                     /* JSON. */
+    TF_JSON,                    /* JSON. */
+    TF_FORMATTED                /* A format string was specified. */
 };
 
 enum cell_format {
@@ -78,9 +79,10 @@  struct table_style {
     enum cell_format cell_format; /* CF_*. */
     bool headings;              /* Include headings? */
     int json_flags;             /* CF_JSON: Flags for json_to_string(). */
+    char *format_string;        /* Valid only with TF_FORMATTED format. */
 };
 
-#define TABLE_STYLE_DEFAULT { TF_TABLE, CF_STRING, true, JSSF_SORT }
+#define TABLE_STYLE_DEFAULT { TF_TABLE, CF_STRING, true, JSSF_SORT, NULL }
 
 #define TABLE_OPTION_ENUMS                      \
     OPT_NO_HEADINGS,                            \
diff --git a/lib/table.man b/lib/table.man
index a8f1094..d47fc71 100644
--- a/lib/table.man
+++ b/lib/table.man
@@ -32,6 +32,23 @@  that represent OVSDB data or data types are expressed in the format
 described in the OVSDB specification; other cells are simply expressed
 as text strings.
 .RE
+.IP "\fBFORMAT\fR"
+\fBFORMAT\fR will emit each row of the table according to the format
+string specified.  The format string is comprised of either literal
+text, or replacement fields delimited by braces \fB{\fIfield\fR}\fR.
+Fields may be either a numeric value (which will be the ordinal
+position of the corresponding heading, index starting at 0), or the
+heading name.
+Additionally, the cell data emitted can be modified with an additional
+modification hint, corresponding to a cell output format.  To specify
+the hint, simply put a ':' after the heading, followed by the cell
+format string.
+.RS
+\fIExample\fR: \fB\-f "elemt0={0:json},elemt1={heading3}"\fR will emit
+rows as "elemt0=" followed by the cell corresponding to the first
+heading in json format, followed by ",elemt1=", followed by the cell
+corresponding to "heading3"
+.RE
 .RE
 .
 .IP "\fB\-d \fIformat\fR"