> On Apr 8, 2018, at 7:54 PM, Darrell Ball <[email protected]> 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
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
