> On Apr 8, 2018, at 7:53 PM, Darrell Ball <[email protected]> wrote:
>
> diff --git a/lib/ct-dpif.c b/lib/ct-dpif.c
> index 5fa3a97..32d55c1 100644
> --- a/lib/ct-dpif.c
> +++ b/lib/ct-dpif.c
> @@ -164,6 +164,14 @@ ct_dpif_get_nconns(struct dpif *dpif, uint32_t *nconns)
> : EOPNOTSUPP);
> }
>
> +int
> +ct_dpif_ipf_change_enabled(struct dpif *dpif, bool v6, bool enable)
> +{
> + return (dpif->dpif_class->ipf_change_enabled
> + ? dpif->dpif_class->ipf_change_enabled(dpif, v6, enable)
> + : EOPNOTSUPP);
> +}
The command is "ipf-set-enabled", and I think that would be a good name for the
function, too. It's a bit shorter, and it follows the pattern of other dpif
functions.
> diff --git a/lib/dpctl.c b/lib/dpctl.c
> index 47f4182..9fc0151 100644
> --- a/lib/dpctl.c
> +++ b/lib/dpctl.c
> @@ -35,6 +35,7 @@
>
> ...
> +static int
> +dpctl_ct_ipf_change_enabled(int argc, const char *argv[],
> + struct dpctl_params *dpctl_p)
> +{
> + struct dpif *dpif;
> + int error = dpctl_ct_open_dp(argc, argv, dpctl_p, &dpif, 4);
> + if (!error) {
> + char v4_or_v6[3] = {0};
> + if (ovs_scan(argv[argc - 2], "%2s", v4_or_v6) &&
> + (!strncmp(v4_or_v6, "v4", 2) || !strncmp(v4_or_v6, "v6", 2))) {
> + uint32_t enabled;
> + if (ovs_scan(argv[argc - 1], "%"SCNu32, &enabled)) {
> + error = ct_dpif_ipf_change_enabled(
> + dpif, !strncmp(v4_or_v6, "v6", 2), enabled);
> + if (!error) {
> + dpctl_print(dpctl_p,
> + "changing fragmentation enabled successful");
> + } else {
> + dpctl_error(dpctl_p, error,
> + "changing fragmentation enabled failed");
I think these two messages would be confusing if someone were attempted to
disable fragmentation. How about putting some code in that prints whether they
were enabling or disabling fragmentation reassembly. For example, "enabling
fragmentation reassembly successful" or "disabling fragmentation reassembly
successful".
> + }
> + } else {
> + error = EINVAL;
> + dpctl_error(
> + dpctl_p, error,
> + "parameter missing: 0 for disabled or 1 for enabled");
> + }
> + } else {
> + error = EINVAL;
> + dpctl_error(dpctl_p, error,
> + "parameter missing: v4 for ipv4 or v6 for ipv6");
I would quote "v4" and "v6" to make it clear exactly what arguments were
required.
> diff --git a/lib/dpctl.man b/lib/dpctl.man
> index 9e9d2dc..9bf489c 100644
> --- a/lib/dpctl.man
> +++ b/lib/dpctl.man
> @@ -268,3 +268,14 @@ Only supported for userspace datapath.
> \*(DX\fBct\-get\-nconns\fR [\fIdp\fR]
> Read the current number of connection tracker connections.
> Only supported for userspace datapath.
> +.
> +.TP
> +\*(DX\fBipf\-set\-enabled\fR [\fIdp\fR] [\fIv4 or v6\fR] \fBenable\fR
I think you want to us "\fB" for "v4" and "v6", since they're constants, and
you should drop the brackets since the argument isn't optional. Also, the "or"
shouldn't be included, and I think using "|" is more common. I think you want
something like the following: "\fBv4\fR | \fBv6\fR".
It seems like the argument is 0 or 1, and not "enable". How about exposing
separate commands for enable and disable? I think it would be clearer, and the
internal implementation could remain the same passing bool arguments.
> +Enables or disables fragmentation handling for the userspace datapath
> +connection tracker. Either v4 or v6 must be specified. Both v4 and v6
These references to "v4" and "v6" should be bolded.
> +are enabled by default. When fragmentation handling is enabled, the
I'd mention that they're default "on" at the end.
> +rules for handling fragments before entering conntrack should not
> +differentiate between first and other fragments. Although, this would
> +logically already be true anyways, it is mentioned for clarity. If there
I would drop the "Although" sentence, since it doesn't add anything.
> diff --git a/lib/dpif-provider.h b/lib/dpif-provider.h
> index 62b3598..08e0944 100644
> --- a/lib/dpif-provider.h
> +++ b/lib/dpif-provider.h
> @@ -444,6 +444,8 @@ struct dpif_class {
> /* Get number of connections tracked. */
> int (*ct_get_nconns)(struct dpif *, uint32_t *nconns);
>
> + /* IP Fragmentation. */
> + int (*ipf_change_enabled)(struct dpif *, bool, bool);
> /* Meters */
I would put a blank line before "/* Meters */" to break them into sections.
> diff --git a/lib/ipf.c b/lib/ipf.c
> index 3837c60..54f27d2 100644
> --- a/lib/ipf.c
> +++ b/lib/ipf.c
> @@ -1236,3 +1236,18 @@ ipf_destroy(void)
> ipf_lock_unlock(&ipf_lock);
> ipf_lock_destroy(&ipf_lock);
> }
> +
> +int
> +ipf_change_enabled(bool v6, bool enable)
All the other arguments in "lib/ipf.c" are 'v4'. How about using it here, too,
instead of 'v6'? (And propagating it up through the call stack.)
> +{
> + if ((v6 != true && v6 != false) ||
> + (enable != true && enable != false)) {
> + return 1;
> + }
If they're bools, how would they be anything be true or false?
> diff --git a/lib/ipf.h b/lib/ipf.h
> index 5861e96..0b45de9 100644
> --- a/lib/ipf.h
> +++ b/lib/ipf.h
> @@ -60,4 +60,7 @@ ipf_init(void);
> void
> ipf_destroy(void);
>
> +int
> +ipf_change_enabled(bool v6, bool enable);
According to the style guide, the return type and function name should be on
the same line.
Thanks,
--Justin
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
