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

[Justin Pettit]

> 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.

> 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".

> +                }
> +            } 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.

> 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".

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.

> +are enabled by default.  When fragmentation handling is enabled, the

I'd mention that they're default "on" at the end.

> +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.

> 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.

> 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.)

> +{
> +    if ((v6 != true && v6 != false) ||
> +        (enable != true && enable != false)) {
> +        return 1;
> +    }

If they're bools, how would they be anything be true or false?

> 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,

--Justin