Add an option to assign server/client port in udhcp[cd] ?

Denys Vlasenko vda.linux at googlemail.com
Thu Nov 15 04:05:15 UTC 2007


On Wednesday 14 November 2007 09:38, Jazz Yao-Tsung Wang wrote:
> Hi,
>
> Here is the patch for busybox-1.8.1 which introduce new option -P for
> udhcp[c/d].
> We had tested it with configuration files in udhcp_configs.tar.gz.


Unfortunately the patch needs a lot of polishing.

For one, please note that bbox is trying to stay small.
You have lots of code duplication. How many times do you
call atoi() on port arguments?


diff -Naur busybox-1.8.1.org/applets/applets.c busybox-1.8.1/applets/applets.c
--- busybox-1.8.1.org/applets/applets.c	2007-11-10 09:40:53.000000000 +0800
+++ busybox-1.8.1/applets/applets.c	2007-11-11 00:08:06.000000000 +0800
@@ -18,7 +18,6 @@
 #warning Note that glibc is unsuitable for static linking anyway.
 #warning If you still want to do it, remove -Wl,--gc-sections
 #warning from top-level Makefile and remove this warning.
-#error Aborting compilation.


??


 #define udhcpc_trivial_usage \
        "[-Cfbnqtv] [-c CID] [-V VCLS] [-H HOSTNAME] [-i INTERFACE]\n" \
-       "	[-p pidfile] [-r IP] [-s script]"
+       "	[-p pidfile] [-r IP] [-s script] [-P CLIENT_PORT]"
 #define udhcpc_full_usage \
 	USE_GETOPT_LONG( \
        "	-V,--vendorclass=CLASSID	Set vendor class identifier" \
@@ -3842,6 +3842,7 @@
        "\n	-q,--quit	Quit after obtaining lease" \
        "\n	-R,--release	Release IP on quit" \
        "\n	-v,--version	Display version" \
+       "\n     -P,--client-port CLIENT_PORT    Use CLIENT_PORT as DHCP Client port (default: 68)" \

Shorter text? "-P,--client-port N   Use port N instead of default 68".

 	) \
 	SKIP_GETOPT_LONG( \
        "	-V CLASSID	Set vendor class identifier" \

The SKIP_GETOPT_LONG case also needs "-P N" help text added.



 int send_discover(uint32_t xid, uint32_t requested)
 {
 	struct dhcpMessage packet;
+	int    SRV_PORT, CLT_PORT;

ALL_CAPS are for constants and macros.

+	
+	SRV_PORT=(client_port == NULL)?SERVER_PORT:(atoi(client_port) - 1);
+	CLT_PORT=(client_port == NULL)?CLIENT_PORT:atoi(client_port);

Don't torture yourself, propagate already-converted port numbers.
Also validate user input - is "-345die" valid port num?


-	return udhcp_raw_packet(&packet, INADDR_ANY, CLIENT_PORT, INADDR_BROADCAST,
-			SERVER_PORT, MAC_BCAST_ADDR, client_config.ifindex);
+	return udhcp_raw_packet(&packet, INADDR_ANY, CLT_PORT, INADDR_BROADCAST, SRV_PORT, MAC_BCAST_ADDR, client_config.ifindex);

It was wrapped because sometimes you need to read code on 80x24 screen.
 
 
 #include <getopt.h>
 #include <syslog.h>
+#include <stdlib.h>    // for atoi()

It is already included from libbb.h.

+	if (opt & OPT_P) {
+		printf("Use %s as DHCP Client Port!\n",client_port);
+	}

Do you mean "Using port %d\n"? Why do you need this message at all?

-			if (listen_mode == LISTEN_KERNEL)
-				sockfd = listen_socket(/*INADDR_ANY,*/ CLIENT_PORT, client_config.interface);
-			else
+			if (listen_mode == LISTEN_KERNEL) {
+			        if (client_port == NULL) {
+			                sockfd = listen_socket(/*INADDR_ANY,*/ CLIENT_PORT, client_config.interface);
+			        } else {
+			                sockfd = listen_socket(/*INADDR_ANY,*/ atoi(client_port), client_config.interface);
+			        }
+			} else {

again atoi()


+	opt = getopt32(argv, "fSP:",&server_port);
+	if ( server_port == NULL)
+	{
+	        printf("Use %d for DHCP Server Port!\n",SERVER_PORT);
+	}
+			if ( server_port == NULL)

Style doesn't match busybox's one. Should be "a, b", "if (expr) {..." etc


+char *server_port = NULL;
+char *client_port = NULL;

These should be added to server_config/client_config structs instead
(and be integers, not strings).


+	int      SRV_PORT=0, CLT_PORT=0;
+	
+	SRV_PORT=(server_port == NULL)?SERVER_PORT:atoi(server_port);
+	CLT_PORT=(server_port == NULL)?CLIENT_PORT:(atoi(server_port) + 1);

Useless =0 init, atoi() again. Whydoyouwriteitallwithoutspaces?

--
vda


More information about the busybox mailing list