Re: [Firebird-devel] Wire protocol specification and wireshark dissector

[mark c. <mar...@ob...>]

Hi Vlad,

Thanks for your thoughts.

Here's a few of mine...

Vlad Khorsun wrote:
>> Hi folks,
>>
>> I've been working on fixing up the wireshark plugin for Firebird.  One 
>> of the things that would have really helped in this process would have 
>> been up-to-date documentation.  After reading what I could find, 
>> including Carlos Guzman Alvarez's paper, I decided that I would be best 
>> off parsing the xdr_protocol routine in protocol.cpp and generating the 
>> required wireshark code.  
>>     
>
>     Correct. I'd said xdr_protocol is the *only* valid source of protocol spec.
>
>   
Agreed - the source _is_ the documentation. Actually, there are too many 
opaque data structures to be really useful but it's a good start.
>> This I have done. As a byproduct, I have 
>> produced an HTML document that I have requested to be posted on 
>> http://www.firebirdnews.org. 
>>
>> In order to do this, I needed to tag up the source code with some extra 
>> info - comments about the packet type (only for documentation) and field 
>> names (for documenation and wireshark). Here's a simple sample:
>>
>>  //: Resquest information about a previously prepared statement
>>  case op_info_sql:
>>    info = &p->p_info;
>>    //: DatabaseHandle
>>    MAP(xdr_short, reinterpret_cast<SSHORT&>(info->p_info_object));
>>    //: IncarnationOfObject
>>    MAP(xdr_short, reinterpret_cast<SSHORT&>(info->p_info_incarnation));
>>    //: RequestBuffer
>>    MAP(xdr_cstring_const, info->p_info_items);
>>    //: ReplyBufferLength
>>    MAP(xdr_short, reinterpret_cast<SSHORT&>(info->p_info_buffer_length));
>>    DEBUG_PRINTSIZE(xdrs, p->p_operation);
>>    return P_TRUE(xdrs, p);
>>     
>
>     I'm sorry, but i see no value in such comments for developer who read the code.
> It adds nothing but require to synchronize it with code changes.
>   
The comments _aren't_ for the developer who reads the code - they are 
for the parser that generates the protocol document and the wireshark 
dissector.  This question is really the entire point of my email - is it 
worth the effort in maintaining the 'comments' in order to generate the 
protocol document and the wireshark dissector? I believe it is, but 
then, I'm not the one who will have to do it.
>  
>   
>> It would be possible to include the the field name in the MAP macro like 
>> this
>>
>> case op_info_sql:
>>    info = &p->p_info;
>>    MAP("DatabaseHandle", xdr_short, 
>> reinterpret_cast<SSHORT&>(info->p_info_object));
>>
>> which I think reads better. The MAP macro can ignore the extra parameter 
>> - executable code would not be affected.
>>
>> How do you feel about including this in the production code?
>>
>> There are some advantages:
>>
>>    * better internal documentation
>>     
>
>     Not sure what you mean by "internal documentation". The code is already easy to 
> read and understand. The names of the fields of corresponding structures usually
> self-explained and good commented in protocol.h file. 
>   
Vlad,  I suspect that you know the code very well so it is not 
surprising that you find the names of the fields self explanatory. I do 
not know the code well and can assure you that they were not immediately 
self-explanatory to me. If you want more people to help with development 
then the easier you make it, the more help you'll get.
>>    * ability to generate a Wire Protocol spec (which seems to be asked
>>      for regularly)
>>    * ability to generate a wireshark dissector.
>>
>> There are some disadvantages:
>>
>>    * extra comments need to be added as the protocol changes
>>    * it may be necessary to split out some cases as, while their
>>      structure is the same, the field names are not. This will result
>>      in a small increase in code size but probably no increase in
>>      execution time.
>>    * opaque data structures need extra 'hints' in the comments.
>>
>> If you like the idea, I'll merge my code with the latest protocol.cpp. 
>> If you don't, I'll delete it. ;-)
>>     
>
>     I don't want to offend you but i see no value in such change for Firebird developers...
>   
I'm not offended. :-)
>   
>> Of course, I'll make the parser/generator code available. It's written 
>> in Delphi but would be easy to port.
>>     
>
>     It is very good intention and i want to thank you for that. But... xdr_protocol is already
> available and easy to port :) 
>   
I'm talking about the parser/generator, not protocol.cpp. The parser 
takes xdr_protocol as _input._
> Regards,
> Vlad
>
> PS As for wireshark plugin you worked on : would it be better if it will be build with FB ?
>     In this case it will be updated almost automagically with FB. You can use necessary
> files from /remote to build plugin... May be you already used this approach, though :)
>   

Well, yes. The parser could easily be built as part of the firebird 
build process. I think it should be. But it requires comments to be 
maintained in protocol.cpp and the parser should be ported from Delphi 
to C++. I consider the delphi parser to be a prototype. I'm happy to do 
the port to production level code but only if the core firebird 
developers will maintain the comments in protocol.cpp. This is why I 
wrote the original email.

Regards,
Mark Chambers