| Message ID | 20180320204632.16781-1-blp@ovn.org |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [ovs-dev,1/4] match: Add 'tun_md' member to struct minimatch. | expand |
Thanks, looks good to me. Reviewed-by: Yifeng Sun <pkusunyifeng@gmail.com> On Tue, Mar 20, 2018 at 1:46 PM, Ben Pfaff <blp@ovn.org> wrote: > struct match has had a 'tun_md' member for a long time, but struct > minimatch has never had one. This doesn't matter for the purposes for > which minimatch is currently used, but it means that a minimatch is not > completely substitutable for a match and therefore blocks some new uses. > This patch adds the member. > > Signed-off-by: Ben Pfaff <blp@ovn.org> > --- > include/openvswitch/match.h | 1 + > include/openvswitch/tun-metadata.h | 5 +++++ > lib/match.c | 7 ++++++- > lib/tun-metadata.c | 17 +++++++++++++++++ > 4 files changed, 29 insertions(+), 1 deletion(-) > > diff --git a/include/openvswitch/match.h b/include/openvswitch/match.h > index 61a67de2c413..e5cf1a0d7c14 100644 > --- a/include/openvswitch/match.h > +++ b/include/openvswitch/match.h > @@ -242,6 +242,7 @@ struct minimatch { > }; > struct miniflow *flows[2]; > }; > + struct tun_metadata_allocation *tun_md; > }; > > void minimatch_init(struct minimatch *, const struct match *); > diff --git a/include/openvswitch/tun-metadata.h b/include/openvswitch/tun- > metadata.h > index 935c5c495027..dc0312ecb724 100644 > --- a/include/openvswitch/tun-metadata.h > +++ b/include/openvswitch/tun-metadata.h > @@ -101,6 +101,11 @@ struct tun_metadata_allocation { > bool valid; /* Set to true after any allocation > occurs. */ > }; > > +struct tun_metadata_allocation *tun_metadata_allocation_clone( > + const struct tun_metadata_allocation *); > +void tun_metadata_allocation_copy(struct tun_metadata_allocation *, > + const struct tun_metadata_allocation *); > + > #ifdef __cplusplus > } > #endif > diff --git a/lib/match.c b/lib/match.c > index 97a5282997b0..3eab0fd5dec9 100644 > --- a/lib/match.c > +++ b/lib/match.c > @@ -1663,6 +1663,8 @@ minimatch_init(struct minimatch *dst, const struct > match *src) > miniflow_alloc(dst->flows, 2, &tmp); > miniflow_init(dst->flow, &src->flow); > minimask_init(dst->mask, &src->wc); > + > + dst->tun_md = tun_metadata_allocation_clone(&src->tun_md); > } > > /* Initializes 'dst' as a copy of 'src'. The caller must eventually free > 'dst' > @@ -1677,6 +1679,7 @@ minimatch_clone(struct minimatch *dst, const struct > minimatch *src) > miniflow_get_values(src->flow), data_size); > memcpy(miniflow_values(&dst->mask->masks), > miniflow_get_values(&src->mask->masks), data_size); > + dst->tun_md = tun_metadata_allocation_clone(src->tun_md); > } > > /* Initializes 'dst' with the data in 'src', destroying 'src'. The > caller must > @@ -1686,6 +1689,7 @@ minimatch_move(struct minimatch *dst, struct > minimatch *src) > { > dst->flow = src->flow; > dst->mask = src->mask; > + dst->tun_md = src->tun_md; > } > > /* Frees any memory owned by 'match'. Does not free the storage in which > @@ -1694,6 +1698,7 @@ void > minimatch_destroy(struct minimatch *match) > { > free(match->flow); > + free(match->tun_md); > } > > /* Initializes 'dst' as a copy of 'src'. */ > @@ -1702,7 +1707,7 @@ minimatch_expand(const struct minimatch *src, struct > match *dst) > { > miniflow_expand(src->flow, &dst->flow); > minimask_expand(src->mask, &dst->wc); > - memset(&dst->tun_md, 0, sizeof dst->tun_md); > + tun_metadata_allocation_copy(&dst->tun_md, src->tun_md); > } > > /* Returns true if 'a' and 'b' match the same packets, false otherwise. > */ > diff --git a/lib/tun-metadata.c b/lib/tun-metadata.c > index c0d9180e020e..f8a0e19524e9 100644 > --- a/lib/tun-metadata.c > +++ b/lib/tun-metadata.c > @@ -924,3 +924,20 @@ tun_metadata_match_format(struct ds *s, const struct > match *match) > ds_put_char(s, ','); > } > } > + > +struct tun_metadata_allocation * > +tun_metadata_allocation_clone(const struct tun_metadata_allocation *src) > +{ > + return src && src->valid ? xmemdup(src, sizeof *src) : NULL; > +} > + > +void > +tun_metadata_allocation_copy(struct tun_metadata_allocation *dst, > + const struct tun_metadata_allocation *src) > +{ > + if (src && src->valid) { > + *dst = *src; > + } else { > + memset(dst, 0, sizeof *dst); > + } > +} > -- > 2.16.1 > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev >
On 20 March 2018 at 13:46, Ben Pfaff <blp@ovn.org> wrote: > struct match has had a 'tun_md' member for a long time, but struct > minimatch has never had one. This doesn't matter for the purposes for > which minimatch is currently used, but it means that a minimatch is not > completely substitutable for a match and therefore blocks some new uses. > This patch adds the member. > > Signed-off-by: Ben Pfaff <blp@ovn.org> > --- > include/openvswitch/match.h | 1 + > include/openvswitch/tun-metadata.h | 5 +++++ > lib/match.c | 7 ++++++- > lib/tun-metadata.c | 17 +++++++++++++++++ > 4 files changed, 29 insertions(+), 1 deletion(-) > > diff --git a/include/openvswitch/match.h b/include/openvswitch/match.h > index 61a67de2c413..e5cf1a0d7c14 100644 > --- a/include/openvswitch/match.h > +++ b/include/openvswitch/match.h > @@ -242,6 +242,7 @@ struct minimatch { > }; > struct miniflow *flows[2]; > }; > + struct tun_metadata_allocation *tun_md; > }; > > void minimatch_init(struct minimatch *, const struct match *); > diff --git a/include/openvswitch/tun-metadata.h > b/include/openvswitch/tun-metadata.h > index 935c5c495027..dc0312ecb724 100644 > --- a/include/openvswitch/tun-metadata.h > +++ b/include/openvswitch/tun-metadata.h > @@ -101,6 +101,11 @@ struct tun_metadata_allocation { > bool valid; /* Set to true after any allocation > occurs. */ > }; > > +struct tun_metadata_allocation *tun_metadata_allocation_clone( > + const struct tun_metadata_allocation *); > +void tun_metadata_allocation_copy(struct tun_metadata_allocation *, > + const struct tun_metadata_allocation *); > + > #ifdef __cplusplus > } > #endif > diff --git a/lib/match.c b/lib/match.c > index 97a5282997b0..3eab0fd5dec9 100644 > --- a/lib/match.c > +++ b/lib/match.c > @@ -1663,6 +1663,8 @@ minimatch_init(struct minimatch *dst, const struct > match *src) > miniflow_alloc(dst->flows, 2, &tmp); > miniflow_init(dst->flow, &src->flow); > minimask_init(dst->mask, &src->wc); > + > + dst->tun_md = tun_metadata_allocation_clone(&src->tun_md); > } > > /* Initializes 'dst' as a copy of 'src'. The caller must eventually free > 'dst' > @@ -1677,6 +1679,7 @@ minimatch_clone(struct minimatch *dst, const struct > minimatch *src) > miniflow_get_values(src->flow), data_size); > memcpy(miniflow_values(&dst->mask->masks), > miniflow_get_values(&src->mask->masks), data_size); > + dst->tun_md = tun_metadata_allocation_clone(src->tun_md); > } > > /* Initializes 'dst' with the data in 'src', destroying 'src'. The > caller must > @@ -1686,6 +1689,7 @@ minimatch_move(struct minimatch *dst, struct > minimatch *src) > { > dst->flow = src->flow; > dst->mask = src->mask; > + dst->tun_md = src->tun_md; > } > > /* Frees any memory owned by 'match'. Does not free the storage in which > @@ -1694,6 +1698,7 @@ void > minimatch_destroy(struct minimatch *match) > { > free(match->flow); > + free(match->tun_md); > } > > /* Initializes 'dst' as a copy of 'src'. */ > @@ -1702,7 +1707,7 @@ minimatch_expand(const struct minimatch *src, struct > match *dst) > { > miniflow_expand(src->flow, &dst->flow); > minimask_expand(src->mask, &dst->wc); > - memset(&dst->tun_md, 0, sizeof dst->tun_md); > + tun_metadata_allocation_copy(&dst->tun_md, src->tun_md); > } > > /* Returns true if 'a' and 'b' match the same packets, false otherwise. > */ > diff --git a/lib/tun-metadata.c b/lib/tun-metadata.c > index c0d9180e020e..f8a0e19524e9 100644 > --- a/lib/tun-metadata.c > +++ b/lib/tun-metadata.c > @@ -924,3 +924,20 @@ tun_metadata_match_format(struct ds *s, const struct > match *match) > ds_put_char(s, ','); > } > } > + > +struct tun_metadata_allocation * > +tun_metadata_allocation_clone(const struct tun_metadata_allocation *src) > +{ > + return src && src->valid ? xmemdup(src, sizeof *src) : NULL; > +} > + > +void > +tun_metadata_allocation_copy(struct tun_metadata_allocation *dst, > + const struct tun_metadata_allocation *src) > +{ > + if (src && src->valid) { > + *dst = *src; > + } else { > + memset(dst, 0, sizeof *dst); > + } > +} > -- > 2.16.1 > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > LGTM. I wonder if this and the related patches are worth considering for a backport. Thanks! Reviewed-by: Armando Migliaccio <armamig@gmail.com>
On Tue, Mar 20, 2018 at 01:46:29PM -0700, Ben Pfaff wrote: > struct match has had a 'tun_md' member for a long time, but struct > minimatch has never had one. This doesn't matter for the purposes for > which minimatch is currently used, but it means that a minimatch is not > completely substitutable for a match and therefore blocks some new uses. > This patch adds the member. > > Signed-off-by: Ben Pfaff <blp@ovn.org> Thanks to everyone who reviewed and tested these patches. I applied them to master.
diff --git a/include/openvswitch/match.h b/include/openvswitch/match.h index 61a67de2c413..e5cf1a0d7c14 100644 --- a/include/openvswitch/match.h +++ b/include/openvswitch/match.h @@ -242,6 +242,7 @@ struct minimatch { }; struct miniflow *flows[2]; }; + struct tun_metadata_allocation *tun_md; }; void minimatch_init(struct minimatch *, const struct match *); diff --git a/include/openvswitch/tun-metadata.h b/include/openvswitch/tun-metadata.h index 935c5c495027..dc0312ecb724 100644 --- a/include/openvswitch/tun-metadata.h +++ b/include/openvswitch/tun-metadata.h @@ -101,6 +101,11 @@ struct tun_metadata_allocation { bool valid; /* Set to true after any allocation occurs. */ }; +struct tun_metadata_allocation *tun_metadata_allocation_clone( + const struct tun_metadata_allocation *); +void tun_metadata_allocation_copy(struct tun_metadata_allocation *, + const struct tun_metadata_allocation *); + #ifdef __cplusplus } #endif diff --git a/lib/match.c b/lib/match.c index 97a5282997b0..3eab0fd5dec9 100644 --- a/lib/match.c +++ b/lib/match.c @@ -1663,6 +1663,8 @@ minimatch_init(struct minimatch *dst, const struct match *src) miniflow_alloc(dst->flows, 2, &tmp); miniflow_init(dst->flow, &src->flow); minimask_init(dst->mask, &src->wc); + + dst->tun_md = tun_metadata_allocation_clone(&src->tun_md); } /* Initializes 'dst' as a copy of 'src'. The caller must eventually free 'dst' @@ -1677,6 +1679,7 @@ minimatch_clone(struct minimatch *dst, const struct minimatch *src) miniflow_get_values(src->flow), data_size); memcpy(miniflow_values(&dst->mask->masks), miniflow_get_values(&src->mask->masks), data_size); + dst->tun_md = tun_metadata_allocation_clone(src->tun_md); } /* Initializes 'dst' with the data in 'src', destroying 'src'. The caller must @@ -1686,6 +1689,7 @@ minimatch_move(struct minimatch *dst, struct minimatch *src) { dst->flow = src->flow; dst->mask = src->mask; + dst->tun_md = src->tun_md; } /* Frees any memory owned by 'match'. Does not free the storage in which @@ -1694,6 +1698,7 @@ void minimatch_destroy(struct minimatch *match) { free(match->flow); + free(match->tun_md); } /* Initializes 'dst' as a copy of 'src'. */ @@ -1702,7 +1707,7 @@ minimatch_expand(const struct minimatch *src, struct match *dst) { miniflow_expand(src->flow, &dst->flow); minimask_expand(src->mask, &dst->wc); - memset(&dst->tun_md, 0, sizeof dst->tun_md); + tun_metadata_allocation_copy(&dst->tun_md, src->tun_md); } /* Returns true if 'a' and 'b' match the same packets, false otherwise. */ diff --git a/lib/tun-metadata.c b/lib/tun-metadata.c index c0d9180e020e..f8a0e19524e9 100644 --- a/lib/tun-metadata.c +++ b/lib/tun-metadata.c @@ -924,3 +924,20 @@ tun_metadata_match_format(struct ds *s, const struct match *match) ds_put_char(s, ','); } } + +struct tun_metadata_allocation * +tun_metadata_allocation_clone(const struct tun_metadata_allocation *src) +{ + return src && src->valid ? xmemdup(src, sizeof *src) : NULL; +} + +void +tun_metadata_allocation_copy(struct tun_metadata_allocation *dst, + const struct tun_metadata_allocation *src) +{ + if (src && src->valid) { + *dst = *src; + } else { + memset(dst, 0, sizeof *dst); + } +}
struct match has had a 'tun_md' member for a long time, but struct minimatch has never had one. This doesn't matter for the purposes for which minimatch is currently used, but it means that a minimatch is not completely substitutable for a match and therefore blocks some new uses. This patch adds the member. Signed-off-by: Ben Pfaff <blp@ovn.org> --- include/openvswitch/match.h | 1 + include/openvswitch/tun-metadata.h | 5 +++++ lib/match.c | 7 ++++++- lib/tun-metadata.c | 17 +++++++++++++++++ 4 files changed, 29 insertions(+), 1 deletion(-)