[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