[13/21] dpp-util: add __DPP_STATUS_MAX

Commit Message

James Prestwood Oct. 12, 2023, 8:01 p.m. UTC

Nice for setting as an invalid status since STATUS_OK is
already zero.
---
 src/dpp-util.h | 1 +
 1 file changed, 1 insertion(+)

Comments

Denis Kenzior Oct. 19, 2023, 3:16 p.m. UTC | #1

Hi James,

On 10/12/23 15:01, James Prestwood wrote:
> Nice for setting as an invalid status since STATUS_OK is
> already zero.

But this enum comes directly from the spec?

> ---
>   src/dpp-util.h | 1 +
>   1 file changed, 1 insertion(+)
> 
> diff --git a/src/dpp-util.h b/src/dpp-util.h
> index 6b00796e..61f1c859 100644
> --- a/src/dpp-util.h
> +++ b/src/dpp-util.h
> @@ -71,6 +71,7 @@ enum dpp_status {
>   	DPP_STATUS_CSR_NEEDED,
>   	DPP_STATUS_CSR_BAD,
>   	DPP_STATUS_NEW_KEY_NEEDED,
> +	__DPP_STATUS_MAX,

In general iwd/ell do not use such constructs since it makes handling of enums 
in switch/case statements a bit more painful.

>   };
>   
>   enum dpp_attribute_type {

Regards,
-Denis

James Prestwood Oct. 23, 2023, 12:35 p.m. UTC | #2

Hi Denis,

On 10/19/23 8:16 AM, Denis Kenzior wrote:
> Hi James,
> 
> On 10/12/23 15:01, James Prestwood wrote:
>> Nice for setting as an invalid status since STATUS_OK is
>> already zero.
> 
> But this enum comes directly from the spec?

Not __DPP_STATUS_MAX obviously, but yes the other status' are all part 
of the spec. Having an "invalid" enum value is just convenient since I 
can check it by value instead of having to store an additional pointer 
to check if the attribute was included:

...
case DPP_ATTR_STATUS:
     status_ptr = data;
     break;
...

if (!status_ptr)
     goto fail;

if (*status_ptr != DPP_STATUS_OK)
     goto fail;

This is how we do it elsewhere though so I can do it this way. To be 
fair, its just an extra if :)

> 
>> ---
>>   src/dpp-util.h | 1 +
>>   1 file changed, 1 insertion(+)
>>
>> diff --git a/src/dpp-util.h b/src/dpp-util.h
>> index 6b00796e..61f1c859 100644
>> --- a/src/dpp-util.h
>> +++ b/src/dpp-util.h
>> @@ -71,6 +71,7 @@ enum dpp_status {
>>       DPP_STATUS_CSR_NEEDED,
>>       DPP_STATUS_CSR_BAD,
>>       DPP_STATUS_NEW_KEY_NEEDED,
>> +    __DPP_STATUS_MAX,
> 
> In general iwd/ell do not use such constructs since it makes handling of 
> enums in switch/case statements a bit more painful.

Thats fine, I can do it like the above pseudocode.

Thanks,
James

> 
>>   };
>>   enum dpp_attribute_type {
> 
> Regards,
> -Denis

Patch

diff --git a/src/dpp-util.h b/src/dpp-util.h
index 6b00796e..61f1c859 100644
--- a/src/dpp-util.h
+++ b/src/dpp-util.h
@@ -71,6 +71,7 @@  enum dpp_status {
 	DPP_STATUS_CSR_NEEDED,
 	DPP_STATUS_CSR_BAD,
 	DPP_STATUS_NEW_KEY_NEEDED,
+	__DPP_STATUS_MAX,
 };
 
 enum dpp_attribute_type {