[systemd-devel] [PATCH 01/28] dhcp: Add DHCP protocol structures and initial defines

[Marcel Holtmann]

Hi Lennart,

>>>>>>> +#define BOOTREQUEST                             1
>>>>>>> +#define BOOTREPLY                               2
>>>>>>> +
>>>>>>> +#define DHCP_DISCOVER                           1
>>>>>>> +#define DHCP_OFFER                              2
>>>>>>> +#define DHCP_REQUEST                            3
>>>>>>> +#define DHCP_DECLINE                            4
>>>>>>> +#define DHCP_ACK                                5
>>>>>>> +#define DHCP_NAK                                6
>>>>>>> +#define DHCP_RELEASE                            7
>>>>>>> +
>>>>>>> +#define DHCP_OVERLOAD_FILE                      1
>>>>>>> +#define DHCP_OVERLOAD_SNAME                     2
>>>>>>> +
>>>>>>> +#define DHCP_OPTION_PAD                         0
>>>>>>> +#define DHCP_OPTION_SUBNET_MASK                 1
>>>>>>> +#define DHCP_OPTION_ROUTER                      3
>>>>>>> +#define DHCP_OPTION_DOMAIN_NAME_SERVER          6
>>>>>>> +#define DHCP_OPTION_HOST_NAME                   12
>>>>>>> +#define DHCP_OPTION_DOMAIN_NAME                 15
>>>>>>> +#define DHCP_OPTION_NTP_SERVER                  42
>>>>>>> +#define DHCP_OPTION_REQUESTED_IP_ADDRESS        50
>>>>>>> +#define DHCP_OPTION_OVERLOAD                    52
>>>>>>> +#define DHCP_OPTION_MESSAGE_TYPE                53
>>>>>>> +#define DHCP_OPTION_PARAMETER_REQUEST_LIST      55
>>>>>>> +#define DHCP_OPTION_END                         255
>>>>>> 
>>>>>> For defines like these I'd really suggest using anonymous enums. It's a
>>>>>> good thing if the compiler knows these things, not just the
>>>>>> pre-processor...
>>>>> 
>>>>> these are wire protocol definitions. What benefit do you gain if the
>>>>> compiler knows them. You always have to handle invalid cases anyway
>>>>> since malicious servers are a reality.
>>>> 
>>>> For example, it's nicer to work with gdb if it can resolve them...
>>> You'll also get an error if you use an enum value of one kind
>>> in a call requiring an enum of different type.
>> 
>> as I said, these are wire protocol definitions. They come in as bits and bytes and not as enums.
> 
> I was suggesting an *anonymous* enum. That doesn't introduce a type or
> anything, it just maps name to integers (of any size). That's all. And
> it does so on the compiler level, rather than in the pre-processor. This
> is useful because gdb knows about the thing then.

fair enough on the gdb level. It is only helping you if the enum value is actually used. So for example in assignments and if-clauses or similar. It does not help you values from the wire that you do not process.

I never really cared about gdb here. Either we have builtin debug/trace printing in our protocol code or we use external tools like tcpdump etc. From BlueZ, oFono and ConnMan past experience, the builtin debug/trace printing has been the most useful tool when we had to debug something that is a wire protocol.

Regards

Marcel