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
