[PATCH v2] tap: Update some function comments for accuracy

[PATCH v2] tap: Update some function comments for accuracy

[David Gibson]

Several of the tap_push_*() functions have doc comments claiming they take
the context pointer, but don't.  Some (tap_push_uh[46]) were broken fairly
recently, but others (tap_push_ip[46]h) have been broken for a long time.

Regardless, fix all the doc comments.

Reported-by: Stefano Brivio <sbrivio@redhat.com>
Fixes: 82a839be9 ("tap: break out building of udp header from tap_udp4_send function")
Fixes: 87e6a4644 ("tap: break out building of udp header from tap_udp6_send function")
Fixes: 2dbc622f5 ("tap: Split tap_ip4_send() into UDP and ICMP variants")
Fixes: 9d8dd8b6f ("tap: Split tap_ip6_send() into UDP and ICMP variants")

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 tap.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

v2:
 * Added missing "Fixes" tags from v1's commit message.

diff --git a/tap.c b/tap.c
index 95d309bd..f3d1f660 100644
--- a/tap.c
+++ b/tap.c
@@ -188,7 +188,7 @@ void *tap_push_l2h(const struct ctx *c, void *buf, uint16_t proto)
 
 /**
  * tap_push_ip4h() - Build IPv4 header for inbound packet, with checksum
- * @c:		Execution context
+ * @ip4h:	Buffer in which to build the IPv4 header
  * @src:	IPv4 source address
  * @dst:	IPv4 destination address
  * @l4len:	IPv4 payload length
@@ -217,7 +217,7 @@ void *tap_push_ip4h(struct iphdr *ip4h, struct in_addr src,
 
 /**
  * tap_push_uh4() - Build UDPv4 header with checksum
- * @c:		Execution context
+ * @uh:		Buffer in which to build the UDP header
  * @src:	IPv4 source address
  * @sport:	UDP source port
  * @dst:	IPv4 destination address
@@ -293,7 +293,7 @@ void tap_icmp4_send(const struct ctx *c, struct in_addr src, struct in_addr dst,
 
 /**
  * tap_push_ip6h() - Build IPv6 header for inbound packet
- * @c:		Execution context
+ * @ip6h:	Buffer in which to build the IPv6 header
  * @src:	IPv6 source address
  * @dst:	IPv6 destination address
  * @l4len:	L4 payload length
@@ -319,7 +319,7 @@ void *tap_push_ip6h(struct ipv6hdr *ip6h,
 
 /**
  * tap_push_uh6() - Build UDPv6 header with checksum
- * @c:		Execution context
+ * @uh:		Buffer in which to build the UDP header
  * @src:	IPv6 source address
  * @sport:	UDP source port
  * @dst:	IPv6 destination address
-- 
2.51.0

Re: [PATCH v2] tap: Update some function comments for accuracy

[Stefano Brivio]

On Mon, 13 Oct 2025 10:49:41 +1100
David Gibson <david@gibson.dropbear.id.au> wrote:

> Several of the tap_push_*() functions have doc comments claiming they take
> the context pointer, but don't.  Some (tap_push_uh[46]) were broken fairly
> recently, but others (tap_push_ip[46]h) have been broken for a long time.
> 
> Regardless, fix all the doc comments.
> 
> Reported-by: Stefano Brivio <sbrivio@redhat.com>
> Fixes: 82a839be9 ("tap: break out building of udp header from tap_udp4_send function")
> Fixes: 87e6a4644 ("tap: break out building of udp header from tap_udp6_send function")
> Fixes: 2dbc622f5 ("tap: Split tap_ip4_send() into UDP and ICMP variants")
> Fixes: 9d8dd8b6f ("tap: Split tap_ip6_send() into UDP and ICMP variants")
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>

Applied, with one minor change to tags (I plan to add some stuff,
including this bit, to the new CONTRIBUTING.md).

For consistency with the Linux kernel, we use SHAs abbreviated to 12
digits there, even though 9-digit abbreviations (produced by default
git-publish settings, I guess) are unlikely to ever lead to any
conflict for us.

Well, that's true at least until the day you all discover
https://github.com/not-an-aardvark/lucky-commit, but at that point the
number of digits wouldn't make a difference.

So, to keep the consistency consistent, I changed those to 12-digit
forms and dropped the extra newline (also added by git-publish). If
you fancy a script checking that for you, see:

  https://lore.kernel.org/all/20190220213729.49deb54f@redhat.com/

I pondered about adding that to hooks/, but it feels a bit like
overstepping. And I can "fix" those in seconds anyway (I regularly do,
I'm just pointing it out on this example as it's rather visible here).

-- 
Stefano

Re: [PATCH v2] tap: Update some function comments for accuracy

[David Gibson]

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

On Thu, Oct 16, 2025 at 01:46:24AM +0200, Stefano Brivio wrote:
> On Mon, 13 Oct 2025 10:49:41 +1100
> David Gibson <david@gibson.dropbear.id.au> wrote:
> 
> > Several of the tap_push_*() functions have doc comments claiming they take
> > the context pointer, but don't.  Some (tap_push_uh[46]) were broken fairly
> > recently, but others (tap_push_ip[46]h) have been broken for a long time.
> > 
> > Regardless, fix all the doc comments.
> > 
> > Reported-by: Stefano Brivio <sbrivio@redhat.com>
> > Fixes: 82a839be9 ("tap: break out building of udp header from tap_udp4_send function")
> > Fixes: 87e6a4644 ("tap: break out building of udp header from tap_udp6_send function")
> > Fixes: 2dbc622f5 ("tap: Split tap_ip4_send() into UDP and ICMP variants")
> > Fixes: 9d8dd8b6f ("tap: Split tap_ip6_send() into UDP and ICMP variants")
> > 
> > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> 
> Applied, with one minor change to tags (I plan to add some stuff,
> including this bit, to the new CONTRIBUTING.md).
> 
> For consistency with the Linux kernel, we use SHAs abbreviated to 12
> digits there, even though 9-digit abbreviations (produced by default
> git-publish settings, I guess)

No, git-publish doesn't manage Fixes tags.  It is the default for git
blame, though, which is where I would have copied them from

> are unlikely to ever lead to any
> conflict for us.
> 
> Well, that's true at least until the day you all discover
> https://github.com/not-an-aardvark/lucky-commit, but at that point the
> number of digits wouldn't make a difference.
> 
> So, to keep the consistency consistent, I changed those to 12-digit
> forms and dropped the extra newline (also added by git-publish). If
> you fancy a script checking that for you, see:
> 
>   https://lore.kernel.org/all/20190220213729.49deb54f@redhat.com/

Thanks, I've put that into my local hooks.

> I pondered about adding that to hooks/, but it feels a bit like
> overstepping. And I can "fix" those in seconds anyway (I regularly do,
> I'm just pointing it out on this example as it's rather visible here).
> 
> -- 
> Stefano
> 

-- 
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 --]

Re: [PATCH v2] tap: Update some function comments for accuracy

[Stefano Brivio]

On Thu, 16 Oct 2025 11:47:22 +1100
David Gibson <david@gibson.dropbear.id.au> wrote:

> On Thu, Oct 16, 2025 at 01:46:24AM +0200, Stefano Brivio wrote:
> > On Mon, 13 Oct 2025 10:49:41 +1100
> > David Gibson <david@gibson.dropbear.id.au> wrote:
> >   
> > > Several of the tap_push_*() functions have doc comments claiming they take
> > > the context pointer, but don't.  Some (tap_push_uh[46]) were broken fairly
> > > recently, but others (tap_push_ip[46]h) have been broken for a long time.
> > > 
> > > Regardless, fix all the doc comments.
> > > 
> > > Reported-by: Stefano Brivio <sbrivio@redhat.com>
> > > Fixes: 82a839be9 ("tap: break out building of udp header from tap_udp4_send function")
> > > Fixes: 87e6a4644 ("tap: break out building of udp header from tap_udp6_send function")
> > > Fixes: 2dbc622f5 ("tap: Split tap_ip4_send() into UDP and ICMP variants")
> > > Fixes: 9d8dd8b6f ("tap: Split tap_ip6_send() into UDP and ICMP variants")
> > > 
> > > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>  
> > 
> > Applied, with one minor change to tags (I plan to add some stuff,
> > including this bit, to the new CONTRIBUTING.md).
> > 
> > For consistency with the Linux kernel, we use SHAs abbreviated to 12
> > digits there, even though 9-digit abbreviations (produced by default
> > git-publish settings, I guess)  
> 
> No, git-publish doesn't manage Fixes tags.  It is the default for git
> blame, though, which is where I would have copied them from
> 
> > are unlikely to ever lead to any
> > conflict for us.
> > 
> > Well, that's true at least until the day you all discover
> > https://github.com/not-an-aardvark/lucky-commit, but at that point the
> > number of digits wouldn't make a difference.
> > 
> > So, to keep the consistency consistent, I changed those to 12-digit
> > forms and dropped the extra newline (also added by git-publish). If
> > you fancy a script checking that for you, see:
> > 
> >   https://lore.kernel.org/all/20190220213729.49deb54f@redhat.com/  
> 
> Thanks, I've put that into my local hooks.

Thanks. By the way, there's still the occasional blank line before
Signed-off-tag: in your patches, which I'm fairly sure comes from
git-publish.

Maybe it would be good to propose a patch for some kind of "Linux
kernel mode" for git-publish omitting that extra line, and probably a
couple of other tweaks? I don't remember what the other differences
(compared to QEMU) were.

It takes me less than a second to drop that line on merge though, so
it's definitely not an issue.

-- 
Stefano

Re: [PATCH v2] tap: Update some function comments for accuracy

[David Gibson]

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

On Wed, Oct 29, 2025 at 12:13:14AM +0100, Stefano Brivio wrote:
> On Thu, 16 Oct 2025 11:47:22 +1100
> David Gibson <david@gibson.dropbear.id.au> wrote:
> 
> > On Thu, Oct 16, 2025 at 01:46:24AM +0200, Stefano Brivio wrote:
> > > On Mon, 13 Oct 2025 10:49:41 +1100
> > > David Gibson <david@gibson.dropbear.id.au> wrote:
> > >   
> > > > Several of the tap_push_*() functions have doc comments claiming they take
> > > > the context pointer, but don't.  Some (tap_push_uh[46]) were broken fairly
> > > > recently, but others (tap_push_ip[46]h) have been broken for a long time.
> > > > 
> > > > Regardless, fix all the doc comments.
> > > > 
> > > > Reported-by: Stefano Brivio <sbrivio@redhat.com>
> > > > Fixes: 82a839be9 ("tap: break out building of udp header from tap_udp4_send function")
> > > > Fixes: 87e6a4644 ("tap: break out building of udp header from tap_udp6_send function")
> > > > Fixes: 2dbc622f5 ("tap: Split tap_ip4_send() into UDP and ICMP variants")
> > > > Fixes: 9d8dd8b6f ("tap: Split tap_ip6_send() into UDP and ICMP variants")
> > > > 
> > > > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>  
> > > 
> > > Applied, with one minor change to tags (I plan to add some stuff,
> > > including this bit, to the new CONTRIBUTING.md).
> > > 
> > > For consistency with the Linux kernel, we use SHAs abbreviated to 12
> > > digits there, even though 9-digit abbreviations (produced by default
> > > git-publish settings, I guess)  
> > 
> > No, git-publish doesn't manage Fixes tags.  It is the default for git
> > blame, though, which is where I would have copied them from
> > 
> > > are unlikely to ever lead to any
> > > conflict for us.
> > > 
> > > Well, that's true at least until the day you all discover
> > > https://github.com/not-an-aardvark/lucky-commit, but at that point the
> > > number of digits wouldn't make a difference.
> > > 
> > > So, to keep the consistency consistent, I changed those to 12-digit
> > > forms and dropped the extra newline (also added by git-publish). If
> > > you fancy a script checking that for you, see:
> > > 
> > >   https://lore.kernel.org/all/20190220213729.49deb54f@redhat.com/  
> > 
> > Thanks, I've put that into my local hooks.
> 
> Thanks. By the way, there's still the occasional blank line before
> Signed-off-tag: in your patches, which I'm fairly sure comes from
> git-publish.

Nope, I'm pretty sure git-publish doesn't touch those, it's just me.
I sometimes break up the tag lines in what seems like a more logical /
readable manner.  I can stop doing that if you'd prefer.

-- 
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 --]