[Spice-devel,v3,16/51] Allows to specify some new attributes for wireshark

Submitted by Frediano Ziglio on July 21, 2015, 4:45 p.m.

Details

Message ID 1437497181-26929-17-git-send-email-fziglio@redhat.com
State New
Headers show

Not browsing as part of any series.

Commit Message

Frediano Ziglio July 21, 2015, 4:45 p.m.
To make output more useful fields from the protocol should have
additional information like description, name, type and so on.

List of attributes added:
- ws_desc, just a simple description
- ws_name name of the field. See below
- ws allow to specify a description and a name. Useful as easy to
  type. If you have a name there is no sense to have no description
- ws_type allows to override type detected, for instance to
  specify is a boolean instead of an int
- ws_base like ws_type but for base (hexadecimal instead of decimal)
- ws_txt and ws_txt_n allows to specify formatting. The difference
  between formatting and description is that description is static
  while these new attributes contain a formatting string as first
  argument followed by arguments representing fields or INDEX for
  the index if we are dumping a structure contained in an array.
  The distinction between ws_txt and ws_txt_n is the item contained
  in an array or not
- ws_inline, this is tricky attribute. It allow to embed structure
  dissecting in the same function. This allow format string and other
  field (for instance switch or array sizes) to be seen by current
  generated function
- ws_as, dissect this structure as another type. Useful to avoid
  changing the protocol but show a slightly modified version.
  This is used for instance to show two fields like x and y as a
  single point. Could also be used to dump a binary data with
  more detail but avoid to change marshalling/demarshalling

These attributes required some changes in the parser as previously
arguments could be only integers and identifiers while they require
string and multiple identifiers (like "this.that").

In wireshark names are important as they can be used to do
queries about packet with specific features.

Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
---
 python_modules/ptypes.py       | 29 +++++++++++++++++++++++++++--
 python_modules/spice_parser.py | 13 +++++++++++--
 2 files changed, 38 insertions(+), 4 deletions(-)

Patch hide | download patch | download mbox

