[ovs-dev] [PATCH 1/2] ofp-errors: Fix bugs in treatment of OpenFlow experimenter errors.
Jarno Rajahalme
jrajahalme at nicira.com
Thu Sep 4 16:51:00 UTC 2014
Acked-by: Jarno Rajahalme <jrajahalme at nicira.com>
On Aug 21, 2014, at 10:45 AM, Ben Pfaff <blp at nicira.com> wrote:
> OpenFlow 1.2 and later have "experimenter errors". The OVS implementation
> was buggy in a few ways. First, a bug in extract-ofp-errors prevented
> OF1.2+ experimenter errors from being properly decoded. Second,
> OF1.2+ experimenter errors have only a type, not a code, whereas all other
> types of errors (standard errors, OF1.0/1.1 Nicira extension errors) have
> both, but extract-ofp-errors didn't properly enforce that.
>
> This commit fixes both problems and improves existing tests to verify that
> encoding and decoding of experimenter errors now works properly.
>
> This commit also fixes the definition of OFPBIC_DUP_INST. It claimed to
> have an OF1.1 experimenter error value although OF1.1 didn't have
> experimenter errors. This commit changes it to use a Nicira extension
> error in OF1.1 instead.
>
> Signed-off-by: Ben Pfaff <blp at nicira.com>
> ---
> build-aux/extract-ofp-errors | 30 +++++++++++++++++++++------
> lib/ofp-errors.h | 2 +-
> tests/ofp-errors.at | 47 ++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 72 insertions(+), 7 deletions(-)
>
> diff --git a/build-aux/extract-ofp-errors b/build-aux/extract-ofp-errors
> index 89dd31a..16bfbc7 100755
> --- a/build-aux/extract-ofp-errors
> +++ b/build-aux/extract-ofp-errors
> @@ -300,14 +300,34 @@ def extract_ofp_errors(fn):
> % (version1_name, version2_name,
> version2_name, version1_name))
>
> - if vendor == vendor_map['NX']:
> + if vendor == vendor_map['OF']:
> + # All standard OpenFlow errors have a type and a code.
> + if code is None:
> + fatal("%s: %s domain requires code" % (dst, vendor_name))
> + elif vendor == vendor_map['NX']:
> + # Before OpenFlow 1.2, OVS used a Nicira extension to
> + # define errors that included a type and a code.
> + #
> + # In OpenFlow 1.2 and later, Nicira extension errors
> + # are defined using the OpenFlow experimenter error
> + # mechanism that includes a type but not a code.
> + if v1 < version_map['1.2'] or v2 < version_map['1.2']:
> + if code is None:
> + fatal("%s: NX1.0 and NX1.1 domains require code"
> + % (dst, vendor_name))
> if v1 >= version_map['1.2'] or v2 >= version_map['1.2']:
> if code is not None:
> fatal("%s: NX1.2+ domains do not have codes" % dst)
> - code = 0
> - elif vendor != vendor_map['OF']:
> + else:
> + # Experimenter extension error for OF1.2+ only.
> + if v1 < version_map['1.2']:
> + fatal("%s: %s domain not supported before OF1.2"
> + % (dst, vendor_name))
> if code is not None:
> - fatal("%s: %s domains do not have codes" % vendor_name)
> + fatal("%s: %s domains do not have codes"
> + % (dst, vendor_name))
> + if code is None:
> + code = 0
>
> for version in range(v1, v2 + 1):
> domain[version].setdefault(vendor, {})
> @@ -376,8 +396,6 @@ static enum ofperr
> if enum not in map:
> continue
> vendor, type_, code = map[enum]
> - if code is None:
> - continue
> value = (vendor << 32) | (type_ << 16) | code
> if value in found:
> continue
> diff --git a/lib/ofp-errors.h b/lib/ofp-errors.h
> index 643fa72..0d5bb3a 100644
> --- a/lib/ofp-errors.h
> +++ b/lib/ofp-errors.h
> @@ -262,7 +262,7 @@ enum ofperr {
> /* OF1.2+(3,8). Permissions error. */
> OFPERR_OFPBIC_EPERM,
>
> - /* ONF1.1-1.3(2600), OF1.4+(3,9). Duplicate instruction. */
> + /* NX1.1(3,256), ONF1.2-1.3(2600), OF1.4+(3,9). Duplicate instruction. */
> OFPERR_OFPBIC_DUP_INST,
>
> /* ## --------------- ## */
> diff --git a/tests/ofp-errors.at b/tests/ofp-errors.at
> index f8c5cc4..daf7ddd 100644
> --- a/tests/ofp-errors.at
> +++ b/tests/ofp-errors.at
> @@ -163,10 +163,29 @@ AT_CHECK([ovs-ofctl ofp-print '0301001855555555 ffff0004 00002320 01020008111111
> OFPT_ERROR (OF1.2) (xid=0x55555555): NXBRC_MUST_BE_ZERO
> OFPT_ECHO_REQUEST (xid=0x11111111): 0 bytes of payload
> ])
> +
> +AT_CHECK([ovs-ofctl ofp-print '0301001812345678 ffff0a28 4f4e4600 0300000812345678'], [0], [dnl
> +OFPT_ERROR (OF1.2) (xid=0x12345678): OFPBIC_DUP_INST
> +OFPT_HELLO (OF1.2) (xid=0x12345678):
> + version bitmap: 0x01, 0x02, 0x03
> +])
> +AT_CHECK([ovs-ofctl ofp-print '0401001812345678 ffff0a28 4f4e4600 0400000812345678'], [0], [dnl
> +OFPT_ERROR (OF1.3) (xid=0x12345678): OFPBIC_DUP_INST
> +OFPT_HELLO (OF1.3) (xid=0x12345678):
> + version bitmap: 0x01, 0x02, 0x03, 0x04
> +])
> +AT_CHECK([ovs-ofctl ofp-print '0501001412345678 00030009 0500000812345678'], [0], [dnl
> +OFPT_ERROR (OF1.4) (xid=0x12345678): OFPBIC_DUP_INST
> +OFPT_HELLO (OF1.4) (xid=0x12345678):
> + version bitmap: 0x01, 0x02, 0x03, 0x04, 0x05
> +])
> AT_CLEANUP
>
> AT_SETUP([encoding experimenter errors])
> AT_KEYWORDS([ofp-print ofp-errors])
> +# Demonstrate that a Nicira extension error gets encoded correctly
> +# using the Nicira error extension format in OF1.0 and OF1.1, and
> +# correctly using the standard experimenter format in OF1.2.
> AT_CHECK(
> [ovs-ofctl encode-error-reply NXBRC_MUST_BE_ZERO 0100000812345678], [0], [dnl
> 00000000 01 01 00 1c 12 34 56 78-b0 c2 00 00 00 00 23 20 @&t@
> @@ -182,4 +201,32 @@ AT_CHECK(
> 00000000 03 01 00 18 12 34 56 78-ff ff 00 04 00 00 23 20 @&t@
> 00000010 03 00 00 08 12 34 56 78-
> ])
> +
> +# Check that OFPERR_OFPBIC_DUP_INST is:
> +# - not encodable in OF1.0 (OF1.0 doesn't have instructions, after all).
> +# - encoded as a Nicira extension in OF1.1.
> +# - encoded as an ONF extension in OF1.2 and OF1.3.
> +# - encoded in the standard form in OF1.4.
> +AT_CHECK(
> + [ovs-ofctl '-vPATTERN:console:%c|%p|%m' encode-error-reply OFPBIC_DUP_INST 0100000812345678], [0], [dnl
> +00000000 01 01 00 1c 12 34 56 78-b0 c2 00 00 00 00 23 20 @&t@
> +00000010 00 01 02 09 01 00 00 08-12 34 56 78 @&t@
> +], [ofp_errors|ERR|cannot encode OFPBIC_DUP_INST for OpenFlow 1.0
> +])
> +AT_CHECK([ovs-ofctl encode-error-reply OFPBIC_DUP_INST 0200000812345678], [0],
> +[00000000 02 01 00 1c 12 34 56 78-b0 c2 00 00 00 00 23 20 @&t@
> +00000010 00 03 01 00 02 00 00 08-12 34 56 78 @&t@
> +])
> +AT_CHECK([ovs-ofctl encode-error-reply OFPBIC_DUP_INST 0300000812345678], [0],
> +[00000000 03 01 00 18 12 34 56 78-ff ff 0a 28 4f 4e 46 00 @&t@
> +00000010 03 00 00 08 12 34 56 78-
> +])
> +AT_CHECK([ovs-ofctl encode-error-reply OFPBIC_DUP_INST 0400000812345678], [0],
> +[00000000 04 01 00 18 12 34 56 78-ff ff 0a 28 4f 4e 46 00 @&t@
> +00000010 04 00 00 08 12 34 56 78-
> +])
> +AT_CHECK([ovs-ofctl encode-error-reply OFPBIC_DUP_INST 0500000812345678], [0],
> +[00000000 05 01 00 14 12 34 56 78-00 03 00 09 05 00 00 08 @&t@
> +00000010 12 34 56 78 @&t@
> +])
> AT_CLEANUP
> --
> 1.7.10.4
>
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev
More information about the dev
mailing list