On Tue, Jun 25, 2024 at 1:02 PM Ilya Maximets <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 > > [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: > 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. > 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: 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 <https://red.ht/sig>
_______________________________________________ discuss mailing list disc...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-discuss