diff --git a/python_modules/ptypes.py b/python_modules/ptypes.py
index cc6960f..bbf158e 100644
--- a/python_modules/ptypes.py
+++ b/python_modules/ptypes.py
@@ -108,6 +108,26 @@  valid_attributes={
     # for a switch this indicates that on network
     # will occupy always same size (maximum size required for all members)
     'fixedsize',
+
+    # description
+    'ws_desc',
+    # field name
+    'ws_name',
+    # combine description + name
+    # name will be used for filtering
+    'ws',
+    # type (BOOLEAN, STRINGZ)
+    'ws_type',
+    # force wireshark base
+    'ws_base',
+    # text to use for format, you can specify parameters
+    'ws_txt',
+    'ws_txt_n',
+    # put structure not in a separate function
+    # used to be able to retrieve fields from parent
+    'ws_inline',
+    # handle this container as another type
+    'ws_as',
 }
 
 attributes_with_arguments={
@@ -119,6 +139,13 @@  attributes_with_arguments={
     'minor',
     'bytes_count',
     'virtual',
+    'ws',
+    'ws_desc',
+    'ws_type',
+    'ws_base',
+    'ws_txt',
+    'ws_txt_n',
+    'ws_as',
 }
 
 def fix_attributes(attribute_list):
@@ -131,8 +158,6 @@  def fix_attributes(attribute_list):
         if not name in attributes_with_arguments:
             if len(lst) > 0:
                 raise Exception("Attribute %s specified with options" % name)
-        elif len(lst) > 1:
-            raise Exception("Attribute %s has more than 1 argument" % name)
         attrs[name] = lst
     return attrs
 
diff --git a/python_modules/spice_parser.py b/python_modules/spice_parser.py
index 97af8b2..a0f969a 100644
--- a/python_modules/spice_parser.py
+++ b/python_modules/spice_parser.py
@@ -3,7 +3,7 @@  import six
 try:
     from pyparsing import Literal, CaselessLiteral, Word, OneOrMore, ZeroOrMore, \
             Forward, delimitedList, Group, Optional, Combine, alphas, nums, restOfLine, cStyleComment, \
-            alphanums, ParseException, ParseResults, Keyword, StringEnd, replaceWith
+            alphanums, ParseException, ParseResults, Keyword, StringEnd, replaceWith, QuotedString
 except ImportError:
     six.print_("Module pyparsing not found.")
     exit(1)
@@ -77,20 +77,29 @@  def SPICE_BNF():
         switch_    = Keyword("switch")
         default_   = Keyword("default")
         case_      = Keyword("case")
+        ws_        = Keyword("@ws")
+        ws_txt_    = Keyword("@ws_txt") | Keyword("@ws_txt_n")
+        ws_desc_   = Keyword("@ws_desc")
 
         identifier = Word( alphas, alphanums + "_" )
+        multi_identifier = Word( alphas, alphanums + "_." )
         enumname = Word( alphanums + "_" )
 
         integer = ( Combine( CaselessLiteral("0x") + Word( nums+"abcdefABCDEF" ) ) |
                     Word( nums+"+-", nums ) ).setName("int").setParseAction(cvtInt)
 
+        string = QuotedString(quoteChar='"', escChar='\\')
+
         typename = identifier.copy().setParseAction(lambda toks : ptypes.TypeRef(str(toks[0])))
 
         # This is just normal "types", i.e. not channels or messages
         typeSpec = Forward()
 
         attributeValue = integer ^ identifier
-        attribute = Group(Combine ("@" + identifier) + Optional(lparen + delimitedList(attributeValue) + rparen))
+        ws = ws_ + lparen + string + comma + multi_identifier + rparen
+        ws_desc = ws_desc_ + lparen + string + rparen
+        ws_txt = ws_txt_ + lparen + string + ZeroOrMore(comma + multi_identifier) + rparen
+        attribute = Group(ws | ws_desc | ws_txt | (Combine ("@" + identifier) + Optional(lparen + delimitedList(attributeValue) + rparen)))
         attributes = Group(ZeroOrMore(attribute))
         arraySizeSpecImage = Group(image_size_ + lparen + integer + comma + identifier + comma + identifier + rparen)
         arraySizeSpecBytes = Group(bytes_ + lparen + identifier + comma + identifier + rparen)

Comments

On Tue, Jul 21, 2015 at 05:45:46PM +0100, Frediano Ziglio wrote:
> To make output more useful fields from the protocol should have
> additional information like description, name, type and so on.
> 
> List of attributes added:
> - ws_desc, just a simple description
> - ws_name name of the field. See below

This attribute does not seem to be used

> - ws allow to specify a description and a name. Useful as easy to
>   type. If you have a name there is no sense to have no description
> - ws_type allows to override type detected, for instance to
>   specify is a boolean instead of an int
> - ws_base like ws_type but for base (hexadecimal instead of decimal)

I guess you could give a list of the valid bases here

> - ws_txt and ws_txt_n allows to specify formatting. The difference
>   between formatting and description is that description is static
>   while these new attributes contain a formatting string as first
>   argument followed by arguments representing fields or INDEX for

or the special value 'INDEX' would probably be a bit more clear

>   the index if we are dumping a structure contained in an array.
>   The distinction between ws_txt and ws_txt_n is the item contained
>   in an array or not

'is whether the item is contained in an array or not'

> - ws_inline, this is tricky attribute. It allow to embed structure

'a tricky attribute. It allows to embed..'

>   dissecting in the same function. This allow format string and other
>   field (for instance switch or array sizes) to be seen by current
>   generated function

'This allows... other fields ... by the function currently being
generated' ?

> - ws_as, dissect this structure as another type. Useful to avoid
>   changing the protocol but show a slightly modified version.
>   This is used for instance to show two fields like x and y as a
>   single point. Could also be used to dump a binary data with
>   more detail but avoid to change marshalling/demarshalling

"This is also used to dump binary data in wireshark with more details
without changing the current marshalling/demarshalling code (eg agent
messages or QUIC image data)"

> 
> These attributes required some changes in the parser as previously
> arguments could be only integers and identifiers while they require
> string and multiple identifiers (like "this.that").
> 
> In wireshark names are important as they can be used to do
> queries about packet with specific features.
> 
> Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
> ---
>  python_modules/ptypes.py       | 29 +++++++++++++++++++++++++++--
>  python_modules/spice_parser.py | 13 +++++++++++--
>  2 files changed, 38 insertions(+), 4 deletions(-)
> 
> diff --git a/python_modules/ptypes.py b/python_modules/ptypes.py
> index cc6960f..bbf158e 100644
> --- a/python_modules/ptypes.py
> +++ b/python_modules/ptypes.py
> @@ -108,6 +108,26 @@ valid_attributes={
>      # for a switch this indicates that on network
>      # will occupy always same size (maximum size required for all members)
>      'fixedsize',
> +
> +    # description
> +    'ws_desc',
> +    # field name
> +    'ws_name',
> +    # combine description + name
> +    # name will be used for filtering
> +    'ws',
> +    # type (BOOLEAN, STRINGZ)
> +    'ws_type',
> +    # force wireshark base
> +    'ws_base',
> +    # text to use for format, you can specify parameters
> +    'ws_txt',
> +    'ws_txt_n',
> +    # put structure not in a separate function
> +    # used to be able to retrieve fields from parent
> +    'ws_inline',
> +    # handle this container as another type
> +    'ws_as',
>  }
>  
>  attributes_with_arguments={
> @@ -119,6 +139,13 @@ attributes_with_arguments={
>      'minor',
>      'bytes_count',
>      'virtual',
> +    'ws',
> +    'ws_desc',
> +    'ws_type',
> +    'ws_base',
> +    'ws_txt',
> +    'ws_txt_n',
> +    'ws_as',
>  }
>  
>  def fix_attributes(attribute_list):
> @@ -131,8 +158,6 @@ def fix_attributes(attribute_list):
>          if not name in attributes_with_arguments:
>              if len(lst) > 0:
>                  raise Exception("Attribute %s specified with options" % name)
> -        elif len(lst) > 1:
> -            raise Exception("Attribute %s has more than 1 argument" % name)
>          attrs[name] = lst
>      return attrs
>  
> diff --git a/python_modules/spice_parser.py b/python_modules/spice_parser.py
> index 97af8b2..a0f969a 100644
> --- a/python_modules/spice_parser.py
> +++ b/python_modules/spice_parser.py
> @@ -3,7 +3,7 @@ import six
>  try:
>      from pyparsing import Literal, CaselessLiteral, Word, OneOrMore, ZeroOrMore, \
>              Forward, delimitedList, Group, Optional, Combine, alphas, nums, restOfLine, cStyleComment, \
> -            alphanums, ParseException, ParseResults, Keyword, StringEnd, replaceWith
> +            alphanums, ParseException, ParseResults, Keyword, StringEnd, replaceWith, QuotedString
>  except ImportError:
>      six.print_("Module pyparsing not found.")
>      exit(1)
> @@ -77,20 +77,29 @@ def SPICE_BNF():
>          switch_    = Keyword("switch")
>          default_   = Keyword("default")
>          case_      = Keyword("case")
> +        ws_        = Keyword("@ws")
> +        ws_txt_    = Keyword("@ws_txt") | Keyword("@ws_txt_n")
> +        ws_desc_   = Keyword("@ws_desc")
>  
>          identifier = Word( alphas, alphanums + "_" )
> +        multi_identifier = Word( alphas, alphanums + "_." )
>          enumname = Word( alphanums + "_" )
>  
>          integer = ( Combine( CaselessLiteral("0x") + Word( nums+"abcdefABCDEF" ) ) |
>                      Word( nums+"+-", nums ) ).setName("int").setParseAction(cvtInt)
>  
> +        string = QuotedString(quoteChar='"', escChar='\\')
> +
>          typename = identifier.copy().setParseAction(lambda toks : ptypes.TypeRef(str(toks[0])))
>  
>          # This is just normal "types", i.e. not channels or messages
>          typeSpec = Forward()
>  
>          attributeValue = integer ^ identifier
> -        attribute = Group(Combine ("@" + identifier) + Optional(lparen + delimitedList(attributeValue) + rparen))
> +        ws = ws_ + lparen + string + comma + multi_identifier + rparen
> +        ws_desc = ws_desc_ + lparen + string + rparen
> +        ws_txt = ws_txt_ + lparen + string + ZeroOrMore(comma + multi_identifier) + rparen
> +        attribute = Group(ws | ws_desc | ws_txt | (Combine ("@" + identifier) + Optional(lparen + delimitedList(attributeValue) + rparen)))
>          attributes = Group(ZeroOrMore(attribute))
>          arraySizeSpecImage = Group(image_size_ + lparen + integer + comma + identifier + comma + identifier + rparen)
>          arraySizeSpecBytes = Group(bytes_ + lparen + identifier + comma + identifier + rparen)
> -- 
> 2.1.0
> 
> _______________________________________________
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/spice-devel
> 
> On Tue, Jul 21, 2015 at 05:45:46PM +0100, Frediano Ziglio wrote:
> > To make output more useful fields from the protocol should have
> > additional information like description, name, type and so on.
> > 
> > List of attributes added:
> > - ws_desc, just a simple description
> > - ws_name name of the field. See below
> 
> This attribute does not seem to be used
>

Hard to spot but is used :-)

...

Frediano
On Thu, Jul 23, 2015 at 10:49:58AM -0400, Frediano Ziglio wrote:
> > 
> > On Tue, Jul 21, 2015 at 05:45:46PM +0100, Frediano Ziglio wrote:
> > > To make output more useful fields from the protocol should have
> > > additional information like description, name, type and so on.
> > > 
> > > List of attributes added:
> > > - ws_desc, just a simple description
> > > - ws_name name of the field. See below
> > 
> > This attribute does not seem to be used
> >
> 
> Hard to spot but is used :-)

