| Message ID | 9414bdc6d3d0212238cec12d4f2d80894910e944.1680801697.git.dcaratti@redhat.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Delegated to: | Paolo Abeni |
| Headers | show |
| Series | YAML template for MPTCP netlink API | expand |
On Thu, 2023-04-06 at 19:31 +0200, Davide Caratti wrote: > when a policy contains multiple nested attributes of the same type, avoid > re-initializing .request and .reply members. It's unclear to me why we need to avoid such re-init... > > Signed-off-by: Davide Caratti <dcaratti@redhat.com> > --- > tools/net/ynl/ynl-gen-c.py | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/tools/net/ynl/ynl-gen-c.py b/tools/net/ynl/ynl-gen-c.py > index cc2f8c945340..ba55f217a006 100755 > --- a/tools/net/ynl/ynl-gen-c.py > +++ b/tools/net/ynl/ynl-gen-c.py > @@ -812,7 +812,8 @@ class Family(SpecFamily): > inherit = set() > nested = spec['nested-attributes'] > if nested not in self.root_sets: > - self.pure_nested_structs[nested] = Struct(self, nested, inherited=inherit) > + if nested not in self.pure_nested_structs: > + self.pure_nested_structs[nested] = Struct(self, nested, inherited=inherit) ... and is unclear to me how the above produces the effect described in the commit message :) Thanks! Paolo
hi Paolo, thanks a lot for looking at this! On Fri, Apr 07, 2023 at 10:00:43AM +0200, Paolo Abeni wrote: > On Thu, 2023-04-06 at 19:31 +0200, Davide Caratti wrote: > > when a policy contains multiple nested attributes of the same type, avoid > > re-initializing .request and .reply members. > > It's unclear to me why we need to avoid such re-init... *disclaimer*: this will sound as a giant word pun, since "pm", "addr" and "attr" are used in any combination in the current netlink API :) That said: "attr" has two nested attributes, "addr" and "remote_addr", of the same type "addr". When ynl-gen-c.py parses "addr" (e.g. in the block describing the "add_addr" operation), it sets self.pure_nested_structs['addr'].Request equal to True, because at least on operation needs it. Then, when it parses "remote_addr", the call to Struct(self, nested, inherited=inherit) will reset the above variable to 'False'. As a consequence, when it comes to print the nla_policy struct, the following code will run: 2127 if args.mode == "kernel": 2128 if args.header: 2129 for _, struct in sorted(parsed.pure_nested_structs.items()): 2130 if struct.request: 2131 cw.p('/* Common nested types */') 2132 break 2133 for attr_set, struct in sorted(parsed.pure_nested_structs.items()): 2134 if struct.request: 2135 print_req_policy_fwd(cw, struct) 2136 cw.nl() since struct.request has become True and then resetted to False, ynl-gen-c.py refuses to print the nla_policy of the nested attribute. Like I mentioned in the cover letter, changing the yaml spec in a way that at least one operation requires 'remote_addr' in the attribute-set will make this issue disappear; anyway, I think that the current behavior of ynl-gen-c.py is not intentional, that's why I did this fix attempt. [...] > > diff --git a/tools/net/ynl/ynl-gen-c.py b/tools/net/ynl/ynl-gen-c.py > > index cc2f8c945340..ba55f217a006 100755 > > --- a/tools/net/ynl/ynl-gen-c.py > > +++ b/tools/net/ynl/ynl-gen-c.py > > @@ -812,7 +812,8 @@ class Family(SpecFamily): > > inherit = set() > > nested = spec['nested-attributes'] > > if nested not in self.root_sets: > > - self.pure_nested_structs[nested] = Struct(self, nested, inherited=inherit) > > + if nested not in self.pure_nested_structs: > > + self.pure_nested_structs[nested] = Struct(self, nested, inherited=inherit) > > ... and is unclear to me how the above produces the effect described in > the commit message :) this hunk avoids re-initializing self.pure_nested_structs['addr'].{request,reply} when it has already been parsed once. If at least a nested attribute sets 'request' to True, then 'request' remains True also when other nested attributes of the same type are found.
On Fri, Apr 7, 2023 at 10:00 AM Paolo Abeni <pabeni@redhat.com> wrote: > > On Thu, 2023-04-06 at 19:31 +0200, Davide Caratti wrote: > > when a policy contains multiple nested attributes of the same type, avoid > > re-initializing .request and .reply members. > > It's unclear to me why we need to avoid such re-init... > > > > > Signed-off-by: Davide Caratti <dcaratti@redhat.com> > > --- > > tools/net/ynl/ynl-gen-c.py | 3 ++- > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > diff --git a/tools/net/ynl/ynl-gen-c.py b/tools/net/ynl/ynl-gen-c.py > > index cc2f8c945340..ba55f217a006 100755 > > --- a/tools/net/ynl/ynl-gen-c.py > > +++ b/tools/net/ynl/ynl-gen-c.py > > @@ -812,7 +812,8 @@ class Family(SpecFamily): > > inherit = set() > > nested = spec['nested-attributes'] > > if nested not in self.root_sets: > > - self.pure_nested_structs[nested] = Struct(self, nested, inherited=inherit) > > + if nested not in self.pure_nested_structs: > > + self.pure_nested_structs[nested] = Struct(self, nested, inherited=inherit) > > ... and is unclear to me how the above produces the effect described in > the commit message :) FTR: I'm removing this patch from the series as it's already in the net-next tree [1]. Thanks! [1] https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.git/commit/?id=67c65ce762adaf3515fe0585
diff --git a/tools/net/ynl/ynl-gen-c.py b/tools/net/ynl/ynl-gen-c.py index cc2f8c945340..ba55f217a006 100755 --- a/tools/net/ynl/ynl-gen-c.py +++ b/tools/net/ynl/ynl-gen-c.py @@ -812,7 +812,8 @@ class Family(SpecFamily): inherit = set() nested = spec['nested-attributes'] if nested not in self.root_sets: - self.pure_nested_structs[nested] = Struct(self, nested, inherited=inherit) + if nested not in self.pure_nested_structs: + self.pure_nested_structs[nested] = Struct(self, nested, inherited=inherit) if attr in rs_members['request']: self.pure_nested_structs[nested].request = True if attr in rs_members['reply']:
when a policy contains multiple nested attributes of the same type, avoid re-initializing .request and .reply members. Signed-off-by: Davide Caratti <dcaratti@redhat.com> --- tools/net/ynl/ynl-gen-c.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)