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

[Stefano Brivio <sbrivio@redhat.com>]

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

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.

-- 
Stefano