In spice.proto? 'git grep ws_name' turns up empty at least.

Christophe
> On Thu, Jul 23, 2015 at 10:49:58AM -0400, Frediano Ziglio wrote:
> > > 
> > > On Tue, Jul 21, 2015 at 05:45:46PM +0100, Frediano Ziglio wrote:
> > > > To make output more useful fields from the protocol should have
> > > > additional information like description, name, type and so on.
> > > > 
> > > > List of attributes added:
> > > > - ws_desc, just a simple description
> > > > - ws_name name of the field. See below
> > > 
> > > This attribute does not seem to be used
> > >
> > 
> > Hard to spot but is used :-)
> 
> In spice.proto? 'git grep ws_name' turns up empty at least.
> 
> Christophe
> 

Actually even from test.proto, I should consider adding to it (it's one reason I have a separate .proto file).
For instance can be used in enumerators to specify just the name as the description is based on enumerator name.

Frediano
On Thu, Jul 23, 2015 at 11:15:14AM -0400, Frediano Ziglio wrote:
> > On Thu, Jul 23, 2015 at 10:49:58AM -0400, Frediano Ziglio wrote:
> > > > 
> > > > On Tue, Jul 21, 2015 at 05:45:46PM +0100, Frediano Ziglio wrote:
> > > > > To make output more useful fields from the protocol should have
> > > > > additional information like description, name, type and so on.
> > > > > 
> > > > > List of attributes added:
> > > > > - ws_desc, just a simple description
> > > > > - ws_name name of the field. See below
> > > > 
> > > > This attribute does not seem to be used
> > > >
> > > 
> > > Hard to spot but is used :-)
> > 
> > In spice.proto? 'git grep ws_name' turns up empty at least.
> > 
> > Christophe
> > 
> 
> Actually even from test.proto, I should consider adding to it (it's
> one reason I have a separate .proto file).
> For instance can be used in enumerators to specify just the name as
> the description is based on enumerator name.

