Re: [PATCH RESEND] packet: Deliver VLAN TPID to userspace

[Daniel Borkmann <dborkman@redhat.com>]

On 11/04/2013 06:16 PM, Ben Hutchings wrote:
> On Wed, 2013-10-30 at 16:03 +0900, Atzm Watanabe wrote:
> [...]
>> Should it be structures as below?
>>
>> struct tpacket_hdr_variant1 {
>>          __u32   tp_rxhash;
>>          __u32   tp_vlan_tci;
>> };
>>
>> struct tpacket_hdr_variant2 {
>>          __u32   tp_rxhash;
>>          __u32   tp_vlan_tci;
>>          __u32   tp_vlan_tpid;
>> };
>>
>> struct tpacket3_hdr {
>>          __u32           tp_next_offset;
>>          __u32           tp_sec;
>>          __u32           tp_nsec;
>>          __u32           tp_snaplen;
>>          __u32           tp_len;
>>          __u32           tp_status;
>>          __u16           tp_mac;
>>          __u16           tp_net;
>>          /* pkt_hdr variants */
>>          union {
>>                  struct tpacket_hdr_variant1 hv1;
>>                  struct tpacket_hdr_variant2 hv2;
>>          };
>> };
>>
>> If it's ok, I'd like to send the patch v2.
> [...]
>
> I think this makes sense.
>
> Also add a BUILD_BUG_ON(TPACKET3_HDRLEN != 48) somewhere.

Sorry for coming late into this discussion.

I think the packet_mmap API with its 3 different API versions
already is a "bit" terrible, and should only be further extended
for user space if there is a _*huge*_ (!) improvement somewhere
(e.g. in terms of packet capturing/transmission performance) that
would justify it. I'm thinking that this could e.g. be in terms
of packet capturing and transmission performance.

Otherwise, each time we want to add a new member, we add yet
another subheader for userspace into tpacket, making it even
more complicated to use, and adding more "legacy" API fragments?

To solve your problem, why don't you add a socket option that,
if _enabled_, would reconstruct the vlan header as it was seen
on the "wire" and push that up to userspace, just like libpcap
does internally. It's not perfect either, but perhaps a better
way to go. What do you think?