Re: [PATCH 13/14] tap: Split tap_ip4_send() into UDP and ICMP variants

[Stefano Brivio <sbrivio@redhat.com>]

On Tue, 18 Oct 2022 23:07:58 +1100
David Gibson <david@gibson.dropbear.id.au> wrote:

> On Tue, Oct 18, 2022 at 05:06:34AM +0200, Stefano Brivio wrote:
> > On Mon, 17 Oct 2022 19:58:06 +1100
> > David Gibson <david@gibson.dropbear.id.au> wrote:
> >   
> > > tap_ip4_send() has special case logic to compute the checksums for UDP
> > > and ICMP packets, which is a mild layering violation.  By using a suitable
> > > helper we can split it into tap_udp4_send() and tap_icmp4_send() functions
> > > without greatly increasing the code size, this removing that layering
> > > violation.
> > > 
> > > We make some small changes to the interface while there.  In both cases
> > > we make the destination IPv4 address a parameter, which will be useful
> > > later.  For the UDP variant we make it take just the UDP payload, and it
> > > will generate the UDP header.  For the ICMP variant we pass in the ICMP
> > > header as before.  The inconsistency is because that's what seems to be
> > > the more natural way to invoke the function in the callers in each case.
> > > 
> > > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > > ---
> > >  icmp.c |  3 ++-
> > >  tap.c  | 75 +++++++++++++++++++++++++++++++++++++++++-----------------
> > >  tap.h  |  7 ++++--
> > >  3 files changed, 60 insertions(+), 25 deletions(-)
> > > 
> > > diff --git a/icmp.c b/icmp.c
> > > index 6493ea9..233acf9 100644
> > > --- a/icmp.c
> > > +++ b/icmp.c
> > > @@ -124,7 +124,8 @@ void icmp_sock_handler(const struct ctx *c, union epoll_ref ref,
> > >  			icmp_id_map[V4][id].seq = seq;
> > >  		}
> > >  
> > > -		tap_ip4_send(c, sr4->sin_addr.s_addr, IPPROTO_ICMP, buf, n);
> > > +		tap_icmp4_send(c, sr4->sin_addr.s_addr, tap_ip4_daddr(c),
> > > +			       buf, n);
> > >  	}
> > >  }
> > >  
> > > diff --git a/tap.c b/tap.c
> > > index 274f4ba..5792880 100644
> > > --- a/tap.c
> > > +++ b/tap.c
> > > @@ -127,20 +127,10 @@ static void *tap_l2_hdr(const struct ctx *c, void *buf, uint16_t proto)
> > >  	return eh + 1;
> > >  }
> > >  
> > > -/**
> > > - * tap_ip4_send() - Send IPv4 packet, with L2 headers, calculating L3/L4 checksums
> > > - * @c:		Execution context
> > > - * @src:	IPv4 source address
> > > - * @proto:	L4 protocol number
> > > - * @in:		Payload
> > > - * @len:	L4 payload length
> > > - */
> > > -void tap_ip4_send(const struct ctx *c, in_addr_t src, uint8_t proto,
> > > -		  const char *in, size_t len)  
> > 
> > I understand why you return ip(4)h + 1 here because I've just reviewed
> > 9/14, I wouldn't know otherwise:
> > 
> > /**
> >  * tap_ip4_hdr() - Build IPv4 header for inbound packet, with checksum
> >  * @c:		Execution context
> >  * @src:	IPv4 source address, network order
> >  * @dst:	IPv4 destination address, network order
> >  * @len:	L4 payload length
> >  * @proto:	L4 protocol number
> >  *
> >  * Return: pointer to write payload to
> >  */  
> 
> Oops, yes, forgot to add a function comment.  Done.
> 
> > > +static void *tap_ip4_hdr(char *buf, in_addr_t src, in_addr_t dst,
> > > +			 size_t len, uint8_t proto)
> > >  {
> > > -	char buf[USHRT_MAX];
> > > -	struct iphdr *ip4h = (struct iphdr *)tap_l2_hdr(c, buf, ETH_P_IP);
> > > -	char *data = (char *)(ip4h + 1);
> > > +	struct iphdr *ip4h = (struct iphdr *)buf;
> > >  
> > >  	ip4h->version = 4;
> > >  	ip4h->ihl = sizeof(struct iphdr) / 4;
> > > @@ -151,20 +141,61 @@ void tap_ip4_send(const struct ctx *c, in_addr_t src, uint8_t proto,
> > >  	ip4h->ttl = 255;
> > >  	ip4h->protocol = proto;
> > >  	ip4h->saddr = src;
> > > -	ip4h->daddr = tap_ip4_daddr(c);
> > > +	ip4h->daddr = dst;
> > >  	csum_ip4_header(ip4h);
> > > +	return ip4h + 1;
> > > +}
> > > +
> > > +/**
> > > + * tap_udp4_send() - Send UDP over IPv4 packet
> > > + * @c:		Execution context
> > > + * @src:	IPv4 source address
> > > + * @sport:	UDP source port
> > > + * @dst:	IPv4 destination address
> > > + * @dport:	UDP destination port
> > > + * @in:		UDP payload contents (not including UDP header)
> > > + * @len:	UDP payload length (not including UDP header)
> > > + */
> > > +/* cppcheck-suppress unusedFunction */
> > > +void tap_udp4_send(const struct ctx *c, in_addr_t src, in_port_t sport,
> > > +		   in_addr_t dst, in_port_t dport,
> > > +		   const void *in, size_t len)
> > > +{
> > > +	size_t udplen = len + sizeof(struct udphdr);
> > > +	char buf[USHRT_MAX];
> > > +	void *ip4h = tap_l2_hdr(c, buf, ETH_P_IP);
> > > +	void *uhp = tap_ip4_hdr(ip4h, src, dst, udplen, IPPROTO_UDP);  
> > 
> > Two observations:
> > 
> > - this saves one line and one cast, but it's really a bit unnatural that
> >   tap_ip4_hdr() doesn't point to the header it just made, or to nothing.
> > 
> >   I would rather have to +1 the return value or the original pointer
> >   instead or having this trick
> >   
> > > +	struct udphdr *uh = (struct udphdr *)uhp;
> > > +	char *data = (char *)(uh + 1);  
> > 
> > - it's longer, but in my opinion clearer, if we split a bit more clearly
> >   the components of the packet, that is, something like (untested):  
> 
> I don't really want to change this.  Yes, it's a bit counterintuitive
> at first blush, but there's a reason for this approach.
> 
> This style of a function which generates a header then points *after*
> it works even if the header it generates is of variable length.
> Advancing to the payload in the caller doesn't (at least not without
> breaking the abstraction I'm trying to generate with these helpers).
> 
> That's not just theoretical, because at some point I'd like to extend
> the l2_hdr function to also allocate space for the qemu socket length
> header.
> 
> I'm certainly open to name changes to make this behaviour more
> obvious, but I think returning the payload pointer not the header
> pointer makes for a better abstraction here.

Hmm, yes, I think the variable length case is a very valid point, and
also in terms of abstraction I see the advantage. There are just two
things I can think of:

- passing the end pointer as argument (not as practical as your
  solution, though)

- naming it tap_ip4_push_hdr(), tap_ip4_hdr_after(),
  tap_ip4_hdr_goto_next(), tap_ip4_leave_header_behind()... I can't
  think of anything better at this point. I'll keep thinking, but at the
  moment I'd be fine even with the current name.

-- 
Stefano