[ovs-dev] [patch v6 05/10] ipf: Add command to disable fragmentation handling.

Darrell Ball dlu998 at gmail.com
Thu Jun 7 03:32:39 UTC 2018


Thanks for the detailed review Justin

On Tue, Jun 5, 2018 at 11:36 PM, Justin Pettit <jpettit at ovn.org> wrote:

>
> > On Apr 8, 2018, at 7:53 PM, Darrell Ball <dlu998 at 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
>
>
>


More information about the dev mailing list