On Thu, May 24, 2018 at 10:20 AM, Ben Pfaff <[email protected]> wrote:
> On Thu, May 24, 2018 at 05:36:10AM -0700, William Tu wrote:
>> On Wed, May 23, 2018 at 7:13 PM, Darrell Ball <[email protected]> wrote:
>> > The struct erspan_metadata is updated to replace the 'version'
>> > placeholder with the erspan base hdr. Also, the erspan
>> > index is defined explicitly as ovs_16aligned_be32 to mirror
>> > its encoding.
>> > Changes to odp_util result from updating the erspan index
>> > type.
>> >
>> > CC: William Tu <[email protected]>
>> > Fixes: 068794b43f0e ("erspan: Add flow-based erspan options")
>> > Signed-off-by: Darrell Ball <[email protected]>
>> > ---
>> > lib/odp-util.c | 36 ++++++++++++++++++++----------------
>> > lib/packets.h | 6 +++---
>> > 2 files changed, 23 insertions(+), 19 deletions(-)
>> >
>> > diff --git a/lib/odp-util.c b/lib/odp-util.c
>> > index 105ac80..767281f 100644
>> > --- a/lib/odp-util.c
>> > +++ b/lib/odp-util.c
>> > @@ -2786,9 +2786,9 @@ odp_tun_key_from_attr__(const struct nlattr *attr,
>> > bool is_mask,
>> >
>> > memcpy(&opts, nl_attr_get(a), attr_len);
>> >
>> > - tun->erspan_ver = opts.version;
>> > + tun->erspan_ver = opts.bh.ver;
>> > if (tun->erspan_ver == 1) {
>> > - tun->erspan_idx = ntohl(opts.u.index);
>> > + tun->erspan_idx =
>> > ntohl(get_16aligned_be32(&opts.u.index));
>> > } else if (tun->erspan_ver == 2) {
>> > tun->erspan_dir = opts.u.md2.dir;
>> > tun->erspan_hwid = get_hwid(&opts.u.md2);
>> > @@ -2890,10 +2890,11 @@ tun_key_to_attr(struct ofpbuf *a, const struct
>> > flow_tnl *tun_key,
>> > !strcmp(tnl_type, "ip6erspan")) &&
>> > (tun_key->erspan_ver == 1 || tun_key->erspan_ver == 2)) {
>> > struct erspan_metadata opts;
>> > + memset(&opts, 0, sizeof opts);
>> >
>> > - opts.version = tun_key->erspan_ver;
>> > - if (opts.version == 1) {
>> > - opts.u.index = htonl(tun_key->erspan_idx);
>> > + opts.bh.ver = tun_key->erspan_ver;
>> > + if (opts.bh.ver == 1) {
>> > + put_16aligned_be32(&opts.u.index, htonl(tun_key->erspan_idx));
>> > } else {
>> > opts.u.md2.dir = tun_key->erspan_dir;
>> > set_hwid(&opts.u.md2, tun_key->erspan_hwid);
>> > @@ -3368,22 +3369,23 @@ format_odp_tun_erspan_opt(const struct nlattr
>> > *attr,
>> > opts = nl_attr_get(attr);
>> > mask = mask_attr ? nl_attr_get(mask_attr) : NULL;
>> >
>> > - ver = (uint8_t)opts->version;
>> > + ver = opts->bh.ver;
>> > if (mask) {
>> > - ver_ma = (uint8_t)mask->version;
>> > + ver_ma = mask->bh.ver;
>> > }
>> >
>> > format_u8u(ds, "ver", ver, mask ? &ver_ma : NULL, verbose);
>> >
>> > - if (opts->version == 1) {
>> > + if (opts->bh.ver == 1) {
>> > if (mask) {
>> > ds_put_format(ds, "idx=%#"PRIx32"/%#"PRIx32",",
>> > - ntohl(opts->u.index),
>> > - ntohl(mask->u.index));
>> > + ntohl(get_16aligned_be32(&opts->u.index)),
>> > + ntohl(get_16aligned_be32(&mask->u.index)));
>> > } else {
>> > - ds_put_format(ds, "idx=%#"PRIx32",", ntohl(opts->u.index));
>> > + ds_put_format(ds, "idx=%#"PRIx32",",
>> > + ntohl(get_16aligned_be32(&opts->u.index)));
>> > }
>> > - } else if (opts->version == 2) {
>> > + } else if (opts->bh.ver == 2) {
>> > dir = opts->u.md2.dir;
>> > hwid = opts->u.md2.hwid;
>> > if (mask) {
>> > @@ -4859,10 +4861,11 @@ scan_erspan_metadata(const char *s,
>> >
>> > if (!strncmp(s, ")", 1)) {
>> > s += 1;
>> > - key->version = ver;
>> > - key->u.index = htonl(idx);
>> > + memset(&key->bh, 0, sizeof key->bh);
>> > + key->bh.ver = ver;
>> > + put_16aligned_be32(&key->u.index, htonl(idx));
>> > if (mask) {
>> > - mask->u.index = htonl(idx_mask);
>> > + put_16aligned_be32(&mask->u.index, htonl(idx_mask));
>> > }
>> > }
>> > return s - s_base;
>> > @@ -4882,7 +4885,8 @@ scan_erspan_metadata(const char *s,
>> >
>> > if (!strncmp(s, ")", 1)) {
>> > s += 1;
>> > - key->version = ver;
>> > + memset(&key->bh, 0, sizeof key->bh);
>> > + key->bh.ver = ver;
>> > key->u.md2.hwid = hwid;
>> > key->u.md2.dir = dir;
>> > if (mask) {
>> > diff --git a/lib/packets.h b/lib/packets.h
>> > index 7645a9d..5c013a3 100644
>> > --- a/lib/packets.h
>> > +++ b/lib/packets.h
>> > @@ -1312,10 +1312,10 @@ struct erspan_md2 {
>> > };
>> >
>> > struct erspan_metadata {
>> > - int version;
>> > + struct erspan_base_hdr bh;
>> > union {
>> > - ovs_be32 index; /* Version 1 (type II)*/
>> > - struct erspan_md2 md2; /* Version 2 (type III) */
>> > + ovs_16aligned_be32 index; /* Version 1 (type II). */
>> > + struct erspan_md2 md2; /* Version 2 (type III). */
>> > } u;
>> > };
>> >
>> Thanks for the patch.
>>
>> We shouldn't change this header since struct erspan_metadata is exposed
>> from kernel's UAPI header. (see include/uapi/linux/erspan.h).
>> OVS kernel module expects this binary format.
>
> We often use different data structures in userspace and the kernel.
> This only changes the userspace one.
>
> It's a bad idea to use "int" for wire formats because its size can vary
> among platforms (although the Linux kernel does rely on it being 32
> bits, we don't make the same assumption in our userspace).
>
the 'int version' here is not wire format. It is to determine version 1 or 2
so that the metadata mode tunnel device knows to whether use
ovs_be32 index or,
struct erspan_md2 md2 (which are wire format)
> It's a bad idea to use "ovs_be32" in packet formats, in userspace,
> because packets aren't always aligned on 32-bit boundaries.
I see. Then I think we should do:
@@ -1314,7 +1314,7 @@ struct erspan_md2 {
struct erspan_metadata {
int version;
union {
- ovs_be32 index; /* Version 1 (type II)*/
+ ovs_16aligned_be32 index; /* Version 1 (type II)*/
struct erspan_md2 md2; /* Version 2 (type III) */
} u;
};
Thanks!
William
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
