| Message ID | 20240327181700.77940-3-donald.hunter@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | netlink: Add nftables spec w/ multi messages | expand |
On Wed, 27 Mar 2024 18:17:00 +0000 Donald Hunter wrote: > - parser = argparse.ArgumentParser(description='YNL CLI sample') > + description = """ > + YNL CLI utility - a general purpose netlink utility that uses YNL specs YNL specs is intentional or should have been YAML? :) > + to drive protocol encoding and decoding. > + """ > + epilog = """ > + The --multi option can be repeated to include several operations > + in the same netlink payload. > + """ > + > + parser = argparse.ArgumentParser(description=description, > + epilog=epilog) > parser.add_argument('--spec', dest='spec', type=str, required=True) > parser.add_argument('--schema', dest='schema', type=str) > parser.add_argument('--no-schema', action='store_true') > parser.add_argument('--json', dest='json_text', type=str) > - parser.add_argument('--do', dest='do', type=str) > - parser.add_argument('--dump', dest='dump', type=str) > + parser.add_argument('--do', dest='do', metavar='OPERATION', type=str) > + parser.add_argument('--dump', dest='dump', metavar='OPERATION', type=str) > parser.add_argument('--sleep', dest='sleep', type=int) > parser.add_argument('--subscribe', dest='ntf', type=str) > parser.add_argument('--replace', dest='flags', action='append_const', > @@ -40,6 +50,8 @@ def main(): > parser.add_argument('--output-json', action='store_true') > parser.add_argument('--dbg-small-recv', default=0, const=4000, > action='store', nargs='?', type=int) > + parser.add_argument('--multi', dest='multi', nargs=2, action='append', > + metavar=('OPERATION', 'JSON_TEXT'), type=str) We'd only support multiple "do" requests, I wonder if we should somehow call this out. Is --multi-do unnecessary extra typing? Code itself looks pretty good!
Jakub Kicinski <kuba@kernel.org> writes: > On Wed, 27 Mar 2024 18:17:00 +0000 Donald Hunter wrote: >> - parser = argparse.ArgumentParser(description='YNL CLI sample') >> + description = """ >> + YNL CLI utility - a general purpose netlink utility that uses YNL specs > > YNL specs is intentional or should have been YAML? :) I'm not sure it was intentional, but YAML is definitely better :-) >> + to drive protocol encoding and decoding. >> + """ >> + epilog = """ >> + The --multi option can be repeated to include several operations >> + in the same netlink payload. >> + """ >> + >> + parser = argparse.ArgumentParser(description=description, >> + epilog=epilog) >> parser.add_argument('--spec', dest='spec', type=str, required=True) >> parser.add_argument('--schema', dest='schema', type=str) >> parser.add_argument('--no-schema', action='store_true') >> parser.add_argument('--json', dest='json_text', type=str) >> - parser.add_argument('--do', dest='do', type=str) >> - parser.add_argument('--dump', dest='dump', type=str) >> + parser.add_argument('--do', dest='do', metavar='OPERATION', type=str) >> + parser.add_argument('--dump', dest='dump', metavar='OPERATION', type=str) >> parser.add_argument('--sleep', dest='sleep', type=int) >> parser.add_argument('--subscribe', dest='ntf', type=str) >> parser.add_argument('--replace', dest='flags', action='append_const', >> @@ -40,6 +50,8 @@ def main(): >> parser.add_argument('--output-json', action='store_true') >> parser.add_argument('--dbg-small-recv', default=0, const=4000, >> action='store', nargs='?', type=int) >> + parser.add_argument('--multi', dest='multi', nargs=2, action='append', >> + metavar=('OPERATION', 'JSON_TEXT'), type=str) > > We'd only support multiple "do" requests, I wonder if we should somehow > call this out. Is --multi-do unnecessary extra typing? I prefer --multi but will update the help text to say "DO-OPERATIION" and "... several do operations". > Code itself looks pretty good!
On Fri, 29 Mar 2024 13:37:31 +0000 Donald Hunter wrote: > > We'd only support multiple "do" requests, I wonder if we should somehow > > call this out. Is --multi-do unnecessary extra typing? > > I prefer --multi but will update the help text to say "DO-OPERATIION" > and "... several do operations". Alright, technically doing multi-dump should also work, but maybe there's less of a benefit there, so we can keep the multi focused on do for now. Looking at the code again, are you sure we'll process all the responses not just the first one? Shouldn't this: + del reqs_by_seq[nl_msg.nl_seq] done = True be something like: del reqs_by_seq[nl_msg.nl_seq] done = len(reqs_by_seq) == 0 ? Would be good to add an example of multi executing some get operations. My other concern is the formatting of the response. For mutli we should probably retain the indexes, e.g. 3 dos should produce an array with a length of 3, some of the entries may be None if the command only acked. Would that make sense?
Jakub Kicinski <kuba@kernel.org> writes: > On Fri, 29 Mar 2024 13:37:31 +0000 Donald Hunter wrote: >> > We'd only support multiple "do" requests, I wonder if we should somehow >> > call this out. Is --multi-do unnecessary extra typing? >> >> I prefer --multi but will update the help text to say "DO-OPERATIION" >> and "... several do operations". > > Alright, technically doing multi-dump should also work, but maybe > there's less of a benefit there, so we can keep the multi focused > on do for now. > > Looking at the code again, are you sure we'll process all the responses > not just the first one? > > Shouldn't this: > > + del reqs_by_seq[nl_msg.nl_seq] > done = True > > be something like: > > del reqs_by_seq[nl_msg.nl_seq] > done = len(reqs_by_seq) == 0 > Hmm yes, that's a good catch. I need to check the DONE semantics for these nftables batch operations. > Would be good to add an example of multi executing some get operations. I think this was a blind spot on my part because nftables doesn't support batch for get operations: https://elixir.bootlin.com/linux/latest/source/net/netfilter/nf_tables_api.c#L9092 I'll need to try using multi for gets without any batch messages and see how everything behaves. > My other concern is the formatting of the response. For mutli we should > probably retain the indexes, e.g. 3 dos should produce an array with a > length of 3, some of the entries may be None if the command only acked. > Would that make sense? As I said, a blind spot on my part - I didn't really think there was a need to do anything for None responses but if get can work then an array of responses will be needed.
On Fri, 29 Mar 2024 at 18:58, Donald Hunter <donald.hunter@gmail.com> wrote: > > Jakub Kicinski <kuba@kernel.org> writes: > > > Looking at the code again, are you sure we'll process all the responses > > not just the first one? > > > > Shouldn't this: > > > > + del reqs_by_seq[nl_msg.nl_seq] > > done = True > > > > be something like: > > > > del reqs_by_seq[nl_msg.nl_seq] > > done = len(reqs_by_seq) == 0 > > > > Hmm yes, that's a good catch. I need to check the DONE semantics for > these nftables batch operations. Well that's a problem: ./tools/net/ynl/cli.py \ --spec Documentation/netlink/specs/nftables.yaml \ --multi batch-begin '{"res-id": 10}' \ --multi newtable '{"name": "test", "nfgen-family": 1}' \ --multi newchain '{"name": "chain", "table": "test", "nfgen-family": 1}' \ --multi batch-end '{"res-id": 10}' Adding: 20778 Adding: 20779 Adding: 20780 Adding: 20781 Done: 20779 Done: 20780 There's no response for 'batch-begin' or 'batch-end'. We may need a per op spec property to tell us if a request will be acknowledged. > > Would be good to add an example of multi executing some get operations. > > I think this was a blind spot on my part because nftables doesn't > support batch for get operations: > > https://elixir.bootlin.com/linux/latest/source/net/netfilter/nf_tables_api.c#L9092 > > I'll need to try using multi for gets without any batch messages and see how > everything behaves. Okay, so it can be made to work. Will incorporate into the next revision: ./tools/net/ynl/cli.py \ --spec Documentation/netlink/specs/nftables.yaml \ --multi gettable '{"name": "test", "nfgen-family": 1}' \ --multi getchain '{"name": "chain", "table": "test", "nfgen-family": 1}' [{'flags': set(), 'handle': 10, 'name': 'test', 'nfgen-family': 1, 'res-id': 200, 'use': 1, 'version': 0}, {'handle': 1, 'name': 'chain', 'nfgen-family': 1, 'res-id': 200, 'table': 'test', 'use': 0, 'version': 0}]
On Fri, 29 Mar 2024 21:01:09 +0000 Donald Hunter wrote: > There's no response for 'batch-begin' or 'batch-end'. We may need a > per op spec property to tell us if a request will be acknowledged. :( Pablo, could we possibly start processing the ACK flags on those messages? Maybe the existing user space doesn't set ACK so nobody would notice? I don't think the messages are otherwise marked as special from the "netlink layer" perspective. > > I think this was a blind spot on my part because nftables doesn't > > support batch for get operations: > > > > https://elixir.bootlin.com/linux/latest/source/net/netfilter/nf_tables_api.c#L9092 > > > > I'll need to try using multi for gets without any batch messages and see how > > everything behaves. > > Okay, so it can be made to work. Will incorporate into the next revision: Great!
On Fri, Mar 29, 2024 at 02:46:39PM -0700, Jakub Kicinski wrote: > On Fri, 29 Mar 2024 21:01:09 +0000 Donald Hunter wrote: > > There's no response for 'batch-begin' or 'batch-end'. We may need a > > per op spec property to tell us if a request will be acknowledged. > > :( > > Pablo, could we possibly start processing the ACK flags on those > messages? Maybe the existing user space doesn't set ACK so nobody > would notice? > > I don't think the messages are otherwise marked as special from > the "netlink layer" perspective. It is possible to explore this. I don't have a use-case for NLM_F_ACK and the begin marker message at this stage. Thanks.
diff --git a/tools/net/ynl/cli.py b/tools/net/ynl/cli.py index f131e33ac3ee..1b8f87b472ba 100755 --- a/tools/net/ynl/cli.py +++ b/tools/net/ynl/cli.py @@ -19,13 +19,23 @@ class YnlEncoder(json.JSONEncoder): def main(): - parser = argparse.ArgumentParser(description='YNL CLI sample') + description = """ + YNL CLI utility - a general purpose netlink utility that uses YNL specs + to drive protocol encoding and decoding. + """ + epilog = """ + The --multi option can be repeated to include several operations + in the same netlink payload. + """ + + parser = argparse.ArgumentParser(description=description, + epilog=epilog) parser.add_argument('--spec', dest='spec', type=str, required=True) parser.add_argument('--schema', dest='schema', type=str) parser.add_argument('--no-schema', action='store_true') parser.add_argument('--json', dest='json_text', type=str) - parser.add_argument('--do', dest='do', type=str) - parser.add_argument('--dump', dest='dump', type=str) + parser.add_argument('--do', dest='do', metavar='OPERATION', type=str) + parser.add_argument('--dump', dest='dump', metavar='OPERATION', type=str) parser.add_argument('--sleep', dest='sleep', type=int) parser.add_argument('--subscribe', dest='ntf', type=str) parser.add_argument('--replace', dest='flags', action='append_const', @@ -40,6 +50,8 @@ def main(): parser.add_argument('--output-json', action='store_true') parser.add_argument('--dbg-small-recv', default=0, const=4000, action='store', nargs='?', type=int) + parser.add_argument('--multi', dest='multi', nargs=2, action='append', + metavar=('OPERATION', 'JSON_TEXT'), type=str) args = parser.parse_args() def output(msg): @@ -73,6 +85,10 @@ def main(): if args.dump: reply = ynl.dump(args.dump, attrs) output(reply) + if args.multi: + ops = [ (item[0], json.loads(item[1]), args.flags) for item in args.multi ] + reply = ynl.do_multi(ops) + output(reply) except NlError as e: print(e) exit(1) diff --git a/tools/net/ynl/lib/ynl.py b/tools/net/ynl/lib/ynl.py index 557ef5a22b7d..cecd89db7d58 100644 --- a/tools/net/ynl/lib/ynl.py +++ b/tools/net/ynl/lib/ynl.py @@ -927,16 +927,11 @@ class YnlFamily(SpecFamily): return op['do']['request']['attributes'].copy() - def _op(self, method, vals, flags=None, dump=False): - op = self.ops[method] - + def _encode_message(self, op, vals, flags, req_seq): nl_flags = Netlink.NLM_F_REQUEST | Netlink.NLM_F_ACK for flag in flags or []: nl_flags |= flag - if dump: - nl_flags |= Netlink.NLM_F_DUMP - req_seq = random.randint(1024, 65535) msg = self.nlproto.message(nl_flags, op.req_value, 1, req_seq) if op.fixed_header: msg += self._encode_struct(op.fixed_header, vals) @@ -944,8 +939,20 @@ class YnlFamily(SpecFamily): for name, value in vals.items(): msg += self._add_attr(op.attr_set.name, name, value, search_attrs) msg = _genl_msg_finalize(msg) + return msg - self.sock.send(msg, 0) + def _ops(self, ops): + reqs_by_seq = {} + req_seq = random.randint(1024, 65535) + payload = b'' + for (method, vals, flags) in ops: + op = self.ops[method] + msg = self._encode_message(op, vals, flags, req_seq) + reqs_by_seq[req_seq] = (op, msg) + payload += msg + req_seq += 1 + + self.sock.send(payload, 0) done = False rsp = [] @@ -954,8 +961,9 @@ class YnlFamily(SpecFamily): nms = NlMsgs(reply, attr_space=op.attr_set) self._recv_dbg_print(reply, nms) for nl_msg in nms: - if nl_msg.extack: - self._decode_extack(msg, op, nl_msg.extack) + if nl_msg.extack and nl_msg.nl_seq in reqs_by_seq: + (req_op, req_msg) = reqs_by_seq[nl_msg.nl_seq] + self._decode_extack(req_msg, req_op, nl_msg.extack) if nl_msg.error: raise NlError(nl_msg) @@ -963,13 +971,15 @@ class YnlFamily(SpecFamily): if nl_msg.extack: print("Netlink warning:") print(nl_msg) + del reqs_by_seq[nl_msg.nl_seq] done = True break decoded = self.nlproto.decode(self, nl_msg) + rsp_op = self.rsp_by_value[decoded.cmd()] # Check if this is a reply to our request - if nl_msg.nl_seq != req_seq or decoded.cmd() != op.rsp_value: + if nl_msg.nl_seq not in reqs_by_seq or decoded.cmd() != rsp_op.rsp_value: if decoded.cmd() in self.async_msg_ids: self.handle_ntf(decoded) continue @@ -977,19 +987,26 @@ class YnlFamily(SpecFamily): print('Unexpected message: ' + repr(decoded)) continue - rsp_msg = self._decode(decoded.raw_attrs, op.attr_set.name) + rsp_msg = self._decode(decoded.raw_attrs, rsp_op.attr_set.name) if op.fixed_header: - rsp_msg.update(self._decode_struct(decoded.raw, op.fixed_header)) + rsp_msg.update(self._decode_struct(decoded.raw, rsp_op.fixed_header)) rsp.append(rsp_msg) if not rsp: return None - if not dump and len(rsp) == 1: + if not Netlink.NLM_F_DUMP in flags and len(rsp) == 1: return rsp[0] return rsp + def _op(self, method, vals, flags): + ops = [(method, vals, flags)] + return self._ops(ops) + def do(self, method, vals, flags=None): - return self._op(method, vals, flags) + return self._op(method, vals, flags or []) def dump(self, method, vals): - return self._op(method, vals, [], dump=True) + return self._op(method, vals, [Netlink.NLM_F_DUMP]) + + def do_multi(self, ops): + return self._ops(ops)
Add a "--multi <op> <json>" command line to ynl that makes it possible to add several operations to a single netlink request payload. The --multi command line option is repeated for each operation. This is used by the nftables family for transaction batches. For example: ./tools/net/ynl/cli.py \ --spec Documentation/netlink/specs/nftables.yaml \ --multi batch-begin '{"res-id": 10}' \ --multi newtable '{"name": "test", "nfgen-family": 1}' \ --multi newchain '{"name": "chain", "table": "test", "nfgen-family": 1}' \ --multi batch-end '{"res-id": 10}' Signed-off-by: Donald Hunter <donald.hunter@gmail.com> --- tools/net/ynl/cli.py | 22 ++++++++++++++++--- tools/net/ynl/lib/ynl.py | 47 +++++++++++++++++++++++++++------------- 2 files changed, 51 insertions(+), 18 deletions(-)