Wireshark-bugs: [Wireshark-bugs] [Bug 3467] Memcache Textual Protocol dissector patch
https://bugs.wireshark.org/bugzilla/show_bug.cgi?id=3467
--- Comment #7 from Rama Chitta <rama@xxxxxxxxx> 2009-05-18 14:12:13 PDT ---
Thanks for the review.
Please find my comments inline:
> In frame 6 and 15 in your example we get two tree items with "Memcache
> Protocol", the latest with only \r\n, which I think is wrong because we only
> have one command. Are you able to fix this?
Thanks for the Review Stig. I think I should be able to fix this. I don't see
this all the time. Will post a fix to it.
> In frame 25, 31 and 32 it seems like some continuation packages, but the does
> not show up correct. Is this due to the dissector or the sent packages?
>
This is because my packet capture size is limited to 300 bytes and the response
is bigger than that. That is why you are not able to see the whole picture...In
such a case, I print whatever I could read and leave the rest out. Seems like a
fair thing to do?
> The binary dissector is using expert info's to provide info when something is
> wrong. Maybe you are able to add this for the text dissector?
>
yes, will look into this and see if they are applicable to the textual
dissector.
> I also find some "goto" statements which could easily be omitted by rewriting
> the code. Personally I dont like goto because they obfusticate the code flow.
>
I'm not a big fan of goto statements either. But I thought they are helpful in
this dissector. Yes, I'll try to replace them. Should not be a problem.
> It's difficult to read a patch with a lot of whitespace changes. I have
> rewritten some of the code in the first patch (coding style is like clothing
> style, but I prefer having the same style in one file), so you can have a look
> at the latest patch.
>
Thanks for the second patch. Yes, my vi settings were completely different from
yours. Got your second patch. It should help now.
--
Configure bugmail: https://bugs.wireshark.org/bugzilla/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.
