Wireshark-dev: Re: [Wireshark-dev] [PATCH] add field support to MySQL dissector
From: Joerg Mayer <[email protected]>
Date: Sun, 27 Apr 2008 14:59:39 +0200
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                                           <[email protected]>
We are stuck with technology when what we really want is just stuff that
works. Some say that should read Microsoft instead of technology.