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

[Gianluca Varenni]

Some comments:
- "Logical block hierarchy": I like it, I would probably leave it like it is 
for now.
- 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...).
- 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.
- padding bytes. For security purposes they should be set to zero (or any 
other common value). I don't think it will affect performance.
- 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.
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.
- 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?!?). 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.
- "(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...
- 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. Also, what happens if multiple filters are 
defined in the options? Are they implicitely AND-combined?
- if_fcslen option: it's not clear what to write in this field when the FCS 
is variable. 0 and the right effective FCS length in each packet? 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!
- 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.
-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.
-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?"
- Name resolution block, two "optional mechanisms" (records vs options). I 
would vote for no. Having options only creates a possibly empty block which 
is something i don't personally like too much. 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).


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.
- options. The option length is the length without padding.
- 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.
-typos here and there
-specified that the if_IPv4addr and if_IPv6addr can be repeated multiple 
times. Added an example for an ipv6 address.
-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)
-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.
- epb_hash option: fixed a typo, bit should read byte
- 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. 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.
- added some options examples here and there

Have a nice day
GV

----- Original Message ----- 
From: "Ulf Lamping" <ulf.lamping at web.de>
To: <ntar-workers at winpcap.org>
Cc: "Gianluca Varenni" <gianluca.varenni at cacetech.com>
Sent: Wednesday, October 17, 2007 3:11 AM
Subject: Major rework / review of pcapng file format in CVS - please review


> Hi List!
>
> I've just committed into ntar CVS  lot's of changes to the PCAPng 
> Specification document. I didn't add any new elements, but tried to 
> clarify, remove inconsistencies, ...
>
> Some of the changes were discussed with Gianluca before, some of them I've 
> just tried to get a better document - but this very certainly needs 
> further work on.
>
>
> What I've changed ...
>
> Lot's of editing:
> change: put block types back into one section (looked ugly) - but keep the 
> seperation of them
> add: section "Logical Block Hierarchy" - an attempt to visualize the 
> dependencies of the blocks (I'm still not 100% satisfied)
> change: some more file format examples
> add: give example content to the Option fields throughout the doc (lot's 
> of TODO here, especially for IPv6)
> change: ASCII art: now includes the block header, byte offsets, ...
> change: obsolete "Packet Block" moved further below other packet blocks 
> (it confuses to mention the obsolete block first)
> add: "recommended file name extension .pcapng" section (to avoid the 
> situation that we have for libpcap files today)
> add: possible block type 0x0A0D0A00-0x0A0D0AFF (caused by buggy ASCII 
> translation of MSIE I've already seen "in the wild")
> add: link layer header appendix (almost empty, needs further work)
> change: add some more TODO and change my previous XXX markers also to TODO
>
> Incompatible changes in "Interface Statistics Block" (this block wasn't 
> used before, so we still can change it):
> change Interface ID to be 32 bits (was 16 bits and a 16 bit reserved 
> field - so it's now consistent with other parts of the doc)
> isb_starttime/isb_endtime now uses fractional seconds according to 
> if_tsaccur (so it's consistent with other timestamps - was formerly fixed 
> to nanoseconds)
>
>
> I think this spec is now better than before (but still far from being 
> perfect), please have a look and comment, hope to hear from you ...
>
> Regards, ULFL