netdev - Re: [PATCH net-next v2 4/4] net: hdlc_fr: Add support for any Ethertype

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAJht_EPggyhiaROvReNJ4hCwQ6+Z0wf4zHADrSAaT8jBE0J+1w@mail.gmail.com>
Date:   Thu, 29 Oct 2020 17:49:31 -0700
From:   Xie He <xie.he.0141@...il.com>
To:     Willem de Bruijn <willemdebruijn.kernel@...il.com>
Cc:     Jakub Kicinski <kuba@...nel.org>,
        "David S. Miller" <davem@...emloft.net>,
        Network Development <netdev@...r.kernel.org>,
        linux-kernel <linux-kernel@...r.kernel.org>,
        Krzysztof Halasa <khc@...waw.pl>
Subject: Re: [PATCH net-next v2 4/4] net: hdlc_fr: Add support for any Ethertype

On Thu, Oct 29, 2020 at 4:53 PM Xie He <xie.he.0141@...il.com> wrote:
>
> > Does it make sense to define a struct snap_hdr instead of manually
> > casting all these bytes?
>
> > And macros or constant integers to self document these kinds of fields.
>
> Yes, we can define a struct snap_hdr, like this:
>
> struct snap_hdr {
>         u8 oui[3];
>         __be16 pid;
> } __packed;
>
> And then the fr_snap_parse function could be like this:
>
> static int fr_snap_parse(struct sk_buff *skb, struct pvc_device *pvc)
> {
>        struct snap_hdr *hdr = (struct snap_hdr *)skb->data;
>
>        if (hdr->oui[0] == OUI_ETHERTYPE_1 &&
>            hdr->oui[1] == OUI_ETHERTYPE_2 &&
>            hdr->oui[2] == OUI_ETHERTYPE_3) {
>                if (!pvc->main)
>                        return -1;
>                skb->dev = pvc->main;
>                skb->protocol = hdr->pid; /* Ethertype */
>                skb_pull(skb, 5);
>                skb_reset_mac_header(skb);
>                return 0;
>
>        } else if (hdr->oui[0] == OUI_802_1_1 &&
>                   hdr->oui[1] == OUI_802_1_2 &&
>                   hdr->oui[2] == OUI_802_1_3) {
>                if (hdr->pid == htons(PID_ETHER_WO_FCS)) {
>
> Would this look cleaner?

Actually I don't think this is significantly cleaner than the previous
version of code. A reader of this code may still wonder what are the
values of all these macros, and he/she may still want to look up the
values of them. The comment in the previous version of code has made
the meaning of these values very clear, and the reader of the code
would not need to go to another place of this file to find the values.

The struct snap_hdr eliminates a cast, but only one cast. So it might
not be very necessary, either. Introducing this struct also makes the
reader need to go to another place of this file to look up the
definition of this struct. So it does not significantly improve the
readability (IMHO).