From mboxrd@z Thu Jan  1 00:00:00 1970
From: David Gibson <david@gibson.dropbear.id.au>
To: passt-dev@passt.top
Subject:
 Re: [PATCH 1/8] Improve types and names for port forwarding configuration
Date: Sat, 24 Sep 2022 19:06:36 +1000
Message-ID: <Yy7InDsQuoEZBhJb@yekko>
In-Reply-To: <20220924082832.6314f9ec@elisabeth>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="===============1779622359961372251=="

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

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:
>=20
> > 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/htm=
l/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.
>=20
> I see, but at the same time:
>=20
> - forgetting to use the new type should be as easy as forgetting to use
>   a define representing the size
>=20
> - 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]"
>=20
> > 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.
>=20
> 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?).
>=20
> Also introducing the first typedef in the whole project for something
> we can implement almost equivalently, hmm.
>=20
> 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.

--=20
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

--===============1779622359961372251==
Content-Type: application/pgp-signature
Content-Transfer-Encoding: base64
Content-Disposition: attachment; filename="signature.asc"
MIME-Version: 1.0

LS0tLS1CRUdJTiBQR1AgU0lHTkFUVVJFLS0tLS0KCmlRSXpCQUVCQ0FBZEZpRUVvVUx4V3U0L1dz
MGRCK1h0Z3lwWTRnRXdZU0lGQW1NdXlJQUFDZ2tRZ3lwWTRnRXcKWVNJYURBLy9lNU1RSHQyaUZG
d1RoK2s4SjkyeXl1VVk4UnJlVTJHak84WWE2TE96a2phZGRLM21MTHpHdjdkVQpEanVYTXlDRUlz
cGp6aVBWdG9QWmYvcG1JS1ZBaW13OG5FdExReFhZaWt2K29NU0ZpcFVIOUI0RXZKZ0FMckRECnR2
S0NDOWxZcGxWa3U2U0tzOFhpdWlRakwxNUZsRzhDc2R1ZCt2RmVDd05MNmZEdmF2MnRjOVBuMXhG
NTA1ajEKdzBSNUw4WDlETHp4RHVUMlYxMm5tdVJleDQ1cCswU2R3K1dOVUFZb0RzdktCV0ZvQ1Zl
bm1LMXl6TmFzWTU1UQp3bVVvdlZGV0ZOaHUrMncwV2pjaE5BWjBaL3V1bDBBTmE0TVNrWGtMS2FY
QkxlanVFelJ6b0pCUmtKZWNhVUxWCjhsWmhIWko1ZWd2ZHNWU25pNitsMEJtQVA0ZXVMQlJpQTBK
dy9SNUFwY1FnKzNwNnpZWm5Kd1J4WHBtbmNlZWsKbnpoa3ROdGxVNjdHQ1pPWW4xZFByUU41WGJo
S04rY1pyNUVXZmEvcGc0b1BxUFc1UFRKaHpldk9sMWVsc1VacgprZzBQMTB0UitLWUVZMStoaXFv
VmFLNHlKemhhb0hGZUtONnkxRTVwWWNMWW5EZCtoWFhDbDVVbjFGWnJnNTVuCmM2czBVZ2VIbU9a
UjZOVlM0eXR6YndkdU5pWXFjOW5xdVNSN0lQVEc2ZG0vVFY0K2NWS1Fxb3E5ekpubXpudk8KbCs5
aWNsVjdCaWY4V2NQT29KZDAxd2xKYkl3ZnJiNFk1em1za1pORm1UU1ZROXpBZjZhRkNzQUZ5TGV4
UU1aagpFVW5Pbnd6VzJHbVRYRyswWm1oMngvWVYrWVNlSXY2S1I3R2NMNmZmOTNFVFoyWE9WSVE9
Cj1zYm1zCi0tLS0tRU5EIFBHUCBTSUdOQVRVUkUtLS0tLQo=

--===============1779622359961372251==--