Re: [ovs-dev] [patch v6 08/10] ipf: Add command to get fragmentation handling status.

[Darrell Ball]

On Wed, Jun 6, 2018 at 10:23 PM, Justin Pettit  wrote:

>
> > On Apr 8, 2018, at 7:54 PM, Darrell Ball  wrote:
> >
> > diff --git a/lib/ct-dpif.c b/lib/ct-dpif.c
> > index 81f9d92..60c8986 100644
> > --- a/lib/ct-dpif.c
> > +++ b/lib/ct-dpif.c
> > @@ -188,6 +188,27 @@ ct_dpif_ipf_set_nfrag_max(struct dpif *dpif,
> uint32_t max_frags)
> > : EOPNOTSUPP);
> > }
> >
> > +int ct_dpif_ipf_get_status(struct dpif *dpif, bool *ipf_v4_enabled,
> > +unsigned int *min_v4_frag_size, unsigned int *nfrag_max,
> > +unsigned int *nfrag, unsigned int *n4frag_accepted,
> > +unsigned int *n4frag_completed_sent,
> > +unsigned int *n4frag_expired_sent, unsigned int *n4frag_too_small,
> > +unsigned int *n4frag_overlap, bool *ipf_v6_enabled,
> > +unsigned int *min_v6_frag_size, unsigned int *n6frag_accepted,
> > +unsigned int *n6frag_completed_sent,
> > +unsigned int *n6frag_expired_sent, unsigned int *n6frag_too_small,
> > +unsigned int *n6frag_overlap)
> > +{
> > ...
> > diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
> > index 542478d..35094f0 100644
> > --- a/lib/dpif-netdev.c
> > +++ b/lib/dpif-netdev.c
> > @@ -5892,6 +5892,38 @@ dpif_netdev_ipf_set_nfrag_max(struct dpif *dpif
> OVS_UNUSED,
> > return ipf_set_nfrag_max(max_frags);
> > }
> >
> > +static int
> > +dpif_netdev_ipf_get_status(struct dpif *dpif OVS_UNUSED,
> > +bool *ipf_v4_enabled, unsigned int *min_v4_frag_size,
> > +unsigned int *nfrag_max, unsigned int *nfrag,
> > +unsigned int *n4frag_accepted, unsigned int *n4frag_completed_sent,
> > +unsigned int *n4frag_expired_sent, unsigned int *n4frag_too_small,
> > +unsigned int *n4frag_overlap, bool *ipf_v6_enabled,
> > +unsigned int *min_v6_frag_size, unsigned int *n6frag_accepted,
> > +unsigned int *n6frag_completed_sent, unsigned int
> *n6frag_expired_sent,
> > +unsigned int *n6frag_too_small, unsigned int *n6frag_overlap)
> > +{
>
> The arguments to these functions are pretty long.  How about using
> 'ipf_status' instead?  Most of the callers are in files that already
> including "ipf.h".
>



It is intentional information hiding from the higher layers.
Also, the API is common across dpif providers.




>
> As before, some of my previous comments will apply to this patch, too.
>


got it; thanks



>
> Thanks,
>
> --Justin
>
>
>
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [patch v6 08/10] ipf: Add command to get fragmentation handling status.

[Justin Pettit]

> On Apr 8, 2018, at 7:54 PM, Darrell Ball  wrote:
> 
> diff --git a/lib/ct-dpif.c b/lib/ct-dpif.c
> index 81f9d92..60c8986 100644
> --- a/lib/ct-dpif.c
> +++ b/lib/ct-dpif.c
> @@ -188,6 +188,27 @@ ct_dpif_ipf_set_nfrag_max(struct dpif *dpif, uint32_t 
> max_frags)
> : EOPNOTSUPP);
> }
> 
> +int ct_dpif_ipf_get_status(struct dpif *dpif, bool *ipf_v4_enabled,
> +unsigned int *min_v4_frag_size, unsigned int *nfrag_max,
> +unsigned int *nfrag, unsigned int *n4frag_accepted,
> +unsigned int *n4frag_completed_sent,
> +unsigned int *n4frag_expired_sent, unsigned int *n4frag_too_small,
> +unsigned int *n4frag_overlap, bool *ipf_v6_enabled,
> +unsigned int *min_v6_frag_size, unsigned int *n6frag_accepted,
> +unsigned int *n6frag_completed_sent,
> +unsigned int *n6frag_expired_sent, unsigned int *n6frag_too_small,
> +unsigned int *n6frag_overlap)
> +{
> ...
> diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
> index 542478d..35094f0 100644
> --- a/lib/dpif-netdev.c
> +++ b/lib/dpif-netdev.c
> @@ -5892,6 +5892,38 @@ dpif_netdev_ipf_set_nfrag_max(struct dpif *dpif 
> OVS_UNUSED,
> return ipf_set_nfrag_max(max_frags);
> }
> 
> +static int
> +dpif_netdev_ipf_get_status(struct dpif *dpif OVS_UNUSED,
> +bool *ipf_v4_enabled, unsigned int *min_v4_frag_size,
> +unsigned int *nfrag_max, unsigned int *nfrag,
> +unsigned int *n4frag_accepted, unsigned int *n4frag_completed_sent,
> +unsigned int *n4frag_expired_sent, unsigned int *n4frag_too_small,
> +unsigned int *n4frag_overlap, bool *ipf_v6_enabled,
> +unsigned int *min_v6_frag_size, unsigned int *n6frag_accepted,
> +unsigned int *n6frag_completed_sent, unsigned int *n6frag_expired_sent,
> +unsigned int *n6frag_too_small, unsigned int *n6frag_overlap)
> +{

The arguments to these functions are pretty long.  How about using 'ipf_status' 
instead?  Most of the callers are in files that already including "ipf.h".

As before, some of my previous comments will apply to this patch, too.

Thanks,

--Justin


___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] [patch v6 08/10] ipf: Add command to get fragmentation handling status.

[Darrell Ball]

A new command "ovs-appctl dpctl/ipf-get-status" is added
for userspace datapath conntrack fragmentation support.
The command shows the configuration status as well as
fragment counters.

Signed-off-by: Darrell Ball 
---
 NEWS|  2 ++
 lib/ct-dpif.c   | 21 +++
 lib/ct-dpif.h   |  6 +
 lib/dpctl.c | 75 -
 lib/dpctl.man   |  5 
 lib/dpif-netdev.c   | 33 +++
 lib/dpif-netlink.c  |  1 +
 lib/dpif-provider.h |  7 +
 lib/ipf.c   | 26 +++
 lib/ipf.h   |  3 +++
 10 files changed, 178 insertions(+), 1 deletion(-)

diff --git a/NEWS b/NEWS
index 4ee43cf..d617335 100644
--- a/NEWS
+++ b/NEWS
@@ -18,6 +18,8 @@ Post-v2.9.0
datapath conntrack fragmentation support.
  * New "ovs-appctl dpctl/ipf-set-maxfrags" command for userspace datapath
conntrack fragmentation support.
+ * New "ovs-appctl dpctl/ipf-get-status" command for userspace datapath
+   conntrack fragmentation support.
- ovs-vsctl: New commands "add-bond-iface" and "del-bond-iface".
- OpenFlow:
  * OFPT_ROLE_STATUS is now available in OpenFlow 1.3.
diff --git a/lib/ct-dpif.c b/lib/ct-dpif.c
index 81f9d92..60c8986 100644
--- a/lib/ct-dpif.c
+++ b/lib/ct-dpif.c
@@ -188,6 +188,27 @@ ct_dpif_ipf_set_nfrag_max(struct dpif *dpif, uint32_t 
max_frags)
 : EOPNOTSUPP);
 }
 
