[ntar-workers] Re: Major rework / review of pcapng file format in CVS - please review

[Ulf Lamping]

Gianluca Varenni schrieb:
> - block types 0x0A0D0A00-0x0A0D0AFF. Ok for me to put them as 
> reserved. Do you know how this can happen with IE? I haven't checked 
> the ntar code, but I'm pretty sure i don't deal with that case (as it 
> should not be possible...).
I was using MSIE7 to get the file using the web interface of web.de (my 
"mail provider"). I'm not sure what's actually happened and where. When 
I downloaded the mail using thunderbird later on, the file was ok. So I 
guess it must be related the way the HTTP server (IIS?) transferred the 
file.
> - mandatory blocks. Maybe we need to specify that the IDB is mandatory 
> in each section of the file. The main problem here is that we define 
> what a section is later in the document.
Maybe explain it a bit better in the IDB section?
> - padding bytes. For security purposes they should be set to zero (or 
> any other common value). I don't think it will affect performance.
Then we should simply remove that TODO.
> - block total length with or without the padding bytes. Here is the 
> thing: blocks can always contain options. Options are 32-bit aligned 
> between each other. So options MUST start on 32-bit boundaries. So if 
> a block contains whatever options, its block total length is 32-bit 
> aligned (and the padding bytes are between the block data and the 
> options). The only case in which there can be ambiguity is when there 
> are no options. I would say that the block total length is 32-bit 
> aligned.
Agreed, but we should explicitly mention this to avoid confusion.
> In any case, if padding is added, the block body of the specific block 
> must contain some internal information specifying the actual length of 
> the data. E.g. in the case of the packet blocks, the captured 
> length/packet length fields clearly specify the data length without 
> padding.
Might be worth mentioned as well.
> - snaplen field: i don't think we need to signal "no limit", but we 
> definitely need to write that 0 is not a valid value (or it is?!?). 
I don't think we should say that 0 is invalid, e.g. if you only want the 
timestamp, length information but no data content itself.
> Also, the sentence "the portion that exceeds this value will not be 
> stored in the file" is a bit ambiguous. Maybe we should point out that 
> it always refers to the packets captured on this interface in the 
> current section of the file.
I guess that's obvious, but it wouldn't hurt to mention it.
> - "(TODO - It would be nice, to have a "invalid Interface ID" defined, 
> e.g. 0xFFFFFFFF)". No problem in adding an "invalid interface id" 
> value, but what's the purpose of it? Nothing came to my mind...
It's about how to work with the data inside an application. My idea is 
to have structs for the blocks and set the members of that struct to 
such an "invalid" marker if the option isn't available. If you don't 
have such a special invalid marker, you need to store two informations 
(a boolean "this value is valid" and the value itself). This is also 
true for lot's of the other values then ...
> - if_filter: i generally agree at splitting the if_filter option into 
> multiple ones, it makes everything easier to understand. Having 
> another layer of indirection (i.e. 1 option with another internal 
> "type" field) is something that I would avoid. 
FULLY ACK
> Also, what happens if multiple filters are defined in the options? Are 
> they implicitely AND-combined?
Although I don't know an application that would do so, it doesn't 
matter. As I understand it, these fields will only be displayed but not 
"used".
> - if_fcslen option: it's not clear what to write in this field when 
> the FCS is variable. 
simply ommit it (we might need to mention this)
> 0 and the right effective FCS length in each packet? 
0 is perfectly valid and should not indicate something else (Wireshark 
uses -1 to indicate "don't know FCS len").
> having the effective FCS length in an option is a bit dumb in my 
> opinion (if we do not have a way to declare that the FCS length is in 
> the option, properly decoding the packets will always require decoding 
> the options of the packet block!
I guess there's no good way to surround this. The other way would be to 
have a mandatory fcs len field with a 0-n value or -1 for "don't know" - 
but I don't know if that's any better ;-)
> - epb_hash option: the sentence "the hash covers only the packet, not 
> the header added by the capture driver" is ambiguous and doesn't make 
> a lot of sense anyway. Does it mean that if the linktype is radiotap 
> (or PPI), then the hash is computed on the packet excluding the 
> radiotap header? This would mean that the hash computation depends on 
> the specific linktype in use, which is dumb.
IMHO I don't see a good use for this at all ;-)
> -nres_ip4record and nres_ip6record: the name is quite inconsistent 
> with similar options in the IDB where we use the string IPv4 and 
> IPv6... same for ns_dnsIP6addr and ns_dnsIP4addr.
should be fixed
> -Interface ID option in the name resolution block. It can make sense. 
> It also introduces a lot of complications when reading a file. Which 
> made me think "what happens if a section has two NRBs with conflicting 
> records e.g. 10.0.0.1 --> www.foo.com and 10.0.0.1 --> www.bar.com?"
The problem is, this can happen already now AFAIK, with all this virtual 
server stuff having lot's of server names with the same IP address - but 
I'm not an expert on this!
> - Name resolution block, two "optional mechanisms" (records vs 
> options). I would vote for no. 
No to what?!?
> Having options only creates a possibly empty block which is something 
> i don't personally like too much. 
As the records are also optional, there is no difference to "Options 
only" that I can see - both can lead to an empty block ;-)
> But it's something we already have in place for the interface 
> statistics block.
> - isb_usrdeliv option: isn't this information redundant? I mean, it's 
> the number of packets captured and delivered to the application, i.e. 
> the ones that we have dumped to the trace file so far (for a specific 
> interface).
... asked by the person who is one of the WinPcap capturing experts ;-)

Ok, seriously, I don't thought about these Options much.
>
>
> Changes (committed on the CVS and on the web version of the document)
> - 64bit values are *not* aligned to 64bit boundaries. Added a TODO note.
ACK
> - options. The option length is the length without padding.
ACK
> - added a note specifying that since all the blocks are 32bit aligned, 
> the section length field in the SHB is always either 
> 0xFFFFFFFFFFFFFFFF or a 32-bit aligned value. Also, moved a note 
> related to it from the end of section 3.1 to the Section Length field 
> description.
ACK
> -typos here and there
> -specified that the if_IPv4addr and if_IPv6addr can be repeated 
> multiple times. Added an example for an ipv6 address.
ACK
> -in the if_speed option, modified "100.000.000" into "10000000 for 
> 100Mbps" (it gets confusing here in the US where the comma is used 
> instead of the dots)
ACK
> -added a note regarding the timestamps: they are not saved as 
> seconds/microseconds like in libpcap. They are saved as a single 
> 64-bit value split into 2 32-bit words.
Which timestamps do you refer to?!?
> - epb_hash option: fixed a typo, bit should read byte
ACK
> - simple packet block. The block data is aligned to 32bit as usual. 
> The picture does not show options. Can it contain options? Section 2.5 
> says that all the blocks can have options. 
ACK (especially a comment block should be possible). Or do we simply say 
that this block is for obsolete applications only, and those 
applications didn't added any blocks to this? If newly written 
applications should not use this block and old applications doesn't put 
an option in here, the picture is correct - but then we should mention 
"this situation" somehow.
> BTW this is one of the worst blocks to decode (in NTAR) as the 
> position of the options depends on a field (IDB::SnapLen) that is not 
> part of the block itself.
Yes, that's ugly.
> - added some options examples here and there
GOOD

Unfortunately, I won't have lot's of time to work on this doc in the 
next time :-(

Regards, ULFL