[ovs-dev] [patch v6 06/10] ipf: Add set minimum fragment size command.
Darrell Ball
dlu998 at gmail.com
Mon Jul 9 16:56:52 UTC 2018
Thanks for the detailed review Justin
On Wed, Jun 6, 2018 at 10:00 PM, Justin Pettit <jpettit at ovn.org> wrote:
>
> > 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.
>
oops, missed this one; I took the time to add for the other cases but not
here - ADD no doubt.
>
> > 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 added more description
>
> I like the name of the function, so what about calling the command
> "ipf-set-min-frag"?
>
I was trying to avoid the 4-part name, guessing people would not like it; I
prefer the 4-part name myself, so we are
in agreement.
>
> For all of these functions, it may be worth mentioning that they only
> apply to the userspace datapath.
>
All the APIs already say that, so I think we are covered.
>
> >
> > 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.
>
yep, this file should follow the practice of using the argument names.
>
> >
> > 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 added a comment just to be on the safe side.
>
> I didn't call them out, but some of my comments on the disabling
> fragmentation handling patches could apply here, too.
>
Got it.
>
> Thanks,
>
> --Justin
>
>
>
More information about the dev
mailing list