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

[Xie He]

On Thu, Oct 29, 2020 at 4:53 PM Xie He <xie.he.0141@xxxxxxxxx> 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).