Wireshark-dev: Re: [Wireshark-dev] Patch for bug 1092 - need review

From: "ronnie sahlberg" <ronniesahlberg@xxxxxxxxx>
Date: Tue, 5 Sep 2006 14:02:59 +0000

looks good.
nothing really apart from cosmetics


1,
+			tftp_info = conversation_get_proto_data(conversation, proto_tftp);
+		        if (!tftp_info) {
+                		tftp_info = se_alloc(sizeof(tftp_conv_info_t));
+			}
+                	tftp_info->blocksize = blocksize;
+                	conversation_add_proto_data(conversation,
proto_tftp, tftp_info);

It would look nicer imho to only add the conversation protocol data if
it did not already exist

something like
tftp_info=c_get_proto_data()
if(!ftp_info){
   tftp_info=se_alloc();
   c_add_proto_data()
}
tftp_info->blocksize=


2, why pass conversation to tftp_dissect_options() when you only want
it to extract the conversation data?

why not pass tftp_info as parameter instead?

if so you may also want to change so there will unconditionally be a
tftp_info assigned from early in dissect_tftp()  just after it has
created the conversations :
i.e after these two lines :
+	  DISSECTOR_ASSERT(conversation);
	}

adding
  tftp_info=c_get_proto_data()
  if(!tftp_info){
	tftp_info=se_alloc()
      tftp_info-> initialize all fields
      c_add_proto_data()
   }

that would eliminate the need to get/add the proto data further down in the code





On 9/5/06, Joerg Mayer <jmayer@xxxxxxxxx> wrote:

As this is the first time I have coded something with conversations I'd
like to get some review before checking it in.

The attached patch will add  handling of the blocksize option as of
rfc2348 (see bug 1092).

  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.