+int ct_dpif_ipf_get_status(struct dpif *dpif, bool *ipf_v4_enabled,
+unsigned int *min_v4_frag_size, unsigned int *nfrag_max,
+unsigned int *nfrag, unsigned int *n4frag_accepted,
+unsigned int *n4frag_completed_sent,
+unsigned int *n4frag_expired_sent, unsigned int *n4frag_too_small,
+unsigned int *n4frag_overlap, bool *ipf_v6_enabled,
+unsigned int *min_v6_frag_size, unsigned int *n6frag_accepted,
+unsigned int *n6frag_completed_sent,
+unsigned int *n6frag_expired_sent, unsigned int *n6frag_too_small,
+unsigned int *n6frag_overlap)
+{
+return (dpif->dpif_class->ipf_get_status
+? dpif->dpif_class->ipf_get_status(dpif, ipf_v4_enabled,
+min_v4_frag_size, nfrag_max, nfrag, n4frag_accepted,
+n4frag_completed_sent, n4frag_expired_sent, n4frag_too_small,
+n4frag_overlap, ipf_v6_enabled, min_v6_frag_size, n6frag_accepted,
+n6frag_completed_sent, n6frag_expired_sent, n6frag_too_small,
+n6frag_overlap)
+: EOPNOTSUPP);
+}
+
 void
 ct_dpif_entry_uninit(struct ct_dpif_entry *entry)
 {
diff --git a/lib/ct-dpif.h b/lib/ct-dpif.h
index 4ce4dd4..8a24128 100644
--- a/lib/ct-dpif.h
+++ b/lib/ct-dpif.h
@@ -203,6 +203,12 @@ int ct_dpif_get_nconns(struct dpif *dpif, uint32_t 
*nconns);
 int ct_dpif_ipf_change_enabled(struct dpif *, bool, bool);
 int ct_dpif_ipf_set_min_frag(struct dpif *, bool, uint32_t);
 int ct_dpif_ipf_set_nfrag_max(struct dpif *, uint32_t);
+int ct_dpif_ipf_get_status(struct dpif *dpif, bool *, unsigned int *,
+   unsigned int *, unsigned int *, unsigned int *,
+   unsigned int *, unsigned int *, unsigned int *,
+   unsigned int *, bool *, unsigned int *,
+   unsigned int *, unsigned int *, unsigned int *,
+   unsigned int *, unsigned int *);
 void ct_dpif_entry_uninit(struct ct_dpif_entry *);
 void ct_dpif_format_entry(const struct ct_dpif_entry *, struct ds *,
   bool verbose, bool print_stats);
diff --git a/lib/dpctl.c b/lib/dpctl.c
index f25c6fc..84064cd 100644
--- a/lib/dpctl.c
+++ b/lib/dpctl.c
@@ -35,7 +35,6 @@
 #include "dpif.h"
 #include "openvswitch/dynamic-string.h"
 #include "flow.h"
-#include "ipf.h"
 #include "openvswitch/match.h"
 #include "netdev.h"
 #include "netdev-dpdk.h"
@@ -1853,6 +1852,78 @@ dpctl_ct_ipf_set_nfrag_max(int argc, const char *argv[],
 return error;
 }
 
+static int
+dpctl_ct_ipf_get_status(int argc, const char *argv[],
+struct dpctl_params *dpctl_p)
+{
+struct dpif *dpif;
+int error = dpctl_ct_open_dp(argc, argv, dpctl_p, , 2);
+if (!error) {
+bool ipf_v4_enabled;
+unsigned int min_v4_frag_size;
+unsigned int nfrag_max;
+unsigned int nfrag;
+unsigned int n4frag_accepted;
+unsigned int n4frag_completed_sent;
+unsigned int n4frag_expired_sent;
+unsigned int n4frag_too_small;
+unsigned int n4frag_overlap;
+unsigned int min_v6_frag_size;
+bool ipf_v6_enabled;
+unsigned int n6frag_accepted;
+unsigned int n6frag_completed_sent;
+unsigned int n6frag_expired_sent;
+unsigned int n6frag_too_small;
+unsigned int n6frag_overlap;
+error = ct_dpif_ipf_get_status(dpif, _v4_enabled,
+_v4_frag_size, _max, , _accepted,
+_completed_sent, _expired_sent, _too_small,
+_overlap,