On Sat, Apr 26, 2008 at 07:07:27PM -0500, Jess Balint wrote:
> Hi, I've attached a patch to add support for field info in the MySQL
> dissector. I've also cleaned up a few things and fixed a small problem
> in the state tracking I added in a previous patch.
Looks nice, a few questions/remarks though:
- Do you have a sample trace that you could upload to the sample captures
page on wiki.wireshark.org? It will help running regression tests.
> @@ -30,6 +30,7 @@
> *
> * the protocol spec at
> * http://public.logicacmg.com/~redferni/mysql/MySQL-Protocol.html
> + * http://forge.mysql.com/wiki/MySQL_Internals_ClientServer_Protocol
> * and MySQL source code
Is the old URL still relevant or can it be removed?
> + if (tree) {
> + ti= proto_tree_add_item(tree, proto_mysql, tvb, offset, -1, FALSE);
> + mysql_tree= proto_item_add_subtree(ti, ett_mysql);
> + proto_tree_add_item(mysql_tree, hf_mysql_packet_length, tvb,
> + offset, 3, TRUE);
> + }
> + offset+= 3;
...
> - if (tree) {
> - ti= proto_tree_add_item(tree, proto_mysql, tvb, offset, -1, FALSE);
> - mysql_tree= proto_item_add_subtree(ti, ett_mysql);
> - proto_tree_add_item(mysql_tree, hf_mysql_packet_length, tvb,
> - offset, 3, TRUE);
> - }
> - offset+= 3;
Why do you move the stuff up?
> + { &hf_mysql_fld_type,
> + { "Type", "mysql.field.type",
> + FT_STRING, BASE_DEC, NULL, 0x0,
> + "Field: type", HFILL }},
...
> + proto_tree_add_string(tree, hf_mysql_fld_type, tvb, offset, -1,
> + val_to_str(tvb_get_guint8(tvb, offset), type_constants, "Unknown (%u)"));
Why do you use val_to_str here instead of VALS(type_constants) in the definition
of hf_mysql_fld_type?
Thanks for doing this!
Ciao
Joerg
--
Joerg Mayer <jmayer@xxxxxxxxx>
We are stuck with technology when what we really want is just stuff that
works. Some say that should read Microsoft instead of technology.