linux-kernel - Re: [PATCH v8 13/25] m68k: Dispatch nvram

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]

Date:   Mon, 31 Dec 2018 09:12:42 +1100 (AEDT)
From:   Finn Thain <fthain@...egraphics.com.au>
To:     LEROY Christophe <christophe.leroy@....fr>
cc:     Joshua Thompson <funaho@...ai.org>,
        linuxppc-dev <linuxppc-dev@...ts.ozlabs.org>,
        Geert Uytterhoeven <geert@...ux-m68k.org>,
        linux-m68k <linux-m68k@...ts.linux-m68k.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Arnd Bergmann <arnd@...db.de>
Subject: Re: [PATCH v8 13/25] m68k: Dispatch nvram_ops calls to Atari or Mac
 functions

On Sun, 30 Dec 2018, LEROY Christophe wrote:

> > > 
> > > Since the operations are almost entirely distinct, why not have two 
> > > separate 'nvram_ops' instances here that each refer to just the set 
> > > they actually need?
> > > 
> > 
> > The reason for that is that I am alergic to code duplication. But I'll 
> > change it if you think it matters. BTW, this patch has already been 
> > acked by Geert.
> 
> I agree it would be cleaner, as it would also avoid this 
> m68k_nvram_get_size() wouldn't it ?
> 

No, that function makes run-time decisions. #ifdef won't work.

> I don't see potential code duplication here, do you ?
> 

Here's my problem with Arnd's suggestion. Consider this C code,

#ifdef FOO
const struct nvram_ops arch_nvram_ops = {
	/* ... */
}
#else
const struct nvram_ops arch_nvram_ops = {
        /* ... */
}
#endif

Lets say you write a hypothetical patch to remove the 'const'. Now you 
have two 'const' keywords to edit, and you have the risk of overlooking 
one of them. The solution to this problem is sometimes referred to as 
"DRY", meaning Don't Repeat Yourself:

const struct nvram_ops arch_nvram_ops = {
        /* ... */
#ifdef FOO
        /* ... */
#else
        /* ... */
#endif
        /* ... */
}

But I'm over-simplifying. Arnd's alternative actually goes like this,

#if defined(CONFIG_MAC) && !defined(CONFIG_ATARI)
const struct nvram_ops arch_nvram_ops = {
        /* ... */
}
#elif !defined(CONFIG_MAC) && defined(CONFIG_ATARI)
const struct nvram_ops arch_nvram_ops = {
        /* ... */
}
#elif defined(CONFIG_MAC) && defined(CONFIG_ATARI)
const struct nvram_ops arch_nvram_ops = {
        /* ... */
}
#endif

So, you're right, this isn't "duplication", it's "triplication".

-- 

> Christophe
>