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

[Justin Pettit]

> 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()

> 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.

> 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.

> 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".

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.

Thanks,

--Justin