Re: [PATCH] build: Fix errors of TCP_REPAIR_* undeclared

[Stefano Brivio <sbrivio@redhat.com>]

Thanks for the patch! A couple of comments, all minor:

On Mon,  1 Sep 2025 17:45:10 +0800
Yumei Huang <yuhuang@redhat.com> wrote:

> Fix the following errors on systems with glibc < 2.29:

Before anything, it's customary to credit the original author of a
patch, even if it was (very) different. Something like:

Based on an original patch by Dongsheng, fix ...

and I think you should also Cc: them (they might want to test / comment
/ review), you can fetch the contact from
https://bugs.passt.top/show_bug.cgi?id=121.

> 
> tcp.c: In function ‘tcp_flow_repair_on’:
> tcp.c:2787:38: error: ‘TCP_REPAIR_ON’ undeclared (first use in this function); did you mean ‘TCP_REPAIR’?
>   if ((rc = repair_set(c, conn->sock, TCP_REPAIR_ON)))
>                                       ^~~~~~~~~~~~~
>                                       TCP_REPAIR
> tcp.c:2787:38: note: each undeclared identifier is reported only once for each function it appears in
> tcp.c: In function ‘tcp_flow_repair_off’:
> tcp.c:2807:38: error: ‘TCP_REPAIR_OFF’ undeclared (first use in this function); did you mean ‘TCP_REPAIR’?
>   if ((rc = repair_set(c, conn->sock, TCP_REPAIR_OFF)))
>                                       ^~~~~~~~~~~~~~
>                                       TCP_REPAIR
> make: *** [Makefile:94: passt] Error 1
> 
> Signed-off-by: Yumei Huang <yuhuang@redhat.com>

Just before this tag, we typically add any references, always using the
"Link:" tag for simplicity.

Many other projects use separate tags such as "Bug:", "Bugzilla:",
"Closes:" and so on, but we don't, so that it's a bit easier to
automate parsing, if needed. In this case that would be:

Link: https://bugs.passt.top/show_bug.cgi?id=121

and perhaps a Reported-by: tag would also be good to have, here.

> ---
>  linux_dep.h    | 6 ++++++
>  passt-repair.c | 1 +
>  2 files changed, 7 insertions(+)
> 
> diff --git a/linux_dep.h b/linux_dep.h
> index 240f50a..c200444 100644
> --- a/linux_dep.h
> +++ b/linux_dep.h
> @@ -135,6 +135,12 @@ struct tcp_info_linux {
>  #define CLOSE_RANGE_UNSHARE	(1U << 1)
>  #endif
>  
> +#ifndef TCP_REPAIR_ON
> +#define TCP_REPAIR_ON           1
> +#define TCP_REPAIR_OFF          0
> +#define TCP_REPAIR_OFF_NO_WP    -1      /* Turn off without window probes */

We use tabs for indentation, and a full tab is displayed as 8 characters
wide. For example:

#define TCP_REPAIR_ON		1
                      ^^ two tabs here, so that TCP_REPAIR_OFF_NO_WP is
                      aligned too

If you need a suggestion for editor showing whitespace (marking tabs and
spaces separately), I use Geany and it does that. I think others use
Emacs, I'm not sure how it works there.

> +#endif
> +
>  __attribute__ ((weak))
>  /* cppcheck-suppress funcArgNamesDifferent */
>  int close_range(unsigned int first, unsigned int last, int flags) {
> diff --git a/passt-repair.c b/passt-repair.c
> index 8c59d7e..c3c140f 100644
> --- a/passt-repair.c
> +++ b/passt-repair.c
> @@ -40,6 +40,7 @@
>  #include <linux/seccomp.h>
>  
>  #include "seccomp_repair.h"
> +#include "linux_dep.h"
>  
>  #define SCM_MAX_FD 253 /* From Linux kernel (include/net/scm.h), not in UAPI */
>  #define REPAIR_EXT		".repair"

The rest looks good to me!

-- 
Stefano