Re: [PATCH 2/2] udp_flow: move all udp_flow functions to udp_flow.c

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

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

On Mon, Aug 05, 2024 at 09:11:36AM +0200, Laurent Vivier wrote:
> On 03/08/2024 09:51, David Gibson wrote:
> > On Fri, Aug 02, 2024 at 06:10:36PM +0200, Laurent Vivier wrote:
> > > No code change.
> > > 
> > > They need to be exported to be available by the vhost-user version of
> > > passt.
> > > 
> > > Signed-off-by: Laurent Vivier <lvivier@redhat.com>
> > 
> > LGTM, with one nit:
> > 
> > > +/**
> > > + * udp_at_sidx() - Get UDP specific flow at given sidx
> > > + * @sidx:    Flow and side to retrieve
> > > + *
> > > + * Return: UDP specific flow at @sidx, or NULL of @sidx is invalid.  Asserts if
> > > + *         the flow at @sidx is not FLOW_UDP.
> > > + */
> > > +struct udp_flow *udp_at_sidx(flow_sidx_t sidx)
> > > +{
> > > +	union flow *flow = flow_at_sidx(sidx);
> > > +
> > > +	if (!flow)
> > > +		return NULL;
> > > +
> > > +	ASSERT(flow->f.type == FLOW_UDP);
> > > +	return &flow->udp;
> > > +}
> > 
> > udp_at_sidx() is so simple it probably makes more sense to have it as
> > an inline in the header file, rather than a regular function.  When it
> > was a local function, I was pretty much assuming the compiler would
> > inline it anyways.
> > 
> 
> I can't move it to udp_flow.h because udp_at_sidx() uses flow_at_sidx() that
> is defined in flow_table.h after udp_flow.h has already been included.
> flow_table.h includes udp_flow.h at its top because it needs "struct
> udp_flow" in union flow.

Ah, I didn't think of that.  Never mind, then.

Reviewed-by: David Gibson <david@gibson.dropbear.id.au>

-- 
David Gibson (he or they)	| 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 --]