Apologies for the delay.

On Wed, Apr 20, 2022 at 8:35 PM Sinan Kaya <ok...@kernel.org> wrote:
>  From b906997217b363c459fdbd2824bfe6c5ac69607e Mon Sep 17 00:00:00 2001
> From: Sinan Kaya <ok...@kernel.org>
> Date: Tue, 19 Apr 2022 13:47:19 +0000
> Subject: [PATCH] udhcpc: add support for sending DHCPINFORM requests
>
> It is useful for applications to be able to query DHCP options
> without renewing IP address.

I would like a more extended information.

Imagine someone who never needed to do this before.
Explain to them what would happen when udhcpc
is run with -I option. How would DHCP handshake change?

Give a real-world example
(say, one based on your situation which prompted you
to make this patch).

Give an example where -e IP is useful.

Maybe give references to RFCs which document
DHCPINFORM usage.

>   /* Broadcast a DHCP request message */
>   /* RFC 2131 3.1 paragraph 3:
> - * "The client _broadcasts_ a DHCPREQUEST message..."
> + * "The client _broadcasts_ a DHCPREQUEST/INFORM message..."

Incorrect. That's not what RFC 2131 3.1 paragraph 3 says.

>    */
>   /* NOINLINE: limit stack usage in caller */
> -static NOINLINE int send_select(uint32_t server, uint32_t requested)
> +static NOINLINE int send_select(uint32_t server, uint32_t requested,
> int inform)
>   {
>         struct dhcp_packet packet;
>         struct in_addr temp_addr;
>         char server_str[sizeof("255.255.255.255")];
> +       const char *direction;
>
>   /*
>    * RFC 2131 4.3.2 DHCPREQUEST message
> @@ -766,11 +771,12 @@ static NOINLINE int send_select(uint32_t server,
> uint32_t requested)
>         /* Fill in: op, htype, hlen, cookie, chaddr fields,
>          * xid field, message type option:
>          */
> -       init_packet(&packet, DHCPREQUEST);
> +       init_packet(&packet, inform ? DHCPINFORM: DHCPREQUEST);
>
>         udhcp_add_simple_option(&packet, DHCP_REQUESTED_IP, requested);
>
> -       udhcp_add_simple_option(&packet, DHCP_SERVER_ID, server);
> +       if (server)
> +               udhcp_add_simple_option(&packet, DHCP_SERVER_ID, server);

Do you mean, if server is not specified, SERVER_ID may be omitted?
Should be omitted?
Should this change be made regardless of DHCPINFORM support?

>
>         /* Add options: maxsize,
>          * "param req" option according to -O, options specified with -x
> @@ -780,11 +786,19 @@ static NOINLINE int send_select(uint32_t server,
> uint32_t requested)
>         temp_addr.s_addr = server;
>         strcpy(server_str, inet_ntoa(temp_addr));
>         temp_addr.s_addr = requested;
> -       bb_info_msg("broadcasting select for %s, server %s",
> -                       inet_ntoa(temp_addr),
> -                       server_str
> +       if (server)
> +               direction = "unicasting";
> +       else
> +               direction = "broadcasting";
> +
> +       bb_info_msg("%s select for %s, server %s",
> +               direction,
> +               inet_ntoa(temp_addr),
> +               server_str
>         );

RFC 2131 3.1 paragraph 3 says:
"The client broadcasts a DHCPREQUEST message".
Always. Even if server-id is known.


> -       return raw_bcast_from_client_data_ifindex(&packet, INADDR_ANY);
> +
> +       // return raw_bcast_from_client_data_ifindex(&packet, INADDR_ANY);
> +       return bcast_or_ucast(&packet, requested, server);
>   }
>
>   /* Unicast or broadcast a DHCP renew message */
> @@ -1161,9 +1175,9 @@ static void client_background(void)
>   //usage:# define IF_UDHCP_VERBOSE(...)
>   //usage:#endif
>   //usage:#define udhcpc_trivial_usage
> -//usage:
> "[-fbq"IF_UDHCP_VERBOSE("v")"RB]"IF_FEATURE_UDHCPC_ARPING("
> [-a[MSEC]]")" [-t N] [-T SEC] [-A SEC|-n]\n"
> +//usage:
> "[-fbq"IF_UDHCP_VERBOSE("v")"RBI]"IF_FEATURE_UDHCPC_ARPING("
> [-a[MSEC]]")" [-t N] [-T SEC] [-A SEC|-n]\n"
>   //usage:       "      [-i IFACE]"IF_FEATURE_UDHCP_PORT(" [-P PORT]")" [-s
> PROG] [-p PIDFILE]\n"
> -//usage:       "       [-oC] [-r IP] [-V VENDOR] [-F NAME] [-x OPT:VAL]...
> [-O OPT]..."
> +//usage:       "       [-oC] [-r IP] [-e IP] [-V VENDOR] [-F NAME] [-x
> OPT:VAL]... [-O OPT]..."
>   //usage:#define udhcpc_full_usage "\n"
>   //usage:     "\n      -i IFACE        Interface to use (default
> "CONFIG_UDHCPC_DEFAULT_INTERFACE")"
>   //usage:      IF_FEATURE_UDHCP_PORT(
> @@ -1172,6 +1186,7 @@ static void client_background(void)
>   //usage:     "\n      -s PROG         Run PROG at DHCP events (default
> "CONFIG_UDHCPC_DEFAULT_SCRIPT")"
>   //usage:     "\n      -p FILE         Create pidfile"
>   //usage:     "\n      -B              Request broadcast replies"
> +//usage:     "\n       -I              Request using inform"

This barely explains anything.

> +//usage:     "\n       -e IP           Request this server IP address"

This is not correct. We do not "request server IPs".
We send requests to servers (sometimes by unicasting to their IPs,
but usually via broadcast).

> +
> +       if (opt & OPT_I)
> +               use_inform = 1;

Why do you need to have use_inform variable when (opt & OPT_I)
works just as well?
_______________________________________________
busybox mailing list
busybox@busybox.net
http://lists.busybox.net/mailman/listinfo/busybox

Reply via email to