[ovs-dev] [patch v6 06/10] ipf: Add set minimum fragment size command.

Justin Pettit jpettit at ovn.org
Thu Jun 7 05:00:18 UTC 2018


> On Apr 8, 2018, at 7:54 PM, Darrell Ball <dlu998 at gmail.com> wrote:

> diff --git a/lib/dpctl.c b/lib/dpctl.c
> index 9fc0151..f6c0a87 100644
> --- a/lib/dpctl.c
> +++ b/lib/dpctl.c
> @@ -1786,6 +1786,44 @@ dpctl_ct_ipf_change_enabled(int argc, const char *argv[],
>     return error;
> }
> 
> +static int
> +dpctl_ct_ipf_set_min_frag(int argc, const char *argv[],
> +                          struct dpctl_params *dpctl_p)
> +{
> ...
> +                if (!error) {
> +                    dpctl_print(dpctl_p,
> +                                "setting minimum fragment size successful");
> +                } else {
> +                    dpctl_error(dpctl_p, error,
> +                                "setting minimum fragment size failed");
> +                }

It might be nice to give users an indication of why this failed.  It looks like that will only happen if the value specified isn't valid, so it may be worth just saying that much.

> diff --git a/lib/dpctl.man b/lib/dpctl.man
> index 9bf489c..6223c15 100644
> --- a/lib/dpctl.man
> +++ b/lib/dpctl.man
> @@ -279,3 +279,10 @@ differentiate between first and other fragments.  Although, this would
> logically already be true anyways, it is mentioned for clarity.  If there
> is a need to differentiate between first and other fragments, do it after
> conntrack.
> +.
> +.TP
> +\*(DX\fBipf\-set\-minfrag\fR [\fIdp\fR] [\fIv4 or v6\fR] \fBminfrag\fR
> +Sets the minimum fragment size supported by the userspace datapath
> +connection tracker.  Either v4 or v6 must be specified.  The default v4
> +value is 1200 and the clamped minimum is 400.  The default v6 value is
> +1280, which is also the clamped minimum.

I think it would be worth explaining a bit more about this parameter.  Can you explain the difference between the value being set here and the clamped value?

I like the name of the function, so what about calling the command "ipf-set-min-frag"?

For all of these functions, it may be worth mentioning that they only apply to the userspace datapath.

> 
> diff --git a/lib/dpif-provider.h b/lib/dpif-provider.h
> index 08e0944..aa9c490 100644
> --- a/lib/dpif-provider.h
> +++ b/lib/dpif-provider.h
> @@ -446,6 +446,8 @@ struct dpif_class {
> 
>     /* IP Fragmentation. */
>     int (*ipf_change_enabled)(struct dpif *, bool, bool);
> +    /* Set minimum fragment allowed. */
> +    int (*ipf_set_min_frag)(struct dpif *, bool, uint32_t);

For all these definitions, I think it would be worth adding the argument names so the prototypes are a bit clearer.

> 
> diff --git a/lib/ipf.c b/lib/ipf.c
> index 54f27d2..24d9b06 100644
> --- a/lib/ipf.c
> +++ b/lib/ipf.c
> @@ -1251,3 +1251,26 @@ ipf_change_enabled(bool v6, bool enable)
>     }
>     return 0;
> }
> +
> +int
> +ipf_set_min_frag(bool v6, uint32_t value)
> +{
> +    /* If the user specifies an unreasonably large number, fragmentation
> +     * will not work well but it will not blow up. */

It won't blow up, but won't it drop fragments from legitimate IP stacks?

I didn't call them out, but some of my comments on the disabling fragmentation handling patches could apply here, too.

Thanks,

--Justin




More information about the dev mailing list