Thanks for the detailed review Justin On Tue, Jun 5, 2018 at 11:36 PM, Justin Pettit <jpet...@ovn.org> wrote:
> > > On Apr 8, 2018, at 7:53 PM, Darrell Ball <dlu...@gmail.com> wrote: > > > > diff --git a/lib/ct-dpif.c b/lib/ct-dpif.c > > index 5fa3a97..32d55c1 100644 > > --- a/lib/ct-dpif.c > > +++ b/lib/ct-dpif.c > > @@ -164,6 +164,14 @@ ct_dpif_get_nconns(struct dpif *dpif, uint32_t > *nconns) > > : EOPNOTSUPP); > > } > > > > +int > > +ct_dpif_ipf_change_enabled(struct dpif *dpif, bool v6, bool enable) > > +{ > > + return (dpif->dpif_class->ipf_change_enabled > > + ? dpif->dpif_class->ipf_change_enabled(dpif, v6, enable) > > + : EOPNOTSUPP); > > +} > > The command is "ipf-set-enabled", and I think that would be a good name > for the function, too. It's a bit shorter, and it follows the pattern of > other dpif functions. > yeah, originally I was using the same name locally and I changed it to this abysmal name; not sure why. > > diff --git a/lib/dpctl.c b/lib/dpctl.c > > index 47f4182..9fc0151 100644 > > --- a/lib/dpctl.c > > +++ b/lib/dpctl.c > > @@ -35,6 +35,7 @@ > > > > ... > > +static int > > +dpctl_ct_ipf_change_enabled(int argc, const char *argv[], > > + struct dpctl_params *dpctl_p) > > +{ > > + struct dpif *dpif; > > + int error = dpctl_ct_open_dp(argc, argv, dpctl_p, &dpif, 4); > > + if (!error) { > > + char v4_or_v6[3] = {0}; > > + if (ovs_scan(argv[argc - 2], "%2s", v4_or_v6) && > > + (!strncmp(v4_or_v6, "v4", 2) || !strncmp(v4_or_v6, "v6", > 2))) { > > + uint32_t enabled; > > + if (ovs_scan(argv[argc - 1], "%"SCNu32, &enabled)) { > > + error = ct_dpif_ipf_change_enabled( > > + dpif, !strncmp(v4_or_v6, "v6", 2), enabled); > > + if (!error) { > > + dpctl_print(dpctl_p, > > + "changing fragmentation enabled > successful"); > > + } else { > > + dpctl_error(dpctl_p, error, > > + "changing fragmentation enabled > failed"); > > I think these two messages would be confusing if someone were attempted to > disable fragmentation. How about putting some code in that prints whether > they were enabling or disabling fragmentation reassembly. For example, > "enabling fragmentation reassembly successful" or "disabling fragmentation > reassembly successful". > yeah, I was lazy and used some prior art error message style. > > > + } > > + } else { > > + error = EINVAL; > > + dpctl_error( > > + dpctl_p, error, > > + "parameter missing: 0 for disabled or 1 for > enabled"); > > + } > > + } else { > > + error = EINVAL; > > + dpctl_error(dpctl_p, error, > > + "parameter missing: v4 for ipv4 or v6 for > ipv6"); > > I would quote "v4" and "v6" to make it clear exactly what arguments were > required. > good point > > > diff --git a/lib/dpctl.man b/lib/dpctl.man > > index 9e9d2dc..9bf489c 100644 > > --- a/lib/dpctl.man > > +++ b/lib/dpctl.man > > @@ -268,3 +268,14 @@ Only supported for userspace datapath. > > \*(DX\fBct\-get\-nconns\fR [\fIdp\fR] > > Read the current number of connection tracker connections. > > Only supported for userspace datapath. > > +. > > +.TP > > +\*(DX\fBipf\-set\-enabled\fR [\fIdp\fR] [\fIv4 or v6\fR] \fBenable\fR > > I think you want to us "\fB" for "v4" and "v6", since they're constants, > and you should drop the brackets since the argument isn't optional. Also, > the "or" shouldn't be included, and I think using "|" is more common. I > think you want something like the following: "\fBv4\fR | \fBv6\fR". > ughhh, did I write "\fIv4 or v6\fR" ? thanks > It seems like the argument is 0 or 1, and not "enable". How about > exposing separate commands for enable and disable? I think it would be > clearer, and the internal implementation could remain the same passing bool > arguments. > > > +Enables or disables fragmentation handling for the userspace datapath > > +connection tracker. Either v4 or v6 must be specified. Both v4 and v6 > > These references to "v4" and "v6" should be bolded. > yep, thanks > > > +are enabled by default. When fragmentation handling is enabled, the > > I'd mention that they're default "on" at the end. > yep; was on my todo list for a brief moment > > > +rules for handling fragments before entering conntrack should not > > +differentiate between first and other fragments. Although, this would > > +logically already be true anyways, it is mentioned for clarity. If > there > > I would drop the "Although" sentence, since it doesn't add anything. > yep; not needed. > > > diff --git a/lib/dpif-provider.h b/lib/dpif-provider.h > > index 62b3598..08e0944 100644 > > --- a/lib/dpif-provider.h > > +++ b/lib/dpif-provider.h > > @@ -444,6 +444,8 @@ struct dpif_class { > > /* Get number of connections tracked. */ > > int (*ct_get_nconns)(struct dpif *, uint32_t *nconns); > > > > + /* IP Fragmentation. */ > > + int (*ipf_change_enabled)(struct dpif *, bool, bool); > > /* Meters */ > > I would put a blank line before "/* Meters */" to break them into sections. > yep > > > diff --git a/lib/ipf.c b/lib/ipf.c > > index 3837c60..54f27d2 100644 > > --- a/lib/ipf.c > > +++ b/lib/ipf.c > > @@ -1236,3 +1236,18 @@ ipf_destroy(void) > > ipf_lock_unlock(&ipf_lock); > > ipf_lock_destroy(&ipf_lock); > > } > > + > > +int > > +ipf_change_enabled(bool v6, bool enable) > > All the other arguments in "lib/ipf.c" are 'v4'. How about using it here, > too, instead of 'v6'? (And propagating it up through the call stack.) > The intention was for at least external interfaces to use "bool v6" parameter (ipf_set_min_frag and ipf_change_enabled) with v4 implied. I intended to go back and change the internal APIs to same, but did not get back to it. I probably will change the internal APIs to v6 parameter. > > > +{ > > + if ((v6 != true && v6 != false) || > > + (enable != true && enable != false)) { > > + return 1; > > + } > > If they're bools, how would they be anything be true or false? > At one point, the API chain from dpctl, including ct_dpif_ipf_change_enabled(), dpif_netdev_ipf_change_enabled() and ipf_change_enabled() were "uint32_t" values and I was doing the range checking at the ipf layer. When I changed the other APIs below dpctl to bool. I intended to move the range checking to dpctl. I just noticed the range check is missing in dpctl and I will move this range check there. > > > diff --git a/lib/ipf.h b/lib/ipf.h > > index 5861e96..0b45de9 100644 > > --- a/lib/ipf.h > > +++ b/lib/ipf.h > > @@ -60,4 +60,7 @@ ipf_init(void); > > void > > ipf_destroy(void); > > > > +int > > +ipf_change_enabled(bool v6, bool enable); > > According to the style guide, the return type and function name should be > on the same line. > thanks > > Thanks, > > --Justin > > > _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev