[ovs-dev] [PATCH v2] Add build checks for portable OpenFlow structure padding and alignment.
Justin Pettit
jpettit at nicira.com
Fri Jan 22 21:50:44 UTC 2010
This is very impressive...so impressive that I'm going to take on faith that all the token parsing is correct. :-)
I think the convention in Python is to wrap everything in functions and then add an "if __name__ == '__main__'" to the bottom of the program that calls into those functions. Very minor, but I think it makes things a bit clearer.
Thanks for pulling this together so quickly. It will be a great sanity-check before pushing the OpenFlow 1.0 changes.
--Justin
On Jan 20, 2010, at 10:02 AM, Ben Pfaff wrote:
> This causes the build to fail with an error message if openflow.h contains
> a structure whose members are not aligned in a portable way.
> ---
> The first version of this patch neglected to include the struct checker
> program itself. This version also adds a check for nicira-ext.h, instead of
> just for openflow.h.
>
> .gitignore | 1 -
> build-aux/.gitignore | 4 +
> build-aux/check-structs | 263 +++++++++++++++++++++++++++++++++++++++++
> configure.ac | 5 +-
> include/openflow/.gitignore | 1 +
> include/openflow/automake.mk | 16 +++
> include/openflow/nicira-ext.h | 4 +-
> m4/openvswitch.m4 | 7 +-
> tests/atlocal.in | 6 +-
> tests/automake.mk | 1 +
> tests/check-structs.at | 41 +++++++
> tests/testsuite.at | 1 +
> 12 files changed, 343 insertions(+), 7 deletions(-)
> create mode 100644 build-aux/.gitignore
> create mode 100755 build-aux/check-structs
> create mode 100644 include/openflow/.gitignore
> create mode 100644 tests/check-structs.at
>
> diff --git a/.gitignore b/.gitignore
> index a9377fb..4e1fccb 100644
> --- a/.gitignore
> +++ b/.gitignore
> @@ -23,7 +23,6 @@
> /aclocal.m4
> /autom4te.cache
> /build-arch-stamp
> -/build-aux
> /build-indep-stamp
> /compile
> /config.guess
> diff --git a/build-aux/.gitignore b/build-aux/.gitignore
> new file mode 100644
> index 0000000..999eae2
> --- /dev/null
> +++ b/build-aux/.gitignore
> @@ -0,0 +1,4 @@
> +/compile
> +/depcomp
> +/install-sh
> +/missing
> diff --git a/build-aux/check-structs b/build-aux/check-structs
> new file mode 100755
> index 0000000..dd3ed71
> --- /dev/null
> +++ b/build-aux/check-structs
> @@ -0,0 +1,263 @@
> +#! /usr/bin/python
> +
> +import sys
> +import re
> +
> +global macros
> +macros = {}
> +
> +global anyWarnings
> +anyWarnings = False
> +
> +global types
> +types = {}
> +types['char'] = {"size": 1, "alignment": 1}
> +types['uint8_t'] = {"size": 1, "alignment": 1}
> +types['uint16_t'] = {"size": 2, "alignment": 2}
> +types['uint32_t'] = {"size": 4, "alignment": 4}
> +types['uint64_t'] = {"size": 8, "alignment": 8}
> +
> +token = None
> +line = ""
> +idRe = "[a-zA-Z_][a-zA-Z_0-9]*"
> +tokenRe = "#?" + idRe + "|[0-9]+|."
> +inComment = False
> +inDirective = False
> +def getToken():
> + global token
> + global line
> + global inComment
> + global inDirective
> + while True:
> + line = line.lstrip()
> + if line != "":
> + if line.startswith("/*"):
> + inComment = True
> + line = line[2:]
> + elif inComment:
> + commentEnd = line.find("*/")
> + if commentEnd < 0:
> + line = ""
> + else:
> + inComment = False
> + line = line[commentEnd + 2:]
> + else:
> + match = re.match(tokenRe, line)
> + token = match.group(0)
> + line = line[len(token):]
> + if token.startswith('#'):
> + inDirective = True
> + elif token in macros and not inDirective:
> + line = macros[token] + line
> + continue
> + return True
> + elif inDirective:
> + token = "$"
> + inDirective = False
> + return True
> + else:
> + global lineNumber
> + line = inputFile.readline()
> + lineNumber += 1
> + while line.endswith("\\\n"):
> + line = line[:-2] + inputFile.readline()
> + lineNumber += 1
> + if line == "":
> + if token == None:
> + fatal("unexpected end of input")
> + token = None
> + return False
> +
> +def fatal(msg):
> + sys.stderr.write("%s:%d: error at \"%s\": %s\n" % (fileName, lineNumber, token, msg))
> + sys.exit(1)
> +
> +def warn(msg):
> + global anyWarnings
> + anyWarnings = True
> + sys.stderr.write("%s:%d: warning: %s\n" % (fileName, lineNumber, msg))
> +
> +def skipDirective():
> + getToken()
> + while token != '$':
> + getToken()
> +
> +def isId(s):
> + return re.match(idRe + "$", s) != None
> +
> +def forceId():
> + if not isId(token):
> + fatal("identifier expected")
> +
> +def forceInteger():
> + if not re.match('[0-9]+$', token):
> + fatal("integer expected")
> +
> +def match(t):
> + if token == t:
> + getToken()
> + return True
> + else:
> + return False
> +
> +def forceMatch(t):
> + if not match(t):
> + fatal("%s expected" % t)
> +
> +def parseTaggedName():
> + assert token in ('struct', 'union')
> + name = token
> + getToken()
> + forceId()
> + name = "%s %s" % (name, token)
> + getToken()
> + return name
> +
> +def parseTypeName():
> + if token in ('struct', 'union'):
> + name = parseTaggedName()
> + elif isId(token):
> + name = token
> + getToken()
> + else:
> + fatal("type name expected")
> +
> + if name in types:
> + return name
> + else:
> + fatal("unknown type \"%s\"" % name)
> +
> +def parseStruct():
> + isStruct = token == 'struct'
> + structName = parseTaggedName()
> + if token == ";":
> + return
> +
> + ofs = size = 0
> + alignment = 4 # ARM has minimum 32-bit alignment
> + forceMatch('{')
> + while not match('}'):
> + typeName = parseTypeName()
> + typeSize = types[typeName]['size']
> + typeAlignment = types[typeName]['alignment']
> +
> + forceId()
> + memberName = token
> + getToken()
> +
> + if match('['):
> + if token == ']':
> + count = 0
> + else:
> + forceInteger()
> + count = int(token)
> + getToken()
> + forceMatch(']')
> + else:
> + count = 1
> +
> + nBytes = typeSize * count
> + if isStruct:
> + if ofs % typeAlignment:
> + shortage = typeAlignment - (ofs % typeAlignment)
> + warn("%s member %s is %d bytes short of %d-byte alignment"
> + % (structName, memberName, shortage, typeAlignment))
> + size += shortage
> + ofs += shortage
> + size += nBytes
> + ofs += nBytes
> + else:
> + if nBytes > size:
> + size = nBytes
> + if typeAlignment > alignment:
> + alignment = typeAlignment
> +
> + forceMatch(';')
> + if size % alignment:
> + shortage = alignment - (size % alignment)
> + if (structName == "struct ofp_packet_in" and
> + shortage == 2 and
> + memberName == 'data' and
> + count == 0):
> + # This is intentional
> + pass
> + else:
> + warn("%s needs %d bytes of tail padding" % (structName, shortage))
> + size += shortage
> + types[structName] = {"size": size, "alignment": alignment}
> +
> +if len(sys.argv) < 2:
> + sys.stderr.write("at least one non-option argument required; "
> + "use --help for help")
> + sys.exit(1)
> +
> +if '--help' in sys.argv:
> + argv0 = sys.argv[0]
> + slash = argv0.rfind('/')
> + if slash:
> + argv0 = argv0[slash + 1:]
> + print '''\
> +%(argv0)s, for checking struct and struct member alignment
> +usage: %(argv0)s HEADER [HEADER]...
> +
> +This program reads the header files specified on the command line and
> +verifies that all struct members are aligned on natural boundaries
> +without any need for the compiler to add additional padding. It also
> +verifies that each struct's size is a multiple of 32 bits (because
> +some ABIs for ARM require all structs to be a multiple of 32 bits), or
> +64 bits if the struct has any 64-bit members, again without the
> +compiler adding additional padding. Finally, it checks struct size
> +assertions using OFP_ASSERT.
> +
> +Header files are read in the order specified. #include directives are
> +not processed, so specify them in dependency order.
> +
> +This program is specialized for reading include/openflow/openflow.h
> +and include/openflow/nicira-ext.h. It will not work on arbitrary
> +header files without extensions.''' % {"argv0": argv0}
> + sys.exit(0)
> +
> +for fileName in sys.argv[1:]:
> + inputFile = open(fileName)
> + lineNumber = 0
> + while getToken():
> + if token in ("#ifdef", "#ifndef", "#include",
> + "#endif", "#elif", "#else"):
> + skipDirective()
> + elif token == "#define":
> + getToken()
> + name = token
> + if line.startswith('('):
> + skipDirective()
> + else:
> + definition = ""
> + getToken()
> + while token != '$':
> + definition += token
> + getToken()
> + macros[name] = definition
> + elif token == "enum":
> + while token != ';':
> + getToken()
> + elif token in ('struct', 'union'):
> + parseStruct()
> + elif match('OFP_ASSERT') or match('BOOST_STATIC_ASSERT'):
> + forceMatch('(')
> + forceMatch('sizeof')
> + forceMatch('(')
> + typeName = parseTypeName()
> + forceMatch(')')
> + forceMatch('=')
> + forceMatch('=')
> + forceInteger()
> + size = int(token)
> + getToken()
> + forceMatch(')')
> + if types[typeName]['size'] != size:
> + warn("%s is %d bytes long but declared as %d" % (
> + typeName, types[typeName]['size'], size))
> + else:
> + fatal("parse error")
> + inputFile.close()
> +if anyWarnings:
> + sys.exit(1)
> diff --git a/configure.ac b/configure.ac
> index 92d5ac0..2f5c87f 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -1,4 +1,4 @@
> -# Copyright (c) 2008, 2009 Nicira Networks
> +# Copyright (c) 2008, 2009, 2010 Nicira Networks
> #
> # Licensed under the Apache License, Version 2.0 (the "License");
> # you may not use this file except in compliance with the License.
> @@ -88,4 +88,7 @@ datapath/linux-2.6/Makefile
> datapath/linux-2.6/Makefile.main
> tests/atlocal])
>
> +dnl This makes sure that include/openflow gets created in the build directory.
> +AC_CONFIG_COMMANDS([include/openflow/openflow.h.stamp])
> +
> AC_OUTPUT
> diff --git a/include/openflow/.gitignore b/include/openflow/.gitignore
> new file mode 100644
> index 0000000..db736db
> --- /dev/null
> +++ b/include/openflow/.gitignore
> @@ -0,0 +1 @@
> +/openflow.h.stamp
> diff --git a/include/openflow/automake.mk b/include/openflow/automake.mk
> index d473155..8c48b1c 100644
> --- a/include/openflow/automake.mk
> +++ b/include/openflow/automake.mk
> @@ -2,3 +2,19 @@ noinst_HEADERS += \
> include/openflow/openflow-mgmt.h \
> include/openflow/nicira-ext.h \
> include/openflow/openflow.h
> +
> +if HAVE_PYTHON
> +all-local: include/openflow/openflow.h.stamp
> +include/openflow/openflow.h.stamp: \
> + include/openflow/openflow.h build-aux/check-structs
> + $(PYTHON) $(srcdir)/build-aux/check-structs $(srcdir)/include/openflow/openflow.h
> + touch $@
> +
> +all-local: include/openflow/nicira-ext.h.stamp
> +include/openflow/nicira-ext.h.stamp: include/openflow/openflow.h include/openflow/nicira-ext.h build-aux/check-structs
> + $(PYTHON) $(srcdir)/build-aux/check-structs $(srcdir)/include/openflow/openflow.h $(srcdir)/include/openflow/nicira-ext.h
> + touch $@
> +endif
> +
> +EXTRA_DIST += build-aux/check-structs
> +DISTCLEANFILES += include/openflow/openflow.h.stamp
> diff --git a/include/openflow/nicira-ext.h b/include/openflow/nicira-ext.h
> index 8434a30..c2cd1ef 100644
> --- a/include/openflow/nicira-ext.h
> +++ b/include/openflow/nicira-ext.h
> @@ -1,5 +1,5 @@
> /*
> - * Copyright (c) 2008, 2009 Nicira Networks
> + * Copyright (c) 2008, 2009, 2010 Nicira Networks
> *
> * Licensed under the Apache License, Version 2.0 (the "License");
> * you may not use this file except in compliance with the License.
> @@ -65,7 +65,7 @@ struct nicira_header {
> uint32_t vendor; /* NX_VENDOR_ID. */
> uint32_t subtype; /* One of NXT_* above. */
> };
> -OFP_ASSERT(sizeof(struct nicira_header) == sizeof(struct ofp_vendor_header) + 4);
> +OFP_ASSERT(sizeof(struct nicira_header) == 16);
>
>
> enum nx_action_subtype {
> diff --git a/m4/openvswitch.m4 b/m4/openvswitch.m4
> index 0890b9f..bdb8b19 100644
> --- a/m4/openvswitch.m4
> +++ b/m4/openvswitch.m4
> @@ -256,7 +256,12 @@ else:
> done
> done
> fi])
> + AC_SUBST([HAVE_PYTHON])
> AM_MISSING_PROG([PYTHON], [python])
> if test $ovs_cv_python != no; then
> PYTHON=$ovs_cv_python
> - fi])
> + HAVE_PYTHON=yes
> + else
> + HAVE_PYTHON=no
> + fi
> + AM_CONDITIONAL([HAVE_PYTHON], [test "$HAVE_PYTHON" = yes])])
> diff --git a/tests/atlocal.in b/tests/atlocal.in
> index d0149c1..5ff0cd2 100644
> --- a/tests/atlocal.in
> +++ b/tests/atlocal.in
> @@ -1,4 +1,6 @@
> # -*- shell-script -*-
> -PERL='@PERL@'
> -LCOV='@LCOV@'
> HAVE_OPENSSL='@HAVE_OPENSSL@'
> +HAVE_PYTHON='@HAVE_PYTHON@'
> +LCOV='@LCOV@'
> +PERL='@PERL@'
> +PYTHON='@PYTHON@'
> diff --git a/tests/automake.mk b/tests/automake.mk
> index dc677eb..044a109 100644
> --- a/tests/automake.mk
> +++ b/tests/automake.mk
> @@ -9,6 +9,7 @@ TESTSUITE_AT = \
> tests/ovsdb-macros.at \
> tests/lcov-pre.at \
> tests/library.at \
> + tests/check-structs.at \
> tests/daemon.at \
> tests/vconn.at \
> tests/dir_name.at \
> diff --git a/tests/check-structs.at b/tests/check-structs.at
> new file mode 100644
> index 0000000..52e92ec
> --- /dev/null
> +++ b/tests/check-structs.at
> @@ -0,0 +1,41 @@
> +AT_BANNER([struct alignment checker unit tests])
> +
> +m4_define([check_structs], [$top_srcdir/build-aux/check-structs])
> +m4_define([RUN_STRUCT_CHECKER],
> + [AT_SKIP_IF([test $HAVE_PYTHON = no])
> + AT_DATA([test.h], [$1
> +])
> + AT_CHECK_UNQUOTED([$PYTHON check_structs test.h], [$2], [$3], [$4])])
> +
> +AT_SETUP([check struct tail padding])
> +RUN_STRUCT_CHECKER(
> +[struct xyz {
> + uint16_t x;
> +};],
> + [1], [],
> + [test.h:3: warning: struct xyz needs 2 bytes of tail padding
> +])
> +AT_CLEANUP
> +
> +AT_SETUP([check struct internal alignment])
> +RUN_STRUCT_CHECKER(
> +[struct xyzzy {
> + uint16_t x;
> + uint32_t y;
> +};],
> + [1], [],
> + [test.h:3: warning: struct xyzzy member y is 2 bytes short of 4-byte alignment
> +])
> +AT_CLEANUP
> +
> +AT_SETUP([check struct declared size])
> +RUN_STRUCT_CHECKER(
> +[struct wibble {
> + uint64_t z;
> +};
> +OFP_ASSERT(sizeof(struct wibble) == 12);
> +],
> + [1], [],
> + [test.h:4: warning: struct wibble is 8 bytes long but declared as 12
> +])
> +AT_CLEANUP
> diff --git a/tests/testsuite.at b/tests/testsuite.at
> index 93d7e6e..0685892 100644
> --- a/tests/testsuite.at
> +++ b/tests/testsuite.at
> @@ -39,6 +39,7 @@ m4_include([tests/ovsdb-macros.at])
> m4_include([tests/lcov-pre.at])
>
> m4_include([tests/library.at])
> +m4_include([tests/check-structs.at])
> m4_include([tests/daemon.at])
> m4_include([tests/vconn.at])
> m4_include([tests/dir_name.at])
> --
> 1.6.3.3
>
>
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev_openvswitch.org
More information about the dev
mailing list