Thanks for the detailed review Justin
On Wed, Jun 6, 2018 at 10:00 PM, Justin Pettit <[email protected]> wrote: > > > 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. > 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 > > > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