I understand what it can be used for, but I don't know if it's useful to
add something that is not used currently in actual .proto files. This is
kind of similar to the ptr32 support you removed imo.

Christophe
> On Thu, Jul 23, 2015 at 11:15:14AM -0400, Frediano Ziglio wrote:
> > > On Thu, Jul 23, 2015 at 10:49:58AM -0400, Frediano Ziglio wrote:
> > > > > 
> > > > > On Tue, Jul 21, 2015 at 05:45:46PM +0100, Frediano Ziglio wrote:
> > > > > > To make output more useful fields from the protocol should have
> > > > > > additional information like description, name, type and so on.
> > > > > > 
> > > > > > List of attributes added:
> > > > > > - ws_desc, just a simple description
> > > > > > - ws_name name of the field. See below
> > > > > 
> > > > > This attribute does not seem to be used
> > > > >
> > > > 
> > > > Hard to spot but is used :-)
> > > 
> > > In spice.proto? 'git grep ws_name' turns up empty at least.
> > > 
> > > Christophe
> > > 
> > 
> > Actually even from test.proto, I should consider adding to it (it's
> > one reason I have a separate .proto file).
> > For instance can be used in enumerators to specify just the name as
> > the description is based on enumerator name.
> 
> I understand what it can be used for, but I don't know if it's useful to
> add something that is not used currently in actual .proto files. This is
> kind of similar to the ptr32 support you removed imo.
> 
> Christophe
> 

Actually I was wrong, you cannot use it... removed!

Frediano