Re: [PATCH 1/8] Improve types and names for port forwarding configuration

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 3327 bytes --]

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:
> 
> > On Sat, Sep 24, 2022 at 12:52:57AM +0200, Stefano Brivio wrote:
> > > On Fri, 23 Sep 2022 14:57:59 +1000
> > > David Gibson <david(a)gibson.dropbear.id.au> wrote:
> > >   
> > > > [...]
> > > > 
> > > > --- /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/mgp00025.html  
> > 
> > As do I, in fact especially for array types, due to their
> > idiosyncratic handling in C.
> > 
> > > :) ...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 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]"
> 
> > 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.

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/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]