On 7/8/2019 8:54 PM, Marcelo Ricardo Leitner wrote:
> On Sun, Jul 07, 2019 at 11:53:47AM +0300, Paul Blakey wrote:
>> New tc action to send packets to conntrack module, commit
>> them, and set a zone, labels, mark, and nat on the connection.
>>
>> It can also clear the packet's conntrack state by using clear.
>>
>> Usage:
>> ct clear
>> ct commit [force] [zone] [mark] [label] [nat]
> Isn't the 'commit' also optional? More like
> ct [commit [force]] [zone] [mark] [label] [nat]
>
>> ct [nat] [zone]
>>
>> Signed-off-by: Paul Blakey <pa...@mellanox.com>
>> Signed-off-by: Marcelo Ricardo Leitner <marcelo.leit...@gmail.com>
>> Signed-off-by: Yossi Kuperman <yoss...@mellanox.com>
>> Acked-by: Jiri Pirko <j...@mellanox.com>
>> Acked-by: Roi Dayan <r...@mellanox.com>
>> ---
> ...
>> +static void
>> +usage(void)
>> +{
>> + fprintf(stderr,
>> + "Usage: ct clear\n"
>> + " ct commit [force] [zone ZONE] [mark MASKED_MARK] [label
>> MASKED_LABEL] [nat NAT_SPEC]\n"
> Ditto here then.
In commit msg and here, it means there is multiple modes of operation. I
think it's easier to split those.
"ct clear" to clear it , not other options can be added here.
"ct commit [force].... " sends to conntrack and commit a connection,
and only for commit can you specify force mark label, and nat with
nat_spec....
and the last one, "ct [nat] [zone ZONE]" is to just send the packet to
conntrack on some zone [optional], restore nat [optional].
>
>> + " ct [nat] [zone ZONE]\n"
>> + "Where: ZONE is the conntrack zone table number\n"
>> + " NAT_SPEC is {src|dst} addr addr1[-addr2] [port
>> port1[-port2]]\n"
>> + "\n");
>> + exit(-1);
>> +}
> ...
>
> The validation below doesn't enforce that commit must be there for
> such case.
which case? commit is optional. the above are the three valid patterns.
>> +static int
>> +parse_ct(struct action_util *a, int *argc_p, char ***argv_p, int tca_id,
>> + struct nlmsghdr *n)
>> +{
>> + struct tc_ct sel = {};
>> + char **argv = *argv_p;
>> + struct rtattr *tail;
>> + int argc = *argc_p;
>> + int ct_action = 0;
>> + int ret;
>> +
>> + tail = addattr_nest(n, MAX_MSG, tca_id);
>> +
>> + if (argc && matches(*argv, "ct") == 0)
>> + NEXT_ARG_FWD();
>> +
>> + while (argc > 0) {
>> + if (matches(*argv, "zone") == 0) {
>> + NEXT_ARG();
>> +
>> + if (ct_parse_u16(*argv,
>> + TCA_CT_ZONE, TCA_CT_UNSPEC, n)) {
>> + fprintf(stderr, "ct: Illegal \"zone\"\n");
>> + return -1;
>> + }
>> + } else if (matches(*argv, "nat") == 0) {
>> + ct_action |= TCA_CT_ACT_NAT;
>> +
>> + NEXT_ARG();
>> + if (matches(*argv, "src") == 0)
>> + ct_action |= TCA_CT_ACT_NAT_SRC;
>> + else if (matches(*argv, "dst") == 0)
>> + ct_action |= TCA_CT_ACT_NAT_DST;
>> + else
>> + continue;
>> +
>> + NEXT_ARG();
>> + if (matches(*argv, "addr") != 0)
>> + usage();
>> +
>> + NEXT_ARG();
>> + ret = ct_parse_nat_addr_range(*argv, n);
>> + if (ret) {
>> + fprintf(stderr, "ct: Illegal nat address
>> range\n");
>> + return -1;
>> + }
>> +
>> + NEXT_ARG_FWD();
>> + if (matches(*argv, "port") != 0)
>> + continue;
>> +
>> + NEXT_ARG();
>> + ret = ct_parse_nat_port_range(*argv, n);
>> + if (ret) {
>> + fprintf(stderr, "ct: Illegal nat port range\n");
>> + return -1;
>> + }
>> + } else if (matches(*argv, "clear") == 0) {
>> + ct_action |= TCA_CT_ACT_CLEAR;
>> + } else if (matches(*argv, "commit") == 0) {
>> + ct_action |= TCA_CT_ACT_COMMIT;
>> + } else if (matches(*argv, "force") == 0) {
>> + ct_action |= TCA_CT_ACT_FORCE;
>> + } else if (matches(*argv, "index") == 0) {
>> + NEXT_ARG();
>> + if (get_u32(&sel.index, *argv, 10)) {
>> + fprintf(stderr, "ct: Illegal \"index\"\n");
>> + return -1;
>> + }
>> + } else if (matches(*argv, "mark") == 0) {
>> + NEXT_ARG();
>> +
>> + ret = ct_parse_mark(*argv, n);
>> + if (ret) {
>> + fprintf(stderr, "ct: Illegal \"mark\"\n");
>> + return -1;
>> + }
>> + } else if (matches(*argv, "label") == 0) {
>> + NEXT_ARG();
>> +
>> + ret = ct_parse_labels(*argv, n);
>> + if (ret) {
>> + fprintf(stderr, "ct: Illegal \"label\"\n");
>> + return -1;
>> + }
>> + } else if (matches(*argv, "help") == 0) {
>> + usage();
>> + } else {
>> + break;
>> + }
>> + NEXT_ARG_FWD();
>> + }
>> +
>> + if (ct_action & TCA_CT_ACT_CLEAR &&
>> + ct_action & ~TCA_CT_ACT_CLEAR) {
>> + fprintf(stderr, "ct: clear can only be used alone\n");
>> + return -1;
>> + }
>> +
>> + if (ct_action & TCA_CT_ACT_NAT_SRC &&
>> + ct_action & TCA_CT_ACT_NAT_DST) {
>> + fprintf(stderr, "ct: src and dst nat can't be used together\n");
>> + return -1;
>> + }
>> +
>> + if ((ct_action & TCA_CT_ACT_COMMIT) &&
>> + (ct_action & TCA_CT_ACT_NAT) &&
>> + !(ct_action & (TCA_CT_ACT_NAT_SRC | TCA_CT_ACT_NAT_DST))) {
>> + fprintf(stderr, "ct: commit and nat must set src or dst\n");
>> + return -1;
>> + }
>> +
>> + if (!(ct_action & TCA_CT_ACT_COMMIT) &&
>> + (ct_action & (TCA_CT_ACT_NAT_SRC | TCA_CT_ACT_NAT_DST))) {
>> + fprintf(stderr, "ct: src or dst is only valid if commit is
>> set\n");
>> + return -1;
>> + }
>> +
>> + parse_action_control_dflt(&argc, &argv, &sel.action, false,
>> + TC_ACT_PIPE);
>> + NEXT_ARG_FWD();
>> +
>> + addattr16(n, MAX_MSG, TCA_CT_ACTION, ct_action);
>> + addattr_l(n, MAX_MSG, TCA_CT_PARMS, &sel, sizeof(sel));
>> + addattr_nest_end(n, tail);
>> +
>> + *argc_p = argc;
>> + *argv_p = argv;
>> + return 0;
>> +}
> ...
