On Sat, Sep 24, 2022 at 08:28:32AM +0200, Stefano Brivio wrote:On Sat, 24 Sep 2022 12:57:25 +1000 David Gibson <david(a)gibson.dropbear.id.au> wrote:I think you convinced me. TBH, I only thought of the size define when I replied here, not when I originally wrote it. I'll respin with that instead of the typedef. -- David Gibson | I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibsonOn Sat, Sep 24, 2022 at 12:52:57AM +0200, Stefano Brivio wrote:I see, but at the same time: - forgetting to use the new type should be as easy as forgetting to use a define representing the size - the risk of doing stupid things (such as trying to pass this by value), despite the clear name of the typedef, looks to me slightly bigger than with "uint8_t ports[SIZE_PORTS]"On Fri, 23 Sep 2022 14:57:59 +1000 David Gibson <david(a)gibson.dropbear.id.au> wrote:As do I, in fact especially for array types, due to their idiosyncratic handling in C.[...] --- /dev/null +++ b/port_fwd.h @@ -0,0 +1,19 @@ +/* SPDX-License-Identifier: AGPL-3.0-or-later + * Copyright Red Hat + * Author: Stefano Brivio <sbrivio(a)redhat.com> + * Author: David Gibson <david(a)gibson.dropbear.id.au> + */ + +#ifndef PORT_FWD_H +#define PORT_FWD_H + +enum port_fwd_mode { + FWD_SPEC = 1, + FWD_NONE, + FWD_AUTO, + FWD_ALL, +}; + +typedef uint8_t port_fwd_map[DIV_ROUND_UP(USHRT_MAX, 8)];Given that this gets conveniently embedded in a struct in 2/8, could we avoid the typedef (or perhaps drop it after 2/8)? It makes the actual type less obvious to figure out, and in general I agree with most points from this slide deck: http://www.kroah.com/linux/talks/ols_2002_kernel_codingstyle_talk/html/mgp0…:) ...well, unless there's some resulting complexity I'm missing.Well, maybe. I added the typedef because of two things: 1) the fact that in one place we use a bitmap of this format separately from the rest for the 'exclude' map in conf_ports(). 2) the fact that we need the size of this map in get_bound_ports(). Having the typedef lets us handle both without duplicating the calculation of the size, which would mean more opportunities to get it wrong.I feel the advantages outweigh the disadvantages of a typedef in this case, but I won't be offended if you disagree. We could use a #define or const of the bitmap size instead.Well, I see now, and I don't have such a strong preference anymore... even though I would still prefer the define (perhaps SIZE_PORTS, based on NUM_PORTS from 7/8?). Also introducing the first typedef in the whole project for something we can implement almost equivalently, hmm. I don't know, if you still think it's clearly better than the alternative, let's go with it, I just have a slight preference against it at this point.