[ipxe-devel] [PATCH 2/7] dhcpv6: Fallback to using DUID-LL for empty UUID

[Hannes Reinecke]

On 04/24/2015 07:05 PM, Michael Brown wrote:
> On 01/04/15 08:26, Hannes Reinecke wrote:
>> --- a/src/include/ipxe/dhcpv6.h
>> +++ b/src/include/ipxe/dhcpv6.h
>> @@ -11,6 +11,7 @@ FILE_LICENCE ( GPL2_OR_LATER_OR_UBDL );
>>
>>   #include <stdint.h>
>>   #include <ipxe/in.h>
>> +#include <ipxe/if_ether.h>
>>   #include <ipxe/uuid.h>
>>
>>   /** DHCPv6 server port */
>> @@ -32,6 +33,16 @@ struct dhcpv6_option {
>>       uint8_t data[0];
>>   } __attribute__ (( packed ));
>>
>> +/** DHCP unique identifier based on UUID (DUID-LL) */
>> +struct dhcpv6_duid_ll {
>> +    /** Type */
>> +    uint16_t type;
>> +    /** Hardware type */
>> +    uint16_t hw_type;
>> +    /** IPv6 link-layer address */
>> +    uint8_t ll_addr[ETH_ALEN];
>> +} __attribute__ (( packed ));
>> +
> 
> Does DUID-LL support non-Ethernet link-layer addresses?
> 
As per RFC 3315: yes.
The ll_addr is actually of variable length, the actual size being
determined by the value of the hw_type (which is required to be
a valid hardware type assigned by IANA).

I'll be renaming it to 'dhcpv6_duid_eth_ll' to clear up
ambiguity and indicate it should be used for Ethernet
link-layer addresses only.

>>       /** Client DUID */
>> -    struct dhcpv6_duid_uuid client_duid;
>> +    union {
>> +        struct dhcpv6_duid_uuid uuid;
>> +        struct dhcpv6_duid_ll ll;
>> +        uint8_t raw[24];
>> +    } client_duid;
> 
> Where does the magic 24 come from?  (Do we need raw[] at all?)
> 
Hmm. Probably we can do away with 'raw'.

>> +    int client_duid_len;
> 
> size_t?
> 
Okay.

>> @@ -935,15 +941,26 @@ int start_dhcpv6 ( struct interface *job,
>> struct net_device *netdev,
>>       addresses.server.sin6.sin6_port = htons ( DHCPV6_SERVER_PORT );
>>
>>       /* Construct client DUID from system UUID */
>> -    dhcpv6->client_duid.type = htons ( DHCPV6_DUID_UUID );
>> +    dhcpv6->client_duid_len = 18;
> 
> Where does the magic 18 come from?
> 
Ah, right. Should be sizeof(struct dhcpv6_duid_uuid).

>> +    dhcpv6->client_duid.uuid.type = htons ( DHCPV6_DUID_UUID );
>>       if ( ( len = fetch_uuid_setting ( NULL, &uuid_setting,
>> -                      &dhcpv6->client_duid.uuid ) ) < 0 ) {
>> +                      &dhcpv6->client_duid.uuid.uuid ) ) <= 0 ) {
>>           rc = len;
>>           DBGC ( dhcpv6, "DHCPv6 %s could not create DUID-UUID:
>> %s\n",
>>                  dhcpv6->netdev->name, strerror ( rc ) );
>>           goto err_client_duid;
>>       }
>> -
>> +    memset(&null_uuid, 0, sizeof(null_uuid));
> 
> You can save code size by having a "static const uuid null_uuid;" -
> it will end up in .bss and so will take no space in the final binary
> (at the cost of 16 bytes of runtime memory, which is cheaper).
> 
> (It might even be worth making it a global in core/uuid.c, as is
> done with e.g. eth_broadcast.)
> 
Okay.

>> +    if (!memcmp( &dhcpv6->client_duid.uuid.uuid, &null_uuid,
>> +             sizeof(null_uuid) )) {
>> +        DBGC ( dhcpv6, "DHCPv6 %s empty DUID-UUID, using DUID-LL\n",
>> +               dhcpv6->netdev->name );
>> +        dhcpv6->client_duid.ll.type = htons ( DHCPV6_DUID_LL );
>> +        dhcpv6->client_duid.ll.hw_type = ll_protocol->ll_proto &
>> 0xFFFF;
> 
> ll_proto is already a uint16_t.
> 
Okay, will be fixing it up.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		               zSeries & Storage
hare at suse.de			               +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)