Re: [PATCH net] tools: ynl: fix mixing ops and notifications on one socket

[Donald Hunter <donald.hunter@gmail.com>]

Jakub Kicinski <kuba@kernel.org> writes:

> The multi message support loosened the connection between the request
> and response handling, as we can now submit multiple requests before
> we start processing responses. Passing the attr set to NlMsgs decoding
> no longer makes sense (if it ever did), attr set may differ message
> by messsage. Isolate the part of decoding responsible for attr-set
> specific interpretation and call it once we identified the correct op.
>
> Without this fix performing SET operation on an ethtool socket, while
> being subscribed to notifications causes:
>
>  # File "tools/net/ynl/pyynl/lib/ynl.py", line 1096, in _op
>  # Exception|     return self._ops(ops)[0]
>  # Exception|            ~~~~~~~~~^^^^^
>  # File "tools/net/ynl/pyynl/lib/ynl.py", line 1040, in _ops
>  # Exception|     nms = NlMsgs(reply, attr_space=op.attr_set)
>  # Exception|                                    ^^^^^^^^^^^
>
> The value of op we use on line 1040 is stale, it comes form the previous
> loop. If a notification comes before a response we will update op to None
> and the next iteration thru the loop will break with the trace above.
>
> Fixes: 6fda63c45fe8 ("tools/net/ynl: fix cli.py --subscribe feature")
> Fixes: ba8be00f68f5 ("tools/net/ynl: Add multi message support to ynl")
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>

Good catch, fix LGTM. It looks like a followup refactor could combine
annotate_extack and _decode_extack and get rid of the attr_space
parameter to NlMsg(). WDYT?

Reviewed-by: Donald Hunter <donald.hunter@gmail.com>

> ---
> CC: donald.hunter@gmail.com
> CC: arkadiusz.kubalewski@intel.com
> ---
>  tools/net/ynl/pyynl/lib/ynl.py | 28 +++++++++++++++++-----------
>  1 file changed, 17 insertions(+), 11 deletions(-)
>
> diff --git a/tools/net/ynl/pyynl/lib/ynl.py b/tools/net/ynl/pyynl/lib/ynl.py
> index ae4d1ef7b83a..7529bce174ff 100644
> --- a/tools/net/ynl/pyynl/lib/ynl.py
> +++ b/tools/net/ynl/pyynl/lib/ynl.py
> @@ -231,14 +231,7 @@ from .nlspec import SpecFamily
>                      self.extack['unknown'].append(extack)
>  
>              if attr_space:
> -                # We don't have the ability to parse nests yet, so only do global
> -                if 'miss-type' in self.extack and 'miss-nest' not in self.extack:
> -                    miss_type = self.extack['miss-type']
> -                    if miss_type in attr_space.attrs_by_val:
> -                        spec = attr_space.attrs_by_val[miss_type]
> -                        self.extack['miss-type'] = spec['name']
> -                        if 'doc' in spec:
> -                            self.extack['miss-type-doc'] = spec['doc']
> +                self.annotate_extack(attr_space)
>  
>      def _decode_policy(self, raw):
>          policy = {}
> @@ -264,6 +257,18 @@ from .nlspec import SpecFamily
>                  policy['mask'] = attr.as_scalar('u64')
>          return policy
>  
> +    def annotate_extack(self, attr_space):
> +        """ Make extack more human friendly with attribute information """
> +
> +        # We don't have the ability to parse nests yet, so only do global
> +        if 'miss-type' in self.extack and 'miss-nest' not in self.extack:
> +            miss_type = self.extack['miss-type']
> +            if miss_type in attr_space.attrs_by_val:
> +                spec = attr_space.attrs_by_val[miss_type]
> +                self.extack['miss-type'] = spec['name']
> +                if 'doc' in spec:
> +                    self.extack['miss-type-doc'] = spec['doc']
> +
>      def cmd(self):
>          return self.nl_type
>  
> @@ -277,12 +282,12 @@ from .nlspec import SpecFamily
>  
>  
>  class NlMsgs:
> -    def __init__(self, data, attr_space=None):
> +    def __init__(self, data):
>          self.msgs = []
>  
>          offset = 0
>          while offset < len(data):
> -            msg = NlMsg(data, offset, attr_space=attr_space)
> +            msg = NlMsg(data, offset)
>              offset += msg.nl_len
>              self.msgs.append(msg)
>  
> @@ -1036,12 +1041,13 @@ genl_family_name_to_id = None
>          op_rsp = []
>          while not done:
>              reply = self.sock.recv(self._recv_size)
> -            nms = NlMsgs(reply, attr_space=op.attr_set)
> +            nms = NlMsgs(reply)
>              self._recv_dbg_print(reply, nms)
>              for nl_msg in nms:
>                  if nl_msg.nl_seq in reqs_by_seq:
>                      (op, vals, req_msg, req_flags) = reqs_by_seq[nl_msg.nl_seq]
>                      if nl_msg.extack:
> +                        nl_msg.annotate_extack(op.attr_set)
>                          self._decode_extack(req_msg, op, nl_msg.extack, vals)
>                  else:
>                      op = None