[PATCH ipvsadm 0/5] Debian package fixes

[PATCH ipvsadm 0/5] Debian package fixes

[Jeremy Sowden]

The Debian package of ipvsadm carries a few patches.  Some of them have been
around for years.  None of them is specific to Debian, so I'm forwarding them
upstream.

Patch 1 bumps the maximum weight accepted by ipvsadm.
Patch 2 fixes an unclear usage error message.
Patches 3-5 make some improvements to the Makefiles.

Jeremy Sowden (5):
  ipvsadm: increase maximum weight value
  ipvsadm: fix ambiguous usage error message
  Use variable for pkg-config in Makefiles
  Support environmental and command-line `*FLAGS` variable in Makefiles
  Make sure libipvs.a is built before ipvsadm

 Makefile         | 19 +++++++++++--------
 ipvsadm.8        |  2 +-
 ipvsadm.c        | 19 +++++++++++--------
 libipvs/Makefile | 29 +++++++++++++++--------------
 4 files changed, 38 insertions(+), 31 deletions(-)

-- 
2.45.2

[PATCH ipvsadm 1/5] ipvsadm: increase maximum weight value

[Jeremy Sowden]

The IPVS kernel module accepts weight up to 2,147,483,647 (INT_MAX), but ipvsadm
limits it to 65,535.  Raise the user space maximum to match the kernel.

Link: https://bugs.debian.org/814348
Signed-off-by: Jeremy Sowden <azazel@debian.org>
---
 ipvsadm.8 | 2 +-
 ipvsadm.c | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/ipvsadm.8 b/ipvsadm.8
index b3bc822aaafc..3d7541a4bf62 100644
--- a/ipvsadm.8
+++ b/ipvsadm.8
@@ -386,7 +386,7 @@ servers are added or modified.
 .B -w, --weight \fIweight\fP
 \fIWeight\fP is an integer specifying the capacity  of a server
 relative to the others in the pool. The valid values of \fIweight\fP
-are 0 through to 65535. The default is 1. Quiescent servers are
+are 0 through to 2147483647. The default is 1. Quiescent servers are
 specified with a weight of zero. A quiescent server will receive no
 new jobs but still serve the existing jobs, for all scheduling
 algorithms distributed with the Linux Virtual Server. Setting a
diff --git a/ipvsadm.c b/ipvsadm.c
index 663d47ab1138..42f31a20e596 100644
--- a/ipvsadm.c
+++ b/ipvsadm.c
@@ -762,7 +762,7 @@ parse_options(int argc, char **argv, struct ipvs_command_entry *ce,
 		case 'w':
 			set_option(options, OPTC_WEIGHT);
 			if ((ce->dest.weight =
-			     string_to_number(optarg, 0, 65535)) == -1)
+			     string_to_number(optarg, 0, INT_MAX)) == -1)
 				fail(2, "illegal weight specified");
 			break;
 		case 'x':
-- 
2.45.2

[PATCH ipvsadm 2/5] ipvsadm: fix ambiguous usage error message

[Jeremy Sowden]

If `-6` is used without `-f`, the usage error message is "-6 used before -f",
which can be misconstrued as warning that both options were used but in the
wrong order.

Change the option-parsing to allow `-6` to appear before `-f` and the error-
message in the case that `-6` was used without `-f`.

Link: http://bugs.debian.org/610596
Signed-off-by: Jeremy Sowden <azazel@debian.org>
---
 ipvsadm.c | 17 ++++++++++-------
 1 file changed, 10 insertions(+), 7 deletions(-)

