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 > [1] 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: 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. 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. 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. 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; } 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"); 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. 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? It is also far from being perfect though. Bets regards, Ilya Maximets. _______________________________________________ discuss mailing list disc...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-discuss
