From mboxrd@z Thu Jan  1 00:00:00 1970
From: Stefano Brivio <sbrivio@redhat.com>
To: passt-dev@passt.top
Subject:
 Re: [PATCH 1/8] Improve types and names for port forwarding configuration
Date: Sat, 24 Sep 2022 08:28:32 +0200
Message-ID: <20220924082832.6314f9ec@elisabeth>
In-Reply-To: <Yy5yFVvyQD0QkvtT@yekko>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="===============8986549733959449445=="

--===============8986549733959449445==
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: quoted-printable

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:
> >  =20
> > > [...]
> > >=20
> > > --- /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 =3D 1,
> > > +	FWD_NONE,
> > > +	FWD_AUTO,
> > > +	FWD_ALL,
> > > +};
> > > +
> > > +typedef uint8_t port_fwd_map[DIV_ROUND_UP(USHRT_MAX, 8)]; =20
> >=20
> > 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:
> >=20
> >   http://www.kroah.com/linux/talks/ols_2002_kernel_codingstyle_talk/html/=
mgp00025.html =20
>=20
> As do I, in fact especially for array types, due to their
> idiosyncratic handling in C.
>=20
> > :) ...well, unless there's some resulting complexity I'm missing. =20
>=20
> 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.

--=20
Stefano


--===============8986549733959449445==--