diff --git a/ipvsadm.c b/ipvsadm.c
index 42f31a20e596..889128017bd1 100644
--- a/ipvsadm.c
+++ b/ipvsadm.c
@@ -523,7 +523,7 @@ static int
 parse_options(int argc, char **argv, struct ipvs_command_entry *ce,
 	      unsigned long long *options, unsigned int *format)
 {
-	int c, parse;
+	int c, parse, ipv6 = 0;
 	poptContext context;
 	char *optarg = NULL, sched_flags_arg[128];
 	struct poptOption options_table[] = {
@@ -829,12 +829,7 @@ parse_options(int argc, char **argv, struct ipvs_command_entry *ce,
 			*format |= FMT_EXACT;
 			break;
 		case '6':
-			if (ce->svc.fwmark) {
-				ce->svc.af = AF_INET6;
-				ce->svc.netmask = 128;
-			} else {
-				fail(2, "-6 used before -f\n");
-			}
+			ipv6 = 1;
 			break;
 		case 'o':
 			set_option(options, OPTC_ONEPACKET);
@@ -935,6 +930,14 @@ parse_options(int argc, char **argv, struct ipvs_command_entry *ce,
 		return -1;
 	}
 
+	if (ipv6) {
+		if (ce->svc.fwmark) {
+			ce->svc.af = AF_INET6;
+			ce->svc.netmask = 128;
+		} else
+			fail(2, "-6 used without -f\n");
+	}
+
 	if (ce->cmd == CMD_TIMEOUT) {
 		char *optarg1, *optarg2;
 
-- 
2.45.2

[PATCH ipvsadm 3/5] Use variable for pkg-config in Makefiles

[Jeremy Sowden]

Replace hard-coded `pkg-config` with a variable to allow it to be
overridden, which is helpful for cross-building.

Link: https://bugs.debian.org/901275
Signed-off-by: Jeremy Sowden <azazel@debian.org>
---
 Makefile         |  9 +++++----
 libipvs/Makefile | 17 +++++++++--------
 2 files changed, 14 insertions(+), 12 deletions(-)

diff --git a/Makefile b/Makefile
index 91a2991f6746..d247d4075160 100644
--- a/Makefile
+++ b/Makefile
@@ -36,6 +36,7 @@ RPMSOURCEDIR	= $(shell rpm --eval '%_sourcedir')
 RPMSPECDIR	= $(shell rpm --eval '%_specdir')
 
 CC		= gcc
+PKG_CONFIG	?= pkg-config
 INCLUDE		=
 SBIN		= $(BUILD_ROOT)/sbin
 MANDIR		= usr/man
@@ -66,10 +67,10 @@ OBJS		= ipvsadm.o config_stream.o dynamic_array.o
 LIBS		= -lpopt
 ifneq (0,$(HAVE_NL))
 LIBS		+= $(shell \
-		if which pkg-config > /dev/null 2>&1; then \
-		  if   pkg-config --libs libnl-genl-3.0  2> /dev/null; then :;\
-		  elif pkg-config --libs libnl-2.0       2> /dev/null; then :;\
-		  elif pkg-config --libs libnl-1         2> /dev/null; then :;\
+		if which $(PKG_CONFIG) > /dev/null 2>&1; then \
+		  if   $(PKG_CONFIG) --libs libnl-genl-3.0  2> /dev/null; then :;\
+		  elif $(PKG_CONFIG) --libs libnl-2.0       2> /dev/null; then :;\
+		  elif $(PKG_CONFIG) --libs libnl-1         2> /dev/null; then :;\
 		  fi; \
 		else echo "-lnl"; fi)
 endif
diff --git a/libipvs/Makefile b/libipvs/Makefile
index f845c8b1675b..b31c8eac514d 100644
--- a/libipvs/Makefile
+++ b/libipvs/Makefile
@@ -1,14 +1,15 @@
 # Makefile for libipvs
 
 CC		= gcc
+PKG_CONFIG	?= pkg-config
 CFLAGS		= -Wall -Wunused -Wstrict-prototypes -g -fPIC
 ifneq (0,$(HAVE_NL))
 CFLAGS		+= -DLIBIPVS_USE_NL
 CFLAGS		+= $(shell \
-		if which pkg-config > /dev/null 2>&1; then \
-		  if   pkg-config --cflags libnl-3.0  2> /dev/null; then :; \
-		  elif pkg-config --cflags libnl-2.0  2> /dev/null; then :; \
-		  elif pkg-config --cflags libnl-1    2> /dev/null; then :; \
+		if which $(PKG_CONFIG) > /dev/null 2>&1; then \
+		  if   $(PKG_CONFIG) --cflags libnl-3.0  2> /dev/null; then :; \
+		  elif $(PKG_CONFIG) --cflags libnl-2.0  2> /dev/null; then :; \
+		  elif $(PKG_CONFIG) --cflags libnl-1    2> /dev/null; then :; \
 		  fi; \
 		fi)
 endif
@@ -17,10 +18,10 @@ INCLUDE		+= $(shell if [ -f ../../ip_vs.h ]; then	\
 		     echo "-I../../."; fi;)
 DEFINES		= $(shell if [ ! -f ../../ip_vs.h ]; then	\
 		    echo "-DHAVE_NET_IP_VS_H"; fi;)
-DEFINES		+= $(shell if which pkg-config > /dev/null 2>&1; then \
-			 if   pkg-config --exists libnl-3.0; then :; \
-			 elif pkg-config --exists libnl-2.0; then :; \
-			 elif pkg-config --exists libnl-1; \
+DEFINES		+= $(shell if which $(PKG_CONFIG) > /dev/null 2>&1; then \
+			 if   $(PKG_CONFIG) --exists libnl-3.0; then :; \
+			 elif $(PKG_CONFIG) --exists libnl-2.0; then :; \
+			 elif $(PKG_CONFIG) --exists libnl-1; \
 			 then echo "-DFALLBACK_LIBNL1"; fi; fi)
 
 .PHONY		= all clean install dist distclean rpm rpms
-- 
2.45.2

[PATCH ipvsadm 4/5] Support environmental and command-line `*FLAGS` variable in Makefiles

[Jeremy Sowden]

The Makefiles don't use `CPPFLAGS` or `LDFLAGS`, and set `CFLAGS`
unconditionally.  Rename `CFLAGS`, and add `CPPFLAGS` and `LDFLAGS` to the
compilation and linking recipes in order to support the common patterns of
providing these flags via the command-line and the environment.

Signed-off-by: Jeremy Sowden <azazel@debian.org>
---
 Makefile         |  8 ++++----
 libipvs/Makefile | 12 ++++++------
 2 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/Makefile b/Makefile
index d247d4075160..d79f72496000 100644
--- a/Makefile
+++ b/Makefile
@@ -47,9 +47,9 @@ INSTALL		= install
 STATIC_LIBS	= libipvs/libipvs.a
 
 ifeq "${ARCH}" "sparc64"
-    CFLAGS = -Wall -Wunused -Wstrict-prototypes -g -m64 -pipe -mcpu=ultrasparc -mcmodel=medlow
+    DEFAULT_CFLAGS = -Wall -Wunused -Wstrict-prototypes -g -m64 -pipe -mcpu=ultrasparc -mcmodel=medlow
 else
-    CFLAGS = -Wall -Wunused -Wstrict-prototypes -g
+    DEFAULT_CFLAGS = -Wall -Wunused -Wstrict-prototypes -g
 endif
 
 
@@ -88,7 +88,7 @@ libs:
 		make -C libipvs
 
 ipvsadm:	$(OBJS) $(STATIC_LIBS)
-		$(CC) $(CFLAGS) -o $@ $^ $(LIBS)
+		$(CC) $(DEFAULT_CFLAGS) $(CFLAGS) $(LDFLAGS) -o $@ $^ $(LIBS)
 
 install:        all
 		if [ ! -d $(SBIN) ]; then $(MKDIR) -p $(SBIN); fi
@@ -140,4 +140,4 @@ debs:
 		dpkg-buildpackage
 
 %.o:	%.c
-		$(CC) $(CFLAGS) $(INCLUDE) $(DEFINES) -c -o $@ $<
+		$(CC) $(DEFAULT_CFLAGS) $(CFLAGS) $(INCLUDE) $(DEFINES) $(CPPFLAGS) -c -o $@ $<
diff --git a/libipvs/Makefile b/libipvs/Makefile
index b31c8eac514d..f29671178422 100644
--- a/libipvs/Makefile
+++ b/libipvs/Makefile
@@ -2,10 +2,10 @@
 
 CC		= gcc
 PKG_CONFIG	?= pkg-config
-CFLAGS		= -Wall -Wunused -Wstrict-prototypes -g -fPIC
+DEFAULT_CFLAGS	= -Wall -Wunused -Wstrict-prototypes -g -fPIC
 ifneq (0,$(HAVE_NL))
-CFLAGS		+= -DLIBIPVS_USE_NL
-CFLAGS		+= $(shell \
+DEFINES		+= -DLIBIPVS_USE_NL
+DEFAULT_CFLAGS	+= $(shell \
 		if which $(PKG_CONFIG) > /dev/null 2>&1; then \
 		  if   $(PKG_CONFIG) --cflags libnl-3.0  2> /dev/null; then :; \
 		  elif $(PKG_CONFIG) --cflags libnl-2.0  2> /dev/null; then :; \
@@ -16,7 +16,7 @@ endif
 
 INCLUDE		+= $(shell if [ -f ../../ip_vs.h ]; then	\
 		     echo "-I../../."; fi;)
-DEFINES		= $(shell if [ ! -f ../../ip_vs.h ]; then	\
+DEFINES		+= $(shell if [ ! -f ../../ip_vs.h ]; then	\
 		    echo "-DHAVE_NET_IP_VS_H"; fi;)
 DEFINES		+= $(shell if which $(PKG_CONFIG) > /dev/null 2>&1; then \
 			 if   $(PKG_CONFIG) --exists libnl-3.0; then :; \
@@ -34,10 +34,10 @@ $(STATIC_LIB):	libipvs.o ip_vs_nl_policy.o
 		ar rv $@ $^
 
 $(SHARED_LIB):	libipvs.o ip_vs_nl_policy.o
-		$(CC) -shared -Wl,-soname,$@ -o $@ $^
+		$(CC) $(DEFAULT_CFLAGS) $(CFLAGS) -shared -Wl,-soname,$@ $(LDFLAGS) -o $@ $^
 
 %.o:		%.c
-		$(CC) $(CFLAGS) $(INCLUDE) $(DEFINES) -c -o $@ $<
+		$(CC) $(DEFAULT_CFLAGS) $(CFLAGS) $(INCLUDE) $(DEFINES) $(CPPFLAGS) -c -o $@ $<
 
 clean:
 		rm -f *.[ao] *~ *.orig *.rej core *.so
-- 
2.45.2

[PATCH ipvsadm 5/5] Make sure libipvs.a is built before ipvsadm

[Jeremy Sowden]

There is no explicit rule in the top-level Makefile to build libipvs.a.  It is
built by the phony target `libs`.  However, there is no guarantee of the order
in which the prerequisites of the `all` target are built, so make may attempt to
link ipvsadm to libipvs.a before it has finished building libipvs.a.

Add a rule to express the dependency of `$(STATIC_LIBS)` on `libs`.

Signed-off-by: Jeremy Sowden <azazel@debian.org>
---
 Makefile | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/Makefile b/Makefile
index d79f72496000..2dda19072365 100644
--- a/Makefile
+++ b/Makefile
@@ -90,6 +90,8 @@ libs:
 ipvsadm:	$(OBJS) $(STATIC_LIBS)
 		$(CC) $(DEFAULT_CFLAGS) $(CFLAGS) $(LDFLAGS) -o $@ $^ $(LIBS)
 
+$(STATIC_LIBS): libs
+
 install:        all
 		if [ ! -d $(SBIN) ]; then $(MKDIR) -p $(SBIN); fi
 		$(INSTALL) -m 0755 ipvsadm $(SBIN)
-- 
2.45.2

Re: [PATCH ipvsadm 2/5] ipvsadm: fix ambiguous usage error message

[Julian Anastasov]

	Hello,

On Sun, 12 Jan 2025, Jeremy Sowden wrote:

> If `-6` is used without `-f`, the usage error message is "-6 used before -f",
> which can be misconstrued as warning that both options were used but in the
> wrong order.
> 
> Change the option-parsing to allow `-6` to appear before `-f` and the error-
> message in the case that `-6` was used without `-f`.
> 
> Link: http://bugs.debian.org/610596
> Signed-off-by: Jeremy Sowden <azazel@debian.org>
> ---
>  ipvsadm.c | 17 ++++++++++-------
>  1 file changed, 10 insertions(+), 7 deletions(-)
> 
> diff --git a/ipvsadm.c b/ipvsadm.c
> index 42f31a20e596..889128017bd1 100644
> --- a/ipvsadm.c
> +++ b/ipvsadm.c
> @@ -523,7 +523,7 @@ static int
>  parse_options(int argc, char **argv, struct ipvs_command_entry *ce,
>  	      unsigned long long *options, unsigned int *format)
>  {
> -	int c, parse;
> +	int c, parse, ipv6 = 0;
>  	poptContext context;
>  	char *optarg = NULL, sched_flags_arg[128];
>  	struct poptOption options_table[] = {
> @@ -829,12 +829,7 @@ parse_options(int argc, char **argv, struct ipvs_command_entry *ce,
>  			*format |= FMT_EXACT;
>  			break;
>  		case '6':
> -			if (ce->svc.fwmark) {
> -				ce->svc.af = AF_INET6;
> -				ce->svc.netmask = 128;
> -			} else {
> -				fail(2, "-6 used before -f\n");
> -			}
> +			ipv6 = 1;
>  			break;
>  		case 'o':
>  			set_option(options, OPTC_ONEPACKET);
> @@ -935,6 +930,14 @@ parse_options(int argc, char **argv, struct ipvs_command_entry *ce,
>  		return -1;
>  	}
>  
> +	if (ipv6) {
> +		if (ce->svc.fwmark) {
> +			ce->svc.af = AF_INET6;

	As ce->svc.af is set later after all options are processed,
the -M option will always see AF_INET in ce->svc.af ...

> +			ce->svc.netmask = 128;

	Now we override the value from -M, so we can not do
this here.

> +		} else
> +			fail(2, "-6 used without -f\n");
> +	}
> +
>  	if (ce->cmd == CMD_TIMEOUT) {
>  		char *optarg1, *optarg2;
>  
> -- 
> 2.45.2

Regards

--
Julian Anastasov <ja@ssi.bg>

Re: [PATCH ipvsadm 2/5] ipvsadm: fix ambiguous usage error message

[Jeremy Sowden]

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

On 2025-01-12, at 23:18:43 +0200, Julian Anastasov wrote:
> On Sun, 12 Jan 2025, Jeremy Sowden wrote:
> > If `-6` is used without `-f`, the usage error message is "-6 used before -f",
> > which can be misconstrued as warning that both options were used but in the
> > wrong order.
> > 
> > Change the option-parsing to allow `-6` to appear before `-f` and the error-
> > message in the case that `-6` was used without `-f`.
> > 
> > Link: http://bugs.debian.org/610596
> > Signed-off-by: Jeremy Sowden <azazel@debian.org>
> > ---
> >  ipvsadm.c | 17 ++++++++++-------
> >  1 file changed, 10 insertions(+), 7 deletions(-)
> > 
> > diff --git a/ipvsadm.c b/ipvsadm.c
> > index 42f31a20e596..889128017bd1 100644
> > --- a/ipvsadm.c
> > +++ b/ipvsadm.c
> > @@ -523,7 +523,7 @@ static int
> >  parse_options(int argc, char **argv, struct ipvs_command_entry *ce,
> >  	      unsigned long long *options, unsigned int *format)
> >  {
> > -	int c, parse;
> > +	int c, parse, ipv6 = 0;
> >  	poptContext context;
> >  	char *optarg = NULL, sched_flags_arg[128];
> >  	struct poptOption options_table[] = {
> > @@ -829,12 +829,7 @@ parse_options(int argc, char **argv, struct ipvs_command_entry *ce,
> >  			*format |= FMT_EXACT;
> >  			break;
> >  		case '6':
> > -			if (ce->svc.fwmark) {
> > -				ce->svc.af = AF_INET6;
> > -				ce->svc.netmask = 128;
> > -			} else {
> > -				fail(2, "-6 used before -f\n");
> > -			}
> > +			ipv6 = 1;
> >  			break;
> >  		case 'o':
> >  			set_option(options, OPTC_ONEPACKET);
> > @@ -935,6 +930,14 @@ parse_options(int argc, char **argv, struct ipvs_command_entry *ce,
> >  		return -1;
> >  	}
> >  
> > +	if (ipv6) {
> > +		if (ce->svc.fwmark) {
> > +			ce->svc.af = AF_INET6;
> 
> 	As ce->svc.af is set later after all options are processed,
> the -M option will always see AF_INET in ce->svc.af ...
> 
> > +			ce->svc.netmask = 128;
> 
> 	Now we override the value from -M, so we can not do
> this here.

Ah, right.  Thanks.  The simplest thing, then, would be just to reword
the error message to something like:

	fail(2, "-6 must follow -f\n");

Will respin.

J.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 931 bytes --]