netdev - Re: [ovs-dev] [PATCH net-next V2 1/1] openvswitch: Declare ovs key structures using macros

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: <20190205202316.GS27607@ovn.org>
Date:   Tue, 5 Feb 2019 12:23:16 -0800
From:   Ben Pfaff <blp@....org>
To:     Gregory Rose <gvrose8192@...il.com>
Cc:     Eli Britstein <elibr@...lanox.com>,
        David Miller <davem@...emloft.net>,
        "yihung.wei@...il.com" <yihung.wei@...il.com>,
        "dev@...nvswitch.org" <dev@...nvswitch.org>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "simon.horman@...ronome.com" <simon.horman@...ronome.com>
Subject: Re: [ovs-dev] [PATCH net-next V2 1/1] openvswitch: Declare ovs key
 structures using macros

On Tue, Feb 05, 2019 at 10:22:09AM -0800, Gregory Rose wrote:
> 
> On 2/5/2019 4:02 AM, Eli Britstein wrote:
> > On 2/4/2019 10:07 PM, David Miller wrote:
> > > From: Yi-Hung Wei <yihung.wei@...il.com>
> > > Date: Mon, 4 Feb 2019 10:47:18 -0800
> > > 
> > > > For example, to see how 'struct ovs_key_ipv6' is defined, now we need
> > > > to trace how OVS_KEY_IPV6_FIELDS is defined, and how OVS_KEY_FIELD_ARR
> > > > and OVS_KEY_FIELD defined.  I think it makes the header file to be
> > > > more complicated.
> > > I completely agree.
> > > 
> > > Unless this is totally unavoidable, I do not want to apply a patch
> > > which makes reading and auditing the networking code more difficult.
> > This technique is discussed for example in
> > https://stackoverflow.com/questions/6635851/real-world-use-of-x-macros,
> > and I found existing examples of using it in the kernel tree:
> > 
> > __BPF_FUNC_MAPPER in commit ebb676daa1a34 ("bpf: Print function name in
> > addition to function id")
> > 
> > __AAL_STAT_ITEMS and __SONET_ITEMS in commit 607ca46e97a1b ("UAPI:
> > (Scripted) Disintegrate include/linux"), the successor of commit
> > 1da177e4c3f4 ("Linux-2.6.12-rc2"). I didn't dig deeper to the past.
> > 
> > I can agree it makes that H file a bit more complicated, but for sure
> > less than ## macros that are widely used.
> > 
> > However, I think the alternatives of generating such defines by some
> > scripts, or having the fields in more than one place are even worse, so
> > it is a kind of unavoidable.
> 
> Why is using a script to generate defines for the requirements of your
> application or driver so bad?  Your patch
> turns openvswitch.h into some hardly readable code - using a script and
> having a machine output the macros
> your application or driver needs seems like a much better alternative to me.

In addition, one of the reasons that developers prefer to avoid
duplication is because of the need to synchronize copies when one of
them changes.  This doesn't apply in the same way to these structures,
because they are ABIs that will not change.