DHCPv4-o-DHCPv6 with udhcp client

[Denys Vlasenko]

Apologies for a very late reply.

On Monday 24 March 2014 18:10, Leonardo Jelenkovic wrote:
> In one project we are trying to use udhcp client from busybox with
> DHCPv6 as transport for DHCPv4 packets (where "upstream" is IPv6 only
> and IPv4 is tunneled over it). Draft that describe this encapsulation
> is "DHCPv4 over DHCPv6 Transport"
> (http://tools.ietf.org/html/draft-ietf-dhc-dhcpv4-over-dhcpv6-06).
> In short, DHCPv4 is placed in an option into DHCPv6 packet.
> 
> That encapsulation/decapsulation is all that we added in (for now).
> 
> It is assumed that IPv6 is previously obtained (e.g. with DHCPv6) and
> that IPv6 address of DHCPv4-o-DHCPv6 server is also known
> (both are command line parameters to udhcpc).
> 
> Tests are performed using modified version of ISC DHCP
> (as DHCPv4-o-DHCPv6 server) that does same
> encapsulation/decapsulation using DHCPv6 as transport.
> 
> Since our system uses busybox we would like that our modification
> (when mature enough) becomes part of busybox (udhcp client).
> 
> Is this idea acceptable for addition to busybox?

Yes, it is acceptable.

> If so, can you give us some comments on our implementation?

It looks reasonably good.

I propose renaming FEATURE_DHCP4o6C to FEATURE_DHCP4_OVER6
as a bit more readable.

You are adding another "static void *d6_find_option(...)" in dhcp4o6.c.
Having two static functions with the same name and similar functionality
in two different files is not a good practice.
Can you improve existing function so that it does what you need?
If not, add a comment why it is impossible or very difficult.
Same goes for a few other d6_foo functions you reimplement.


"#if 1 /* not working! */" part looks confusing. If it doesn't work,
why is it enabled?


"memset ( &dhcp4o6_data.dst_ip, 0, 16 )" is not matching code style
of the project. Please use "memset(&dhcp4o6_data.dst_ip, 0, 16)"

+               bb_error_msg("Valid local (client) IPv6 address must be provided!");
+               exit(1);
we have bb_error_msg_and_die() for this.



+#if ENABLE_FEATURE_DHCP4o6C
+       if ( !client_config.mode4o6 )
+#endif
        return udhcp_send_raw_packet(packet,
                /*src*/ INADDR_ANY, CLIENT_PORT,
                /*dst*/ INADDR_BROADCAST, SERVER_PORT, MAC_BCAST_ADDR,
                client_config.ifindex);
+#if ENABLE_FEATURE_DHCP4o6C
+       else
+               return dhcp4o6_send_packet (packet, 1);
+#endif

The above block needs to be made more readable.
For example, by defining DHCP4_OVER6 to be client_config.mode4o6
if ENABLE_FEATURE_DHCP4o6C=y, or 0 otherwise, you can avoid #if's:

        if (DHCP4_OVER6)
                return dhcp4o6_send_packet(packet, 1);
        return udhcp_send_raw_packet(packet,
                /*src*/ INADDR_ANY, CLIENT_PORT,
                /*dst*/ INADDR_BROADCAST, SERVER_PORT, MAC_BCAST_ADDR,
                client_config.ifindex);




+       IF_FEATURE_DHCP4o6C(char *str_6c, *str_6s;)
...
+               if ( !(opt & OPT_I) )
+                       str_6c = NULL;

Just set it to NULL unconditionally before getopt call.