[ovs-dev] [patch v6 09/10] ipf: Enhance ipf_get_status.

Darrell Ball dlu998 at gmail.com
Mon Jul 9 17:00:40 UTC 2018


On Thu, Jun 7, 2018 at 12:11 AM, 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/ct-dpif.c b/lib/ct-dpif.c
> > index 60c8986..adcf42b 100644
> > --- a/lib/ct-dpif.c
> > +++ b/lib/ct-dpif.c
> > @@ -209,6 +209,30 @@ int ct_dpif_ipf_get_status(struct dpif *dpif, bool
> *ipf_v4_enabled,
> >             : EOPNOTSUPP);
> > }
> >
> > +int
> > +ct_dpif_ipf_dump_start(struct dpif *dpif, struct ipf_dump_ctx
> **dump_ctx)
> > +{
> > +    return (dpif->dpif_class->ipf_dump_start
> > +           ? dpif->dpif_class->ipf_dump_start(dpif, dump_ctx)
> > +           : EOPNOTSUPP);
> > +}
> > +
> > +int
> > +ct_dpif_ipf_dump_next(struct dpif *dpif, void *dump_ctx,  char **dump)
> > +{
> > +    return (dpif->dpif_class->ipf_dump_next
> > +            ? dpif->dpif_class->ipf_dump_next(dpif, dump_ctx, dump)
> > +            : EOPNOTSUPP);
> > +}
> > +
> > +int
> > +ct_dpif_ipf_dump_done(struct dpif *dpif, void *dump_ctx)
> > +{
> > +    return (dpif->dpif_class->ipf_dump_done
> > +            ? dpif->dpif_class->ipf_dump_done(dpif, dump_ctx)
> > +            : EOPNOTSUPP);
> > +}
>
> It would be helpful to have descriptions for these functions, including
> mentioning that '*dump' must be freed and that ct_dpif_ipf_dump_done() must
> be called after a call to ct_dpif_ipf_dump_start()
>


sure; I added descriptions



>
> > diff --git a/lib/dpctl.c b/lib/dpctl.c
> > index 84064cd..7c1aa65 100644
> > --- a/lib/dpctl.c
> > +++ b/lib/dpctl.c
> >
> > @@ -1852,12 +1853,37 @@ dpctl_ct_ipf_set_nfrag_max(int argc, const char
> *argv[],
> >     return error;
> > }
> >
> > +static void
> > +dpctl_dump_ipf(struct dpif *dpif, struct dpctl_params *dpctl_p)
> > +{
> > +    struct ipf_dump_ctx *dump_ctx;
> > +    char *dump;
> > +
> > +    int error = ct_dpif_ipf_dump_start(dpif, &dump_ctx);
> > +    if (error) {
> > +        dpctl_error(dpctl_p, error, "starting ipf dump");
> > +        return;
> > +    }
> > +
> > +    dpctl_print(dpctl_p, "\n\tFragment Lists:\n\n");
>
> I think this is a single list.
>


It is a list of fragment lists, where one fragment list represents a list
of fragments, so there are multiple "fragment lists".



>
> > diff --git a/lib/dpctl.man b/lib/dpctl.man
> > index 2e8c287..afb270e 100644
> > --- a/lib/dpctl.man
> > +++ b/lib/dpctl.man
> > @@ -296,6 +296,7 @@ module while fragments are incomplete, but will
> timeout after 15 seconds.
> > Memory pool sizing should be set accordingly when fragmentation is
> enabled.
> > .
> > .TP
> > -\*(DX\fBipf\-get\-status\fR [\fIdp\fR]
> > +\*(DX\fBipf\-get\-status\fR [\fIdp\fR] [\fIverbose\fR]
> > Gets the configuration settings and fragment counters associated with the
> > -fragmentation handling of the userspace datapath connection tracker.
> > +fragmentation handling of the userspace datapath connection tracker.  If
> > +verbose is specified, also dumps the ipf list entries.
>
> We usually use "-m" and "--more" as the flags to indicate verbose.  The
> description of "dump-conntrack" provides one example.  Also, like
> "dump-conntrack", it might be nice to provide a "zone" argument.
>


Since I saw both "verbose" and "more" used, I was not sure which was
preferred
I switched over to "more"; it seems to cover a wider range of meanings.

I left out the "zone" option for now; these lists are expected to be very
transient under normal conditions unlike conntrack and the if there is an
error
condition with lots of fragment lists, the user can always just grep the
zone value.



>
> > diff --git a/lib/dpif-provider.h b/lib/dpif-provider.h
> > index 82fbbfc..385394f 100644
> > --- a/lib/dpif-provider.h
> > +++ b/lib/dpif-provider.h
> > @@ -24,6 +24,7 @@
> >
> > #include "openflow/openflow.h"
> > #include "dpif.h"
> > +#include "ipf.h"
> > #include "util.h"
> >
> > #ifdef  __cplusplus
> > @@ -457,6 +458,10 @@ struct dpif_class {
> >         unsigned int *, bool *, unsigned int *, unsigned int *,
> >         unsigned int *, unsigned int *, unsigned int *,
> >         unsigned int *);
> > +    int (*ipf_dump_start)(struct dpif *, struct ipf_dump_ctx **);
> > +    /* Gets an ipf list entry to display. */
> > +    int (*ipf_dump_next)(struct dpif *, void *, char **);
>
> In reference to this comment, it doesn't really get an ipf entry as much
> as write it to a buffer.
>
> > +static void
> > +ipf_dump_create(const struct ipf_list *ipf_list, struct ds *ds)
> > +{
> > +
> > +    ds_put_cstr(ds, "frag list elem=(");
>
> Since the header will state that this is a frag list, I don't think it's
> necessary to prepend this to each element.  Ideally this would look and act
> similar to other dump commands such as "dump-conntrack".
>

it is not needed; I dropped the prepending.



>
> This appears to be just adding a verbose option to the previous patch, so
> I think these patches can just be merged into a single one.
>


I originally spliced out this patch since it covers internal state, whereas
the first patch is externally observable information
and thinking it was easier to review being split up.

Merging together is also fine and easier for me to administer, so sure, I
merged them now.



>
> Thanks,
>
> --Justin
>
>
>


More information about the dev mailing list