UCS/CONFIG/PARSER: Handle NULL entries in sparse arrays

[michal-shalev]

What?

Following https://github.com/openucx/ucx/pull/10172

• Changed parser.c to support sparse parsing of configuration environment variables.
• Removed unnecessary flags from UCX_EXTRA_OP_ATTR_FLAGS.
• Removed [*_LAST] = NULL null entries in configuration arrays.
• Introduced UCS_CONFIG_DECLARE_ALLOWED_VALUES and UCS_CONFIG_DEFINE_ALLOWED_VALUES macros for cleaner handling of configuration arrays and their associated allowed values.
• Added UCS_CONFIG_GET_ALLOWED_VALUES macro to retrieve allowed values consistently.
• Changed UCS_CONFIG_TYPE_ENUM, UCS_CONFIG_TYPE_UINT_ENUM, and UCS_CONFIG_TYPE_BITMAP to work seamlessly with the new macros.

Why?

• The previous implementation relied on the last entry in configuration arrays being NULL to determine the end of the array. This required unnecessary placeholder entries like [*_LAST] = NULL.
• The previous implementation of UCS_CONFIG_TYPE_ENUM, UCS_CONFIG_TYPE_UINT_ENUM, and UCS_CONFIG_TYPE_BITMAP required contiguous indices, leading to core dumps when accessing NULL values due to gaps in the array.

How?

• Introduced ucs_config_allowed_values_t to explicitly pass both the array pointer and the number of valid entries to the parser.
• Updated parser.c to use the size specified in ucs_config_allowed_values_t instead of relying on NULL-terminated arrays.
• Added the following macros:
  • UCS_CONFIG_DECLARE_ALLOWED_VALUES: For declaring allowed values arrays in headers when they are defined elsewhere.
  • UCS_CONFIG_DEFINE_ALLOWED_VALUES: For defining allowed values arrays in source files.
  • UCS_CONFIG_GET_ALLOWED_VALUES: For retrieving the allowed values object associated with an array.
• Modified UCS_CONFIG_TYPE_ENUM, UCS_CONFIG_TYPE_UINT_ENUM, and UCS_CONFIG_TYPE_BITMAP to leverage the new macros and handle sparse arrays properly.

[michal-shalev]

is it possible to fix UCS_CONFIG_TYPE_BITMAP for sparse, instead of introducing new UCS_CONFIG_TYPE_FLAGS?

@tvegas1 The alternative is to change UCS_CONFIG_TYPE_BITMAP instances unrelated to the flags, but it is not critical for me.

[yosefe]

is it possible to fix UCS_CONFIG_TYPE_BITMAP for sparse, instead of introducing new UCS_CONFIG_TYPE_FLAGS?

We could do it nicely by adding another "unsigned long" field to ucs_config_parser_t to hold the count, something like

typedef struct ucs_config_parser {
    ...
    void                     (*help)(char *buf, size_t max, const void *arg);
    void                     (*doc)(ucs_string_buffer_t *strb, const void *arg);
    const void               *arg_ptr;
    unsigned long            arg_ulong;
};

And setting arg_ulong by ucs_static_array_size

[michal-shalev]

is it possible to fix UCS_CONFIG_TYPE_BITMAP for sparse, instead of introducing new UCS_CONFIG_TYPE_FLAGS?

We could do it nicely by adding another "unsigned long" field to ucs_config_parser_t to hold the count, something like

typedef struct ucs_config_parser {
    ...
    void                     (*help)(char *buf, size_t max, const void *arg);
    void                     (*doc)(ucs_string_buffer_t *strb, const void *arg);
    const void               *arg_ptr;
    unsigned long            arg_ulong;
};

And setting arg_ulong by ucs_static_array_size

We could but it will require changes in all the parsers, which is more than the scope of the flags, LMK WDYT @yosefe

[yosefe]

is it possible to fix UCS_CONFIG_TYPE_BITMAP for sparse, instead of introducing new UCS_CONFIG_TYPE_FLAGS?

We could do it nicely by adding another "unsigned long" field to ucs_config_parser_t to hold the count, something like

typedef struct ucs_config_parser {
    ...
    void                     (*help)(char *buf, size_t max, const void *arg);
    void                     (*doc)(ucs_string_buffer_t *strb, const void *arg);
    const void               *arg_ptr;
    unsigned long            arg_ulong;
};

And setting arg_ulong by ucs_static_array_size

We could but it will require changes in all the parsers, which is more than the scope of the flags, LMK WDYT @yosefe

I think it's worth it , but would appreciate @gleon99 @brminich @tvegas1 opinions

[tvegas1]

We could but it will require changes in all the parsers, which is more than the scope of the flags, LMK WDYT @yosefe

I think it's worth it , but would appreciate @gleon99 @brminich @tvegas1 opinions

I would try to have only one UCS_CONFIG_TYPE_BITMAP that supports holes because _TYPE_FLAGS seems to be more or less the same, if that's possible.

[michal-shalev]

@gleon99 @iyastreb After a discussion with @yosefe, he requested to move forward with this code by replacing the macro-based approach with a new data structure that maps numbers to strings. Since most arrays in the config parser are based on enums, this structure should also store the size, addressing both the sparse array issue and cases where different parts of the code need to retrieve a string based on an enum offset. However, since this task is low priority, it will take some time before I get back to it. Moving this back to draft for now.