Re: [ovs-dev] [PATCH v7 1/2] ofproto-dpif-mirror: Reduce number of function parameters.
On 2/26/24 22:16, Mike Pattrick wrote:
> Previously the mirror_set() and mirror_get() functions took a large
> number of parameters, which was inefficient and difficult to read and
> extend. This patch moves most of the parameters into a struct.
>
> Signed-off-by: Mike Pattrick
> Acked-by: Simon Horman
> Acked-by: Eelco Chaudron
> ---
> ofproto/ofproto-dpif-mirror.c | 61 ++-
> ofproto/ofproto-dpif-mirror.h | 42 +++-
> ofproto/ofproto-dpif-xlate.c | 29 -
> ofproto/ofproto-dpif.c| 23 ++---
> 4 files changed, 91 insertions(+), 64 deletions(-)
>
> diff --git a/ofproto/ofproto-dpif-mirror.c b/ofproto/ofproto-dpif-mirror.c
> index 343b75f0e..a84c843b3 100644
> --- a/ofproto/ofproto-dpif-mirror.c
> +++ b/ofproto/ofproto-dpif-mirror.c
> @@ -207,19 +207,23 @@ mirror_bundle_dst(struct mbridge *mbridge, struct
> ofbundle *ofbundle)
> }
>
> int
> -mirror_set(struct mbridge *mbridge, void *aux, const char *name,
> - struct ofbundle **srcs, size_t n_srcs,
> - struct ofbundle **dsts, size_t n_dsts,
> - unsigned long *src_vlans, struct ofbundle *out_bundle,
> - uint16_t snaplen,
> - uint16_t out_vlan)
> +mirror_set(struct mbridge *mbridge, void *aux,
> + const struct ofproto_mirror_settings *ms,
> + const struct mirror_bundles *mb)
> +
> {
> struct mbundle *mbundle, *out;
> mirror_mask_t mirror_bit;
> struct mirror *mirror;
> struct hmapx srcs_map; /* Contains "struct ofbundle *"s. */
> struct hmapx dsts_map; /* Contains "struct ofbundle *"s. */
> +uint16_t out_vlan;
> +
> +if (!ms || !mbridge) {
> +return EINVAL;
> +}
>
> +out_vlan = ms->out_vlan;
> mirror = mirror_lookup(mbridge, aux);
> if (!mirror) {
> int idx;
> @@ -227,7 +231,7 @@ mirror_set(struct mbridge *mbridge, void *aux, const char
> *name,
> idx = mirror_scan(mbridge);
> if (idx < 0) {
> VLOG_WARN("maximum of %d port mirrors reached, cannot create %s",
> - MAX_MIRRORS, name);
> + MAX_MIRRORS, ms->name);
> return EFBIG;
> }
>
> @@ -242,8 +246,8 @@ mirror_set(struct mbridge *mbridge, void *aux, const char
> *name,
> unsigned long *vlans = ovsrcu_get(unsigned long *, >vlans);
>
> /* Get the new configuration. */
> -if (out_bundle) {
> -out = mbundle_lookup(mbridge, out_bundle);
> +if (mb->out_bundle) {
> +out = mbundle_lookup(mbridge, mb->out_bundle);
> if (!out) {
> mirror_destroy(mbridge, mirror->aux);
> return EINVAL;
> @@ -252,16 +256,16 @@ mirror_set(struct mbridge *mbridge, void *aux, const
> char *name,
> } else {
> out = NULL;
> }
> -mbundle_lookup_multiple(mbridge, srcs, n_srcs, _map);
> -mbundle_lookup_multiple(mbridge, dsts, n_dsts, _map);
> +mbundle_lookup_multiple(mbridge, mb->srcs, mb->n_srcs, _map);
> +mbundle_lookup_multiple(mbridge, mb->dsts, mb->n_dsts, _map);
>
> /* If the configuration has not changed, do nothing. */
> if (hmapx_equals(_map, >srcs)
> && hmapx_equals(_map, >dsts)
> -&& vlan_bitmap_equal(vlans, src_vlans)
> +&& vlan_bitmap_equal(vlans, ms->src_vlans)
> && mirror->out == out
> && mirror->out_vlan == out_vlan
> -&& mirror->snaplen == snaplen)
> +&& mirror->snaplen == ms->snaplen)
> {
> hmapx_destroy(_map);
> hmapx_destroy(_map);
> @@ -275,15 +279,15 @@ mirror_set(struct mbridge *mbridge, void *aux, const
> char *name,
> hmapx_swap(_map, >dsts);
> hmapx_destroy(_map);
>
> -if (vlans || src_vlans) {
> +if (vlans || ms->src_vlans) {
> ovsrcu_postpone(free, vlans);
> -vlans = vlan_bitmap_clone(src_vlans);
> +vlans = vlan_bitmap_clone(ms->src_vlans);
> ovsrcu_set(>vlans, vlans);
> }
>
> mirror->out = out;
> mirror->out_vlan = out_vlan;
> -mirror->snaplen = snaplen;
> +mirror->snaplen = ms->snaplen;
>
> /* Update mbundles. */
> mirror_bit = MIRROR_MASK_C(1) << mirror->idx;
> @@ -406,23 +410,22 @@ mirror_update_stats(struct mbridge *mbridge,
> mirror_mask_t mirrors,
> /* Retrieves the mirror numbered 'index' in 'mbridge'. Returns true if such
> a
> * mirror exists, false otherwise.
> *
> - * If successful, '*vlans' receives the mirror's VLAN membership information,
> + * If successful 'mc->vlans' receives the mirror's VLAN membership
> information,
> * either a null pointer if the mirror includes all VLANs or a 4096-bit
> bitmap
> * in which a 1-bit indicates that the mirror includes a particular VLAN,
> - * '*dup_mirrors' receives a bitmap of mirrors whose output duplicates mirror
> - * 'index', '*out' receives the output ofbundle (if any), and '*out_vlan'
> - * receives the output
Re: [ovs-dev] [PATCH v7 1/2] ofproto-dpif-mirror: Reduce number of function parameters.
On 26 Feb 2024, at 22:16, Mike Pattrick wrote: > Previously the mirror_set() and mirror_get() functions took a large > number of parameters, which was inefficient and difficult to read and > extend. This patch moves most of the parameters into a struct. > > Signed-off-by: Mike Pattrick > Acked-by: Simon Horman > Acked-by: Eelco Chaudron Re-acking as there are quite some changes since rev5. Acked-by: Eelco Chaudron ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH v7 1/2] ofproto-dpif-mirror: Reduce number of function parameters.
Previously the mirror_set() and mirror_get() functions took a large
number of parameters, which was inefficient and difficult to read and
extend. This patch moves most of the parameters into a struct.
Signed-off-by: Mike Pattrick
Acked-by: Simon Horman
Acked-by: Eelco Chaudron
---
ofproto/ofproto-dpif-mirror.c | 61 ++-
ofproto/ofproto-dpif-mirror.h | 42 +++-
ofproto/ofproto-dpif-xlate.c | 29 -
ofproto/ofproto-dpif.c| 23 ++---
4 files changed, 91 insertions(+), 64 deletions(-)
diff --git a/ofproto/ofproto-dpif-mirror.c b/ofproto/ofproto-dpif-mirror.c
index 343b75f0e..a84c843b3 100644
--- a/ofproto/ofproto-dpif-mirror.c
+++ b/ofproto/ofproto-dpif-mirror.c
@@ -207,19 +207,23 @@ mirror_bundle_dst(struct mbridge *mbridge, struct
ofbundle *ofbundle)
}
int
-mirror_set(struct mbridge *mbridge, void *aux, const char *name,
- struct ofbundle **srcs, size_t n_srcs,
- struct ofbundle **dsts, size_t n_dsts,
- unsigned long *src_vlans, struct ofbundle *out_bundle,
- uint16_t snaplen,
- uint16_t out_vlan)
+mirror_set(struct mbridge *mbridge, void *aux,
+ const struct ofproto_mirror_settings *ms,
+ const struct mirror_bundles *mb)
+
{
struct mbundle *mbundle, *out;
mirror_mask_t mirror_bit;
struct mirror *mirror;
struct hmapx srcs_map; /* Contains "struct ofbundle *"s. */
struct hmapx dsts_map; /* Contains "struct ofbundle *"s. */
+uint16_t out_vlan;
+
+if (!ms || !mbridge) {
+return EINVAL;
+}
+out_vlan = ms->out_vlan;
mirror = mirror_lookup(mbridge, aux);
if (!mirror) {
int idx;
@@ -227,7 +231,7 @@ mirror_set(struct mbridge *mbridge, void *aux, const char
*name,
idx = mirror_scan(mbridge);
if (idx < 0) {
VLOG_WARN("maximum of %d port mirrors reached, cannot create %s",
- MAX_MIRRORS, name);
+ MAX_MIRRORS, ms->name);
return EFBIG;
}
@@ -242,8 +246,8 @@ mirror_set(struct mbridge *mbridge, void *aux, const char
*name,
unsigned long *vlans = ovsrcu_get(unsigned long *, >vlans);
/* Get the new configuration. */
-if (out_bundle) {
-out = mbundle_lookup(mbridge, out_bundle);
+if (mb->out_bundle) {
+out = mbundle_lookup(mbridge, mb->out_bundle);
if (!out) {
mirror_destroy(mbridge, mirror->aux);
return EINVAL;
@@ -252,16 +256,16 @@ mirror_set(struct mbridge *mbridge, void *aux, const char
*name,
} else {
out = NULL;
}
-mbundle_lookup_multiple(mbridge, srcs, n_srcs, _map);
-mbundle_lookup_multiple(mbridge, dsts, n_dsts, _map);
+mbundle_lookup_multiple(mbridge, mb->srcs, mb->n_srcs, _map);
+mbundle_lookup_multiple(mbridge, mb->dsts, mb->n_dsts, _map);
/* If the configuration has not changed, do nothing. */
if (hmapx_equals(_map, >srcs)
&& hmapx_equals(_map, >dsts)
-&& vlan_bitmap_equal(vlans, src_vlans)
+&& vlan_bitmap_equal(vlans, ms->src_vlans)
&& mirror->out == out
&& mirror->out_vlan == out_vlan
-&& mirror->snaplen == snaplen)
+&& mirror->snaplen == ms->snaplen)
{
hmapx_destroy(_map);
hmapx_destroy(_map);
@@ -275,15 +279,15 @@ mirror_set(struct mbridge *mbridge, void *aux, const char
*name,
hmapx_swap(_map, >dsts);
hmapx_destroy(_map);
-if (vlans || src_vlans) {
+if (vlans || ms->src_vlans) {
ovsrcu_postpone(free, vlans);
-vlans = vlan_bitmap_clone(src_vlans);
+vlans = vlan_bitmap_clone(ms->src_vlans);
ovsrcu_set(>vlans, vlans);
}
mirror->out = out;
mirror->out_vlan = out_vlan;
-mirror->snaplen = snaplen;
+mirror->snaplen = ms->snaplen;
/* Update mbundles. */
mirror_bit = MIRROR_MASK_C(1) << mirror->idx;
@@ -406,23 +410,22 @@ mirror_update_stats(struct mbridge *mbridge,
mirror_mask_t mirrors,
/* Retrieves the mirror numbered 'index' in 'mbridge'. Returns true if such a
* mirror exists, false otherwise.
*
- * If successful, '*vlans' receives the mirror's VLAN membership information,
+ * If successful 'mc->vlans' receives the mirror's VLAN membership information,
* either a null pointer if the mirror includes all VLANs or a 4096-bit bitmap
* in which a 1-bit indicates that the mirror includes a particular VLAN,
- * '*dup_mirrors' receives a bitmap of mirrors whose output duplicates mirror
- * 'index', '*out' receives the output ofbundle (if any), and '*out_vlan'
- * receives the output VLAN (if any).
+ * 'mc->dup_mirrors' receives a bitmap of mirrors whose output duplicates
+ * mirror 'index', 'mc->out' receives the output ofbundle (if any),
+ * and 'mc->out_vlan' receives the output VLAN (if any).
*
* Everything returned here is assumed to be RCU protected.
*/
bool
