On 6/25/24 13:12, Ales Musil wrote:
>
>
> On Tue, Jun 25, 2024 at 1:02 PM Ilya Maximets <i.maxim...@ovn.org
> <mailto:i.maxim...@ovn.org>> wrote:
>
> On 6/24/24 17:52, Ales Musil via discuss wrote:
> > Hi,
> >
> > I would like to propose a universal coding style using clang-format
> [0]. I've put
> > together the config file that is matching the current coding style as
> closely as
> > possible [1]. I've tried it on some random pieces of code that we have
> in the OVN
> > codebase and there are some instances where it doesn't much 1:1 (down
> below). The
> > idea would be to add this into CI and run this only on diff so we don't
> have to
> > modify old code. Let me know what you think and if it would be
> acceptable. In the
> > end we can have a style and let some pieces of code diverge on that.
> >
> > Before:
> > ctl_error(ctx, "Same routing policy already existed on the "
> > "logical router %s.", ctx->argv[1]);
> >
> > After:
> >
> > ctl_error(ctx,
> > "Same routing policy already existed on the logical "
> > "router %s.",
> > ctx->argv[1]);
> >
> > Before:
> > return (out_port != VIGP_CONTROL_PATH
> > ? alpheus_output_port(dp, skb, out_port)
> > : alpheus_output_control(dp, skb, fwd_save_skb(skb),
> > VIGR_ACTION));
> >
> > After:
> > return (
> > out_port != VIGP_CONTROL_PATH
> > ? alpheus_output_port(dp, skb, out_port)
> > : alpheus_output_control(dp, skb, fwd_save_skb(skb),
> VIGR_ACTION));
>
> IMHO, the extra indentation on ? and : lines doesn't look good.
>
> >
> >
> >
> > Thanks,
> > Ales
> >
> >
> > [0] https://gist.github.com/almusil/d9fc05c9e9ab6cef9448e123b49c351e
> <https://gist.github.com/almusil/d9fc05c9e9ab6cef9448e123b49c351e>
> > [1]
> https://docs.ovn.org/en/stable/internals/contributing/coding-style.html
> <https://docs.ovn.org/en/stable/internals/contributing/coding-style.html>
> Having an automatic formatter will always have some issues, so it should
> never be gating. Some other problems I see running the provided [0]
> format
> config on some OVN files:
>
>
> Hi Ilya,
>
>
> 1.
> -static void update_lport_tracking(const struct sbrec_port_binding *pb,
> - struct hmap *tracked_dp_bindings,
> - bool claimed);
> +static void
> +update_lport_tracking(const struct sbrec_port_binding *pb,
> + struct hmap *tracked_dp_bindings, bool claimed);
>
> It moves function names in prototypes to the new line, which is a big
> no-no for me.
>
>
>
> This can be changed in config, however that option is unfortunately either
> prototypes + definitions
> or nothing on the newline.
If it's both or nothing, then this check should be disabled, I think.
The whole point is to have different formatting for prototypes and
definitions.
>
>
> 2. It sorts header includes which is not always a correct thing to do,
> e.g. some system headers must be included in a certain order, so we'll
> need to look out for issues like this.
>
>
>
> Sorting can be disabled completely.
>
>
> 3. You're adding all the iteration macros to the configuration, these
> will need to be generated from database schemas at the build step in
> order to avoid false-positives. Not sure how to handle OVS database
> schema changes.
>
>
> In theory we don't need to have them, but then it won't add the extra space.
>
>
>
> 4. It tends to fit many function arguments into single line, we don't
> always want that. Sometimes we want one argument per line to
> minimize the mess on changes.
>
> 5. This doesn't look good:
>
> @@ -3642,8 +3576,7 @@ binding_lport_check_and_cleanup(struct
> binding_lport *b_lport,
> case LP_EXTERNAL:
> case LP_LOCALNET:
> case LP_REMOTE:
> - case LP_UNKNOWN:
> - cleanup_blport = true;
> + case LP_UNKNOWN: cleanup_blport = true;
>
>
> This is allowed according to the coding style:
It's not OK to do that for fall-through cases.
>
> |switch| statements with very short, uniform cases may use an abbreviated
> style:
>
> switch (code) {
> case 200: return "OK";
> case 201: return "Created";
> case 202: return "Accepted";
> case 204: return "No Content";
> default: return "Unknown";
> }
>
>
> But again it's something that can be changed in the config.
>
>
> }
>
> cleanup:
>
> 6. This one is also strange:
>
> - const char *iface_id_ver = smap_get(&iface->external_ids,
> - "iface-id-ver");
> + const char *iface_id_ver = smap_get(&iface->external_ids,
> "iface-id-"
> +
> "ver");
>
> This one is just weird:
>
> - const char *chassis_name =
> smap_get(&sbha_ch->external_ids,
> - "chassis-name");
> + const char *chassis_name =
> smap_get(&sbha_ch->external_ids, "chass"
> +
> "is-"
> +
> "nam"
> +
> "e");
>
>
>
>
> I agree this is bad, there might be a way to avoid this string splits.
>
>
>
> 7. It aligns closely placed definitions:
>
> -#define OVN_INSTALLED_EXT_ID "ovn-installed"
> +#define OVN_INSTALLED_EXT_ID "ovn-installed"
> #define OVN_INSTALLED_TS_EXT_ID "ovn-installed-ts"
>
> Feels like Go, and it will have the same issue as in Go - it will ask
> to re-align everything if a slightly longer definition is added.
>
>
> Again this can be disabled.
>
>
>
> 8. A crime against the coding style:
>
> @@ -91,7 +90,6 @@ static bool use_ct_inv_match = true;
> static bool default_acl_drop;
>
> #define MAX_OVN_TAGS 4096
> -^L
>
> /* Due to various hard-coded priorities need to implement ACLs, the
> * northbound database supports a smaller range of ACL priorities than
>
> 9. Destroyed formatting in comments:
>
> @@ -201,25 +198,25 @@ static bool default_acl_drop;
> *
> * Logical Switch pipeline:
> *
> +----+----------------------------------------------+---+-----------------------------------+
> - * | R0 | REGBIT_{CONNTRACK/DHCP/DNS} | |
> |
> - * | | REGBIT_{HAIRPIN/HAIRPIN_REPLY} | |
> |
> - * | | REGBIT_ACL_HINT_{ALLOW_NEW/ALLOW/DROP/BLOCK} | |
> |
> - * | | REGBIT_ACL_{LABEL/STATELESS} | X |
> |
> - * +----+----------------------------------------------+ X |
> |
> - * | R5 | UNUSED | X |
> LB_L2_AFF_BACKEND_IP6 |
> - * | R1 | ORIG_DIP_IPV4 (>= IN_PRE_STATEFUL) | R |
> |
> - * +----+----------------------------------------------+ E |
> |
> - * | R2 | ORIG_TP_DPORT (>= IN_PRE_STATEFUL) | G |
> |
> - * +----+----------------------------------------------+ 0 |
> |
> - * | R3 | ACL LABEL | |
> |
> + * | R0 | REGBIT_{CONNTRACK/DHCP/DNS} | | | | |
> + * REGBIT_{HAIRPIN/HAIRPIN_REPLY} | | | | |
> + * REGBIT_ACL_HINT_{ALLOW_NEW/ALLOW/DROP/BLOCK} | | | | |
> + * REGBIT_ACL_{LABEL/STATELESS} | X | |
> + * +----+----------------------------------------------+ X | | | R5 |
> UNUSED |
> + * X | LB_L2_AFF_BACKEND_IP6 | | R1 |
> ORIG_DIP_IPV4 (>=
> + * IN_PRE_STATEFUL) | R | |
> + * +----+----------------------------------------------+ E | | | R2 |
> + * ORIG_TP_DPORT (>= IN_PRE_STATEFUL) | G | |
> + * +----+----------------------------------------------+ 0 | | | R3 |
> ACL LABEL
> + * | | |
> *
> +----+----------------------------------------------+---+-----------------------------------+
>
> 10. I don't like this:
>
> -#define ICMP4_NEED_FRAG_FORMAT \
> - "icmp4_error {" \
> - "%s" \
> - REGBIT_EGRESS_LOOPBACK" = 1; " \
> - REGBIT_PKT_LARGER" = 0; " \
> - "eth.dst = %s; " \
> - "ip4.dst = ip4.src; " \
> - "ip4.src = %s; " \
> - "ip.ttl = 255; " \
> ...
> +#define ICMP4_NEED_FRAG_FORMAT \
> + "icmp4_error {" \
> + "%s" REGBIT_EGRESS_LOOPBACK " = 1; " REGBIT_PKT_LARGER " = 0; " \
> + "eth.dst = %s; " \
> + "ip4.dst = ip4.src; " \
> + "ip4.src = %s; " \
> + "ip.ttl = 255; " \
>
> It is important to keep bits on separate lines here, otherwise it's
> getting much harder to read.
>
> The following wasn't great before, but became much worse after:
>
> ds_put_format(actions,
> - REG_DHCP_RELAY_DIP_IPV4" = ip4.dst; "
> - REGBIT_DHCP_RELAY_RESP_CHK
> - " = dhcp_relay_resp_chk(%s, %s); next; /* DHCP_RELAY_RESP
> */",
> - op->lrp_networks.ipv4_addrs[0].addr_s, server_ip_str);
> + REG_DHCP_RELAY_DIP_IPV4 " = "
> + "ip4.dst;"
> + " "
> REGBIT_DHCP_RELAY_RESP_CHK
> + " = dhcp_relay_resp_chk(%s,
> %s); "
> + "next; /* DHCP_RELAY_RESP
> */",
> + op->lrp_networks.ipv4_addrs[0].addr_s,
> server_ip_str);
>
> In general, northd code is not easy to read, but I really don't like
> what clang-format does with it in many places.
>
>
> I only checked two OVN files, so maybe there is more.
>
> Also, have you tried GNU indent referenced in the coding style document?
>
>
> Last time I tried indent it had a lot of similar issues, as you have
> mentioned far from perfect for our specific style.
>
>
> It is also far from being perfect though.
>
> Bets regards, Ilya Maximets.
>
>
> That's the problem with any codebase that might adapt some enforced coding
> style. I'm open to suggestions when it comes to the config tweaking.
>
> Thanks,
> Ales
>
> --
>
> Ales Musil
>
> Senior Software Engineer - OVN Core
>
> Red Hat EMEA <https://www.redhat.com>
>
> amu...@redhat.com <mailto:amu...@redhat.com>
>
> <https://red.ht/sig>
>
_______________________________________________
discuss mailing list
disc...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-discuss
