| Message ID | 20231215035009.498049-2-liuhangbin@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | ynl-gen: update check format | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Clearly marked for net-next |
| netdev/apply | fail | Patch does not apply to net-next |
On Fri, 15 Dec 2023 11:50:07 +0800 Hangbin Liu wrote: > As the description of 'struct nla_policy', the len means the maximum length > of string, binary. Or minimum length of attribute payload for others. > But most time we only use it for string and binary. The meaning of 'len' in nla_policy is confusing to people writing new families. IIRC I used max-len / min-len / extact-len and not len on purpose in the YAML, so that there's no confusion what means what for which type... Obviously it is slightly confusing for people like you who convert the existing families to YAML specs, but the risk of bugs seems lower there. So I'd prefer to stick to the existing set of options. Is the existing code gen incorrect or just hard to wrap one's head around?
On Fri, Dec 15, 2023 at 06:06:03PM -0800, Jakub Kicinski wrote: > On Fri, 15 Dec 2023 11:50:07 +0800 Hangbin Liu wrote: > > As the description of 'struct nla_policy', the len means the maximum length > > of string, binary. Or minimum length of attribute payload for others. > > But most time we only use it for string and binary. > > The meaning of 'len' in nla_policy is confusing to people writing new > families. IIRC I used max-len / min-len / extact-len and not len on > purpose in the YAML, so that there's no confusion what means what for > which type... > > Obviously it is slightly confusing for people like you who convert > the existing families to YAML specs, but the risk of bugs seems lower > there. So I'd prefer to stick to the existing set of options. > > Is the existing code gen incorrect or just hard to wrap one's head > around? > The max-len / min-len / extact-len micro are used by binary. For string we need to use "len" to define the max length. e.g. static const struct nla_policy team_nl_option_policy[TEAM_ATTR_OPTION_MAX + 1] = { [TEAM_ATTR_OPTION_UNSPEC] = { .type = NLA_UNSPEC, }, [TEAM_ATTR_OPTION_NAME] = { .type = NLA_STRING, .len = TEAM_STRING_MAX_LEN, }, Thanks Hangbin
On Sat, 16 Dec 2023 16:35:40 +0800 Hangbin Liu wrote: > The max-len / min-len / extact-len micro are used by binary. For string we > need to use "len" to define the max length. e.g. > > static const struct nla_policy > team_nl_option_policy[TEAM_ATTR_OPTION_MAX + 1] = { > [TEAM_ATTR_OPTION_UNSPEC] = { .type = NLA_UNSPEC, }, > [TEAM_ATTR_OPTION_NAME] = { > .type = NLA_STRING, > .len = TEAM_STRING_MAX_LEN, > }, max-len / min-len / extact-len are just the names in the spec. We can put the value provided in the spec as max-len inside nla_policy as len, given that for string spec::max-len == policy::len Am I confused?
On Mon, Dec 18, 2023 at 02:22:09PM -0800, Jakub Kicinski wrote: > On Sat, 16 Dec 2023 16:35:40 +0800 Hangbin Liu wrote: > > The max-len / min-len / extact-len micro are used by binary. For string we > > need to use "len" to define the max length. e.g. > > > > static const struct nla_policy > > team_nl_option_policy[TEAM_ATTR_OPTION_MAX + 1] = { > > [TEAM_ATTR_OPTION_UNSPEC] = { .type = NLA_UNSPEC, }, > > [TEAM_ATTR_OPTION_NAME] = { > > .type = NLA_STRING, > > .len = TEAM_STRING_MAX_LEN, > > }, > > max-len / min-len / extact-len are just the names in the spec. > We can put the value provided in the spec as max-len inside > nla_policy as len, given that for string spec::max-len == policy::len > > Am I confused? Yes, we can do that. While this looks like another magic. When user set max-len for a string type in the yaml spec. After converting to c code, it's .len attribute. This still makes user confused. Anyway, this is just a matter of choosing apple or banana. I'm OK to using the current YAML spec policy. Thanks Hangbin
diff --git a/Documentation/netlink/genetlink-c.yaml b/Documentation/netlink/genetlink-c.yaml index c58f7153fcf8..66083cdbf43e 100644 --- a/Documentation/netlink/genetlink-c.yaml +++ b/Documentation/netlink/genetlink-c.yaml @@ -199,14 +199,17 @@ properties: max: description: Max value for an integer attribute. $ref: '#/$defs/len-or-limit' + len: + description: Max length for a string or a binary attribute. + $ref: '#/$defs/len-or-define' min-len: description: Min length for a binary attribute. $ref: '#/$defs/len-or-define' max-len: - description: Max length for a string or a binary attribute. + description: Max length for a binary attribute. $ref: '#/$defs/len-or-define' exact-len: - description: Exact length for a string or a binary attribute. + description: Exact length for a binary attribute. $ref: '#/$defs/len-or-define' sub-type: *attr-type display-hint: &display-hint diff --git a/Documentation/netlink/genetlink-legacy.yaml b/Documentation/netlink/genetlink-legacy.yaml index 938703088306..9a9ab7468469 100644 --- a/Documentation/netlink/genetlink-legacy.yaml +++ b/Documentation/netlink/genetlink-legacy.yaml @@ -242,14 +242,17 @@ properties: max: description: Max value for an integer attribute. $ref: '#/$defs/len-or-limit' + len: + description: Max length for a string or a binary attribute. + $ref: '#/$defs/len-or-define' min-len: description: Min length for a binary attribute. $ref: '#/$defs/len-or-define' max-len: - description: Max length for a string or a binary attribute. + description: Max length for a binary attribute. $ref: '#/$defs/len-or-define' exact-len: - description: Exact length for a string or a binary attribute. + description: Exact length for a binary attribute. $ref: '#/$defs/len-or-define' sub-type: *attr-type display-hint: *display-hint diff --git a/Documentation/netlink/genetlink.yaml b/Documentation/netlink/genetlink.yaml index 3283bf458ff1..9be071a622cf 100644 --- a/Documentation/netlink/genetlink.yaml +++ b/Documentation/netlink/genetlink.yaml @@ -166,14 +166,17 @@ properties: max: description: Max value for an integer attribute. $ref: '#/$defs/len-or-limit' + len: + description: Max length for a string or a binary attribute. + $ref: '#/$defs/len-or-define' min-len: description: Min length for a binary attribute. $ref: '#/$defs/len-or-define' max-len: - description: Max length for a string or a binary attribute. + description: Max length for a binary attribute. $ref: '#/$defs/len-or-define' exact-len: - description: Exact length for a string or a binary attribute. + description: Exact length for a binary attribute. $ref: '#/$defs/len-or-define' sub-type: *attr-type display-hint: &display-hint diff --git a/Documentation/netlink/netlink-raw.yaml b/Documentation/netlink/netlink-raw.yaml index ad5395040765..2c393b234511 100644 --- a/Documentation/netlink/netlink-raw.yaml +++ b/Documentation/netlink/netlink-raw.yaml @@ -241,14 +241,17 @@ properties: min: description: Min value for an integer attribute. type: integer + len: + description: Max length for a string or a binary attribute. + $ref: '#/$defs/len-or-define' min-len: description: Min length for a binary attribute. $ref: '#/$defs/len-or-define' max-len: - description: Max length for a string or a binary attribute. + description: Max length for a binary attribute. $ref: '#/$defs/len-or-define' exact-len: - description: Exact length for a string or a binary attribute. + description: Exact length for a binary attribute. $ref: '#/$defs/len-or-define' sub-type: *attr-type display-hint: *display-hint diff --git a/include/net/netlink.h b/include/net/netlink.h index 28039e57070a..e7f6e22282d3 100644 --- a/include/net/netlink.h +++ b/include/net/netlink.h @@ -464,6 +464,7 @@ struct nla_policy { .max = _len \ } #define NLA_POLICY_MIN_LEN(_len) NLA_POLICY_MIN(NLA_BINARY, _len) +#define NLA_POLICY_MAX_LEN(_len) NLA_POLICY_MAX(NLA_BINARY, _len) /** * struct nl_info - netlink source information diff --git a/tools/net/ynl/ynl-gen-c.py b/tools/net/ynl/ynl-gen-c.py index 7fc1aa788f6f..88a2048d668d 100755 --- a/tools/net/ynl/ynl-gen-c.py +++ b/tools/net/ynl/ynl-gen-c.py @@ -427,13 +427,10 @@ class TypeString(Type): return f'.type = YNL_PT_NUL_STR, ' def _attr_policy(self, policy): - if 'exact-len' in self.checks: - mem = 'NLA_POLICY_EXACT_LEN(' + str(self.checks['exact-len']) + ')' - else: - mem = '{ .type = ' + policy - if 'max-len' in self.checks: - mem += ', .len = ' + str(self.get_limit('max-len')) - mem += ', }' + mem = '{ .type = ' + policy + if 'len' in self.checks: + mem += ', .len = ' + str(self.get_limit('len')) + mem += ', }' return mem def attr_policy(self, cw): @@ -481,13 +478,15 @@ class TypeBinary(Type): def _attr_policy(self, policy): if 'exact-len' in self.checks: mem = 'NLA_POLICY_EXACT_LEN(' + str(self.checks['exact-len']) + ')' + elif 'max-len' in self.checks: + mem = 'NLA_POLICY_MAX_LEN(' + str(self.checks['max-len']) + ')' + elif 'min-len' in self.checks: + mem = 'NLA_POLICY_MIN_LEN(' + str(self.checks['min-len']) + ')' else: - mem = '{ ' - if len(self.checks) == 1 and 'min-len' in self.checks: - mem += '.len = ' + str(self.get_limit('min-len')) - elif len(self.checks) == 0: - mem += '.type = NLA_BINARY' - else: + mem = '{ .type = ' + policy + if len(self.checks) == 1 and 'len' in self.checks: + mem += '.len = ' + str(self.get_limit('len')) + elif len(self.checks) > 1: raise Exception('One or more of binary type checks not implemented, yet') mem += ', }' return mem
As the description of 'struct nla_policy', the len means the maximum length of string, binary. Or minimum length of attribute payload for others. But most time we only use it for string and binary. On the other hand, the NLA_POLICY_{EXACT_LEN, MIN_LEN, MAX_LEN} should only be used for binary only. Signed-off-by: Hangbin Liu <liuhangbin@gmail.com> --- Documentation/netlink/genetlink-c.yaml | 7 ++++-- Documentation/netlink/genetlink-legacy.yaml | 7 ++++-- Documentation/netlink/genetlink.yaml | 7 ++++-- Documentation/netlink/netlink-raw.yaml | 7 ++++-- include/net/netlink.h | 1 + tools/net/ynl/ynl-gen-c.py | 25 ++++++++++----------- 6 files changed, 33 insertions(+), 21 deletions(-)