diff mbox series

[ovs-dev,v7,1/2] ofproto-dpif-mirror: Reduce number of function parameters.

Message ID 20240226211649.553204-1-mkp@redhat.com
State Changes Requested
Headers show
Series [ovs-dev,v7,1/2] ofproto-dpif-mirror: Reduce number of function parameters. | expand

Checks

Context Check Description
ovsrobot/apply-robot success apply and check: success
ovsrobot/github-robot-_Build_and_Test success github build: passed
ovsrobot/intel-ovs-compilation success test: success

Commit Message

Mike Pattrick Feb. 26, 2024, 9:16 p.m. UTC
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 <mkp@redhat.com>
Acked-by: Simon Horman <horms@ovn.org>
Acked-by: Eelco Chaudron <echaudro@redhat.com>
---
 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(-)

Comments

Eelco Chaudron March 1, 2024, 2:57 p.m. UTC | #1
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 <mkp@redhat.com>
> Acked-by: Simon Horman <horms@ovn.org>
> Acked-by: Eelco Chaudron <echaudro@redhat.com>

Re-acking as there are quite some changes since rev5.


Acked-by: Eelco Chaudron <echaudro@redhat.com>
Ilya Maximets March 1, 2024, 7:38 p.m. UTC | #2
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 <mkp@redhat.com>
> Acked-by: Simon Horman <horms@ovn.org>
> Acked-by: Eelco Chaudron <echaudro@redhat.com>
> ---
>  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 *, &mirror->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, &srcs_map);
> -    mbundle_lookup_multiple(mbridge, dsts, n_dsts, &dsts_map);
> +    mbundle_lookup_multiple(mbridge, mb->srcs, mb->n_srcs, &srcs_map);
> +    mbundle_lookup_multiple(mbridge, mb->dsts, mb->n_dsts, &dsts_map);
>  
>      /* If the configuration has not changed, do nothing. */
>      if (hmapx_equals(&srcs_map, &mirror->srcs)
>          && hmapx_equals(&dsts_map, &mirror->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(&srcs_map);
>          hmapx_destroy(&dsts_map);
> @@ -275,15 +279,15 @@ mirror_set(struct mbridge *mbridge, void *aux, const char *name,
>      hmapx_swap(&dsts_map, &mirror->dsts);
>      hmapx_destroy(&dsts_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(&mirror->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
> -mirror_get(struct mbridge *mbridge, int index, const unsigned long **vlans,
> -           mirror_mask_t *dup_mirrors, struct ofbundle **out,
> -           int *snaplen, int *out_vlan)
> +mirror_get(struct mbridge *mbridge, int index,
> +           struct mirror_config *mc)
>  {
>      struct mirror *mirror;
>  
> -    if (!mbridge) {
> +    if (!mc || !mbridge) {
>          return false;
>      }
>  
> @@ -433,11 +436,11 @@ mirror_get(struct mbridge *mbridge, int index, const unsigned long **vlans,
>      /* Assume 'mirror' is RCU protected, i.e., it will not be freed until this
>       * thread quiesces. */
>  
> -    *vlans = ovsrcu_get(unsigned long *, &mirror->vlans);
> -    *dup_mirrors = mirror->dup_mirrors;
> -    *out = mirror->out ? mirror->out->ofbundle : NULL;
> -    *out_vlan = mirror->out_vlan;
> -    *snaplen = mirror->snaplen;
> +    mc->vlans = ovsrcu_get(unsigned long *, &mirror->vlans);
> +    mc->dup_mirrors = mirror->dup_mirrors;
> +    mc->out_bundle = mirror->out ? mirror->out->ofbundle : NULL;
> +    mc->out_vlan = mirror->out_vlan;
> +    mc->snaplen = mirror->snaplen;
>      return true;
>  }
>  
> diff --git a/ofproto/ofproto-dpif-mirror.h b/ofproto/ofproto-dpif-mirror.h
> index eed63ec4a..a983b3876 100644
> --- a/ofproto/ofproto-dpif-mirror.h
> +++ b/ofproto/ofproto-dpif-mirror.h
> @@ -22,9 +22,38 @@
>  #define MAX_MIRRORS 32
>  typedef uint32_t mirror_mask_t;
>  
> +struct ofproto_mirror_settings;
>  struct ofproto_dpif;
>  struct ofbundle;
>  
> +struct mirror_bundles {
> +    struct ofbundle **srcs;
> +    size_t n_srcs;
> +
> +    struct ofbundle **dsts;
> +    size_t n_dsts;
> +
> +    struct ofbundle *out_bundle;
> +};
> +
> +struct mirror_config {
> +    /* A bitmap of mirrors that duplicate the current mirror. */
> +    mirror_mask_t dup_mirrors;
> +
> +    /* VLANs of packets to select for mirroring. */
> +    unsigned long *vlans;           /* vlan_bitmap, NULL selects all VLANs. */
> +
> +    /* Output (mutually exclusive). */
> +    struct ofbundle *out_bundle;    /* A registered ofbundle handle or NULL. */
> +    uint16_t out_vlan;              /* Output VLAN, not used if out_bundle is
> +                                       set. */
> +
> +    /* Max size of a mirrored packet in bytes, if set to zero then no
> +     * truncation will occur.  */
> +    uint16_t snaplen;
> +};
> +
> +
>  /* The following functions are used by handler threads without any locking,
>   * assuming RCU protection. */
>  
> @@ -38,9 +67,8 @@ mirror_mask_t mirror_bundle_dst(struct mbridge *, struct ofbundle *);
>  
>  void mirror_update_stats(struct mbridge*, mirror_mask_t, uint64_t packets,
>                           uint64_t bytes);
> -bool mirror_get(struct mbridge *, int index, const unsigned long **vlans,
> -                mirror_mask_t *dup_mirrors, struct ofbundle **out,
> -                int *snaplen, int *out_vlan);
> +bool mirror_get(struct mbridge *mbridge, int index,
> +                struct mirror_config *mc);

Not a full review, but don't add names for obviously typed arguments
in prototypes.

>  
>  /* The remaining functions are assumed to be called by the main thread only. */
>  
> @@ -50,11 +78,9 @@ bool mbridge_need_revalidate(struct mbridge *);
>  void mbridge_register_bundle(struct mbridge *, struct ofbundle *);
>  void mbridge_unregister_bundle(struct mbridge *, struct ofbundle *);
>  
> -int mirror_set(struct 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);
> +int mirror_set(struct mbridge *mbridge, void *aux,
> +               const struct ofproto_mirror_settings *ms,
> +               const struct mirror_bundles *mb);

Same here.

Best regards, Ilya Maximets.
diff mbox series

Patch

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 *, &mirror->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, &srcs_map);
-    mbundle_lookup_multiple(mbridge, dsts, n_dsts, &dsts_map);
+    mbundle_lookup_multiple(mbridge, mb->srcs, mb->n_srcs, &srcs_map);
+    mbundle_lookup_multiple(mbridge, mb->dsts, mb->n_dsts, &dsts_map);
 
     /* If the configuration has not changed, do nothing. */
     if (hmapx_equals(&srcs_map, &mirror->srcs)
         && hmapx_equals(&dsts_map, &mirror->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(&srcs_map);
         hmapx_destroy(&dsts_map);
@@ -275,15 +279,15 @@  mirror_set(struct mbridge *mbridge, void *aux, const char *name,
     hmapx_swap(&dsts_map, &mirror->dsts);
     hmapx_destroy(&dsts_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(&mirror->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
-mirror_get(struct mbridge *mbridge, int index, const unsigned long **vlans,
-           mirror_mask_t *dup_mirrors, struct ofbundle **out,
-           int *snaplen, int *out_vlan)
+mirror_get(struct mbridge *mbridge, int index,
+           struct mirror_config *mc)
 {
     struct mirror *mirror;
 
-    if (!mbridge) {
+    if (!mc || !mbridge) {
         return false;
     }
 
@@ -433,11 +436,11 @@  mirror_get(struct mbridge *mbridge, int index, const unsigned long **vlans,
     /* Assume 'mirror' is RCU protected, i.e., it will not be freed until this
      * thread quiesces. */
 
-    *vlans = ovsrcu_get(unsigned long *, &mirror->vlans);
-    *dup_mirrors = mirror->dup_mirrors;
-    *out = mirror->out ? mirror->out->ofbundle : NULL;
-    *out_vlan = mirror->out_vlan;
-    *snaplen = mirror->snaplen;
+    mc->vlans = ovsrcu_get(unsigned long *, &mirror->vlans);
+    mc->dup_mirrors = mirror->dup_mirrors;
+    mc->out_bundle = mirror->out ? mirror->out->ofbundle : NULL;
+    mc->out_vlan = mirror->out_vlan;
+    mc->snaplen = mirror->snaplen;
     return true;
 }
 
diff --git a/ofproto/ofproto-dpif-mirror.h b/ofproto/ofproto-dpif-mirror.h
index eed63ec4a..a983b3876 100644
--- a/ofproto/ofproto-dpif-mirror.h
+++ b/ofproto/ofproto-dpif-mirror.h
@@ -22,9 +22,38 @@ 
 #define MAX_MIRRORS 32
 typedef uint32_t mirror_mask_t;
 
+struct ofproto_mirror_settings;
 struct ofproto_dpif;
 struct ofbundle;
 
+struct mirror_bundles {
+    struct ofbundle **srcs;
+    size_t n_srcs;
+
+    struct ofbundle **dsts;
+    size_t n_dsts;
+
+    struct ofbundle *out_bundle;
+};
+
+struct mirror_config {
+    /* A bitmap of mirrors that duplicate the current mirror. */
+    mirror_mask_t dup_mirrors;
+
+    /* VLANs of packets to select for mirroring. */
+    unsigned long *vlans;           /* vlan_bitmap, NULL selects all VLANs. */
+
+    /* Output (mutually exclusive). */
+    struct ofbundle *out_bundle;    /* A registered ofbundle handle or NULL. */
+    uint16_t out_vlan;              /* Output VLAN, not used if out_bundle is
+                                       set. */
+
+    /* Max size of a mirrored packet in bytes, if set to zero then no
+     * truncation will occur.  */
+    uint16_t snaplen;
+};
+
+
 /* The following functions are used by handler threads without any locking,
  * assuming RCU protection. */
 
@@ -38,9 +67,8 @@  mirror_mask_t mirror_bundle_dst(struct mbridge *, struct ofbundle *);
 
 void mirror_update_stats(struct mbridge*, mirror_mask_t, uint64_t packets,
                          uint64_t bytes);
-bool mirror_get(struct mbridge *, int index, const unsigned long **vlans,
-                mirror_mask_t *dup_mirrors, struct ofbundle **out,
-                int *snaplen, int *out_vlan);
+bool mirror_get(struct mbridge *mbridge, int index,
+                struct mirror_config *mc);
 
 /* The remaining functions are assumed to be called by the main thread only. */
 
@@ -50,11 +78,9 @@  bool mbridge_need_revalidate(struct mbridge *);
 void mbridge_register_bundle(struct mbridge *, struct ofbundle *);
 void mbridge_unregister_bundle(struct mbridge *, struct ofbundle *);
 
-int mirror_set(struct 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);
+int mirror_set(struct mbridge *mbridge, void *aux,
+               const struct ofproto_mirror_settings *ms,
+               const struct mirror_bundles *mb);
 void mirror_destroy(struct mbridge *, void *aux);
 int mirror_get_stats(struct mbridge *, void *aux, uint64_t *packets,
                      uint64_t *bytes);
diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
index 89f183182..aa2a514f9 100644
--- a/ofproto/ofproto-dpif-xlate.c
+++ b/ofproto/ofproto-dpif-xlate.c
@@ -2279,16 +2279,12 @@  mirror_packet(struct xlate_ctx *ctx, struct xbundle *xbundle,
      * 'used_mirrors', as long as some candidates remain.  */
     mirror_mask_t used_mirrors = 0;
     while (mirrors) {
-        const unsigned long *vlans;
-        mirror_mask_t dup_mirrors;
-        struct ofbundle *out;
-        int out_vlan;
-        int snaplen;
+        struct mirror_config mc;
 
         /* Get the details of the mirror represented by the rightmost 1-bit. */
-        if (OVS_UNLIKELY(!mirror_get(xbridge->mbridge, raw_ctz(mirrors),
-                                     &vlans, &dup_mirrors,
-                                     &out, &snaplen, &out_vlan))) {
+        if (OVS_UNLIKELY(!mirror_get(xbridge->mbridge,
+                                     raw_ctz(mirrors),
+                                     &mc))) {
             /* The mirror got reconfigured before we got to read it's
              * configuration. */
             mirrors = zero_rightmost_1bit(mirrors);
@@ -2298,10 +2294,10 @@  mirror_packet(struct xlate_ctx *ctx, struct xbundle *xbundle,
 
         /* If this mirror selects on the basis of VLAN, and it does not select
          * 'vlan', then discard this mirror and go on to the next one. */
-        if (vlans) {
+        if (mc.vlans) {
             ctx->wc->masks.vlans[0].tci |= htons(VLAN_CFI | VLAN_VID_MASK);
         }
-        if (vlans && !bitmap_is_set(vlans, xvlan.v[0].vid)) {
+        if (mc.vlans && !bitmap_is_set(mc.vlans, xvlan.v[0].vid)) {
             mirrors = zero_rightmost_1bit(mirrors);
             continue;
         }
@@ -2313,21 +2309,22 @@  mirror_packet(struct xlate_ctx *ctx, struct xbundle *xbundle,
          * destination, so that we don't mirror to them again.  This must be
          * done now to ensure that output_normal(), below, doesn't recursively
          * output to the same mirrors. */
-        ctx->mirrors |= dup_mirrors;
-        ctx->mirror_snaplen = snaplen;
+        ctx->mirrors |= mc.dup_mirrors;
+        ctx->mirror_snaplen = mc.snaplen;
 
         /* Send the packet to the mirror. */
-        if (out) {
-            struct xbundle *out_xbundle = xbundle_lookup(ctx->xcfg, out);
+        if (mc.out_bundle) {
+            struct xbundle *out_xbundle = xbundle_lookup(ctx->xcfg,
+                                                         mc.out_bundle);
             if (out_xbundle) {
                 output_normal(ctx, out_xbundle, &xvlan);
             }
-        } else if (xvlan.v[0].vid != out_vlan
+        } else if (xvlan.v[0].vid != mc.out_vlan
                    && !eth_addr_is_reserved(ctx->xin->flow.dl_dst)) {
             struct xbundle *xb;
             uint16_t old_vid = xvlan.v[0].vid;
 
-            xvlan.v[0].vid = out_vlan;
+            xvlan.v[0].vid = mc.out_vlan;
             LIST_FOR_EACH (xb, list_node, &xbridge->xbundles) {
                 if (xbundle_includes_vlan(xb, &xvlan)
                     && !xbundle_mirror_out(xbridge, xb)) {
diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
index f59d69c4d..4f31f5ebb 100644
--- a/ofproto/ofproto-dpif.c
+++ b/ofproto/ofproto-dpif.c
@@ -3645,7 +3645,7 @@  mirror_set__(struct ofproto *ofproto_, void *aux,
              const struct ofproto_mirror_settings *s)
 {
     struct ofproto_dpif *ofproto = ofproto_dpif_cast(ofproto_);
-    struct ofbundle **srcs, **dsts;
+    struct mirror_bundles mb;
     int error;
     size_t i;
 
@@ -3654,23 +3654,24 @@  mirror_set__(struct ofproto *ofproto_, void *aux,
         return 0;
     }
 
-    srcs = xmalloc(s->n_srcs * sizeof *srcs);
-    dsts = xmalloc(s->n_dsts * sizeof *dsts);
+    mb.srcs = xmalloc(s->n_srcs * sizeof *mb.srcs);
+    mb.dsts = xmalloc(s->n_dsts * sizeof *mb.dsts);
 
     for (i = 0; i < s->n_srcs; i++) {
-        srcs[i] = bundle_lookup(ofproto, s->srcs[i]);
+        mb.srcs[i] = bundle_lookup(ofproto, s->srcs[i]);
     }
 
     for (i = 0; i < s->n_dsts; i++) {
-        dsts[i] = bundle_lookup(ofproto, s->dsts[i]);
+        mb.dsts[i] = bundle_lookup(ofproto, s->dsts[i]);
     }
 
-    error = mirror_set(ofproto->mbridge, aux, s->name, srcs, s->n_srcs, dsts,
-                       s->n_dsts, s->src_vlans,
-                       bundle_lookup(ofproto, s->out_bundle),
-                       s->snaplen, s->out_vlan);
-    free(srcs);
-    free(dsts);
+    mb.n_srcs = s->n_srcs;
+    mb.n_dsts = s->n_dsts;
+    mb.out_bundle = bundle_lookup(ofproto, s->out_bundle);
+
+    error = mirror_set(ofproto->mbridge, aux, s, &mb);
+    free(mb.srcs);
+    free(mb.dsts);
     return error;
 }