On 11/10/2023 20:08, Eduard Zingerman wrote:
> On Wed, 2023-10-11 at 17:41 +0100, Alan Maguire wrote:
> [...]
>>>> + }
>>>> + }
>>>> +
>>>> + for (i = 0; i < ARRAY_SIZE(btf_features); i++) {
>>>> + bool *bval = (bool *)(((void *)conf_load) + btf_features[i].conf_load_offset);
>>>> + bool match = encode_all;
>>>> +
>>>> + if (!match) {
>>>> + for (j = 0; j < n; j++) {
>>>> + if (strcmp(feature_list[j], btf_features[i].name) == 0) {
>>>> + match = true;
>>>> + break;
>>>> + }
>>>> + }
>>>> + }
>>>> + if (match)
>>>> + *bval = btf_features[i].skip ? false : true;
>>>
>>> I'm not sure I understand the logic behind "skip" features.
>>> Take `decl_tag` for example:
>>> - by default conf_load->skip_encoding_btf_decl_tag is 0;
>>> - if `--btf_features=decl_tag` is passed it is still 0 because of the
>>> `skip ? false : true` logic.
>>>
>>> If there is no way to change "skip" features why listing these at all?
>>>
>> You're right; in the case of a skip feature, I think we need the
>> following behaviour
>>
>> 1. we skip the encoding by default (so the equivalent of
>> --skip_encoding_btf_decl_tag, setting skip_encoding_btf_decl_tag
>> to true
>>
>> 2. if the user however specifies the logical inversion of the skip
>> feature in --btf_features (in this case "decl_tag" - or "all")
>> skip_encoding_btf_decl_tag is set to false.
>>
>> So in my code we had 2 above but not 1. If both were in place I think
>> we'd have the right set of behaviours. Does that sound right?
>
> You mean when --features=? is specified we default to
> conf_load->skip_encoding_btf_decl_tag = true, and set it to false only
> if "all" or "decl_tag" is listed in features, right?
>
Yep. Here's the comment I was thinking of adding for the next version,
hopefully it clarifies this all a bit more than the original.
+/* --btf_features=feature1[,feature2,..] allows us to specify
+ * a list of requested BTF features or "all" to enable all features.
+ * These are translated into the appropriate conf_load values via
+ * struct btf_feature which specifies the associated conf_load
+ * boolean field and whether its default (representing the feature being
+ * off) is false or true.
+ *
+ * btf_features is for opting _into_ features so for a case like
+ * conf_load->btf_gen_floats, the translation is simple; the presence
+ * of the "float" feature in --btf_features sets conf_load->btf_gen_floats
+ * to true.
+ *
+ * The more confusing case is for features that are enabled unless
+ * skipping them is specified; for example
+ * conf_load->skip_encoding_btf_type_tag. By default - to support
+ * the opt-in model of only enabling features the user asks for -
+ * conf_load->skip_encoding_btf_type_tag is set to true (meaning no
+ * type_tags) and it is only set to false if --btf_features contains
+ * the "type_tag" feature.
+ *
+ * So from the user perspective, all features specified via
+ * --btf_features are enabled, and if a feature is not specified,
+ * it is disabled.
*/
>> Maybe a better way to express all this would be to rename the "skip"
>> field in "struct btf_feature" to "default" - so in the case of a "skip"
>> feature, the default is true, but for opt-in features, the default is false.
>
> Yes, I agree, "default" is better as "skip" is a bit confusing.
>
Thanks; I'll use that next time.
Alan
> [...]
