Message ID | 20190310053132.17164-2-elibr@mellanox.com |
---|---|
State | Changes Requested |
Headers | show |
Series | Do not rewrite fields with the same values as matched | expand |
Bleep bloop. Greetings Eli Britstein, I am a robot and I have tried out your patch. Thanks for your contribution. I encountered some error that I wasn't expecting. See the details below. checkpatch: WARNING: Line is 100 characters long (recommended limit is 79) #59 FILE: build-aux/extract-odp-netlink-macros-h:24: awk_cmd+=' print " {offsetof(struct '"${struct_name}"', "$3"), sizeof("$1,$2")}, \\";' WARNING: Line is 100 characters long (recommended limit is 79) #61 FILE: build-aux/extract-odp-netlink-macros-h:26: awk_cmd+=' print " {offsetof(struct '"${struct_name}"', "$2"), sizeof(" $1")}, \\";' WARNING: Line is 94 characters long (recommended limit is 79) #65 FILE: build-aux/extract-odp-netlink-macros-h:30: awk -F ";" "NR>${line_start} && NR<=${line_end}"' {print $1}' $hfile | eval "awk $awk_cmd" WARNING: Line is 83 characters long (recommended limit is 79) #71 FILE: build-aux/extract-odp-netlink-macros-h:36: echo "/* Generated automatically from <include/odp-netlink.h> -- do not modify! */" Lines checked: 117, Warnings: 4, Errors: 0 Please check this out. If you feel there has been an error, please email aconole@bytheb.org Thanks, 0-day Robot
On Sun, Mar 10, 2019 at 05:31:31AM +0000, Eli Britstein wrote: > Generate datapath ovs key fields offset and size array macros as a > pre-step for bit-wise comparing fields, with no functional change. > > Signed-off-by: Eli Britstein <elibr@mellanox.com> > Reviewed-by: Roi Dayan <roid@mellanox.com> Thanks! This introduces a script that uses /bin/bash. I don't think that we have other build or runtime dependencies on bash, so it would be better to avoid introducing one here. I would rewrite it in terms of portable Bourne shell constructs.
On 18/03/2019 23:58, Ben Pfaff wrote: > On Sun, Mar 10, 2019 at 05:31:31AM +0000, Eli Britstein wrote: >> Generate datapath ovs key fields offset and size array macros as a >> pre-step for bit-wise comparing fields, with no functional change. >> >> Signed-off-by: Eli Britstein <elibr@mellanox.com> >> Reviewed-by: Roi Dayan <roid@mellanox.com> > > Thanks! > > This introduces a script that uses /bin/bash. I don't think that we > have other build or runtime dependencies on bash, so it would be better > to avoid introducing one here. I would rewrite it in terms of portable > Bourne shell constructs. > Hi Ben, just to clarify, when you see portable Bourne shell constructs, you mean so it can work with other shells like dash on ubuntu? There are other scripts depending on bash. examples are tests in tests/system-traffic.at, the bash auto completion scripts, ovs-sim. do you still prefer us to be compatible and use /bin/sh shebang ? Thanks, Roi
> On 18/03/2019 23:58, Ben Pfaff wrote: >> On Sun, Mar 10, 2019 at 05:31:31AM +0000, Eli Britstein wrote: >>> Generate datapath ovs key fields offset and size array macros as a >>> pre-step for bit-wise comparing fields, with no functional change. >>> >>> Signed-off-by: Eli Britstein <elibr at mellanox.com> >>> Reviewed-by: Roi Dayan <roid at mellanox.com> >> >> Thanks! >> >> This introduces a script that uses /bin/bash. I don't think that we >> have other build or runtime dependencies on bash, so it would be better >> to avoid introducing one here. I would rewrite it in terms of portable >> Bourne shell constructs. >> > > Hi Ben, > > just to clarify, when you see portable Bourne shell constructs, > you mean so it can work with other shells like dash on ubuntu? > > There are other scripts depending on bash. > examples are tests in tests/system-traffic.at, the bash auto completion scripts, ovs-sim. > do you still prefer us to be compatible and use /bin/sh shebang ? > > Thanks, > Roi Hi. At least this patch set breaks the FreeBSD build: mv ovn/lib/ovn-nb-idl.ovsidl.tmp ovn/lib/ovn-nb-idl.ovsidl bash -f ./build-aux/extract-odp-netlink-macros-h include/odp-netlink.h > include/odp-netlink-macros.h /bin/sh: bash: not found gmake: *** [Makefile:8327: include/odp-netlink-macros.h] Error 127 gmake: *** Waiting for unfinished jobs.... Exit status: 2 More details here: https://cirrus-ci.com/task/5827199230803968 BTW, all the code parts you mentioned that depends on bash are not part of the build or base test system. tests/system-traffic.at depends on Linux and is not part of a base testsuite, bash auto completion scripts are obviously bash dependant and not used without bash (autocompletion tests are skipped if bash is not available), and ovs-sim is just a developer tool that intended for manual testing so not that important. But your code makes generic build process bash dependent. This causes build failure on FreeBSD and possibly on other systems without bash available. Best regards, Ilya Maximets.
On 19/03/2019 09:42, Ilya Maximets wrote: >> On 18/03/2019 23:58, Ben Pfaff wrote: >>> On Sun, Mar 10, 2019 at 05:31:31AM +0000, Eli Britstein wrote: >>>> Generate datapath ovs key fields offset and size array macros as a >>>> pre-step for bit-wise comparing fields, with no functional change. >>>> >>>> Signed-off-by: Eli Britstein <elibr at mellanox.com> >>>> Reviewed-by: Roi Dayan <roid at mellanox.com> >>> >>> Thanks! >>> >>> This introduces a script that uses /bin/bash. I don't think that we >>> have other build or runtime dependencies on bash, so it would be better >>> to avoid introducing one here. I would rewrite it in terms of portable >>> Bourne shell constructs. >>> >> >> Hi Ben, >> >> just to clarify, when you see portable Bourne shell constructs, >> you mean so it can work with other shells like dash on ubuntu? >> >> There are other scripts depending on bash. >> examples are tests in tests/system-traffic.at, the bash auto completion scripts, ovs-sim. >> do you still prefer us to be compatible and use /bin/sh shebang ? >> >> Thanks, >> Roi > > Hi. > > At least this patch set breaks the FreeBSD build: > > mv ovn/lib/ovn-nb-idl.ovsidl.tmp ovn/lib/ovn-nb-idl.ovsidl > bash -f ./build-aux/extract-odp-netlink-macros-h include/odp-netlink.h > include/odp-netlink-macros.h > /bin/sh: bash: not found > gmake: *** [Makefile:8327: include/odp-netlink-macros.h] Error 127 > gmake: *** Waiting for unfinished jobs.... > Exit status: 2 > > More details here: https://eur03.safelinks.protection.outlook.com/?url=https%3A%2F%2Fcirrus-ci.com%2Ftask%2F5827199230803968&data=02%7C01%7Croid%40mellanox.com%7Ce5343a3f2e594a860d1b08d6ac3e71d7%7Ca652971c7d2e4d9ba6a4d149256f461b%7C0%7C0%7C636885781524519796&sdata=Kx6JXs5FKFeL6TgzzB8cNE9UnC%2B%2BK7OnFpiImgB6apI%3D&reserved=0 > > BTW, all the code parts you mentioned that depends on bash are not part of the > build or base test system. tests/system-traffic.at depends on Linux and is not > part of a base testsuite, bash auto completion scripts are obviously bash dependant > and not used without bash (autocompletion tests are skipped if bash is not available), > and ovs-sim is just a developer tool that intended for manual testing so not that > important. > But your code makes generic build process bash dependent. This causes build failure > on FreeBSD and possibly on other systems without bash available. > > Best regards, Ilya Maximets. > thanks Ilya, we'll do the changes needed.
On Tue, Mar 19, 2019 at 06:55:10AM +0000, Roi Dayan wrote: > > > On 18/03/2019 23:58, Ben Pfaff wrote: > > On Sun, Mar 10, 2019 at 05:31:31AM +0000, Eli Britstein wrote: > >> Generate datapath ovs key fields offset and size array macros as a > >> pre-step for bit-wise comparing fields, with no functional change. > >> > >> Signed-off-by: Eli Britstein <elibr@mellanox.com> > >> Reviewed-by: Roi Dayan <roid@mellanox.com> > > > > Thanks! > > > > This introduces a script that uses /bin/bash. I don't think that we > > have other build or runtime dependencies on bash, so it would be better > > to avoid introducing one here. I would rewrite it in terms of portable > > Bourne shell constructs. > > > > Hi Ben, > > just to clarify, when you see portable Bourne shell constructs, > you mean so it can work with other shells like dash on ubuntu? Yes. I have always implicitly thought that any shell that matches Debian's requirements should be acceptable. See the policy document at https://www.debian.org/doc/debian-policy/policy.txt in section 10.4 "Scripts". The most important part of this for OVS purposes is the following bullet: * "local" to create a scoped variable must be supported, including listing multiple variables in a single local command and assigning a value to a variable at the same time as localizing it. "local" may or may not preserve the variable value from an outer scope if no assignment is present. Uses such as: fname () { local a b c=delta d # ... use a, b, c, d ... } must be supported and must set the value of "c" to "delta". > There are other scripts depending on bash. > examples are tests in tests/system-traffic.at, the bash auto completion scripts, ovs-sim. > do you still prefer us to be compatible and use /bin/sh shebang ? I wasn't aware that system-traffic.at depended on bash. It is not ideal. Do you know what dependencies it has? However, we do not expect every developer to run the system-traffic tests, and I believe that it necessarily depends on Linux-specific and third-party utilities anyhow, so it is less important that it work everywhere. The bash auto completion scripts are optional. You can use them if you already use bash and you want them, but there is no downside if you do not. ovs-sim is also somewhat specialized. I couldn't figure out how to do what I wanted without bash, so I decided that it could use bash. This patch, however, introduces a new script without which OVS cannot build at all. It should not depend on bash, absent general agreement in the project that bash is an acceptable new dependency (and absent documentation in the build instructions that bash is a build dependency). I think it would be easy to remove the dependency on bash here, so I think that it should be removed. Thanks, Ben.
diff --git a/.gitignore b/.gitignore index 60e7818c3..2ac9cdac7 100644 --- a/.gitignore +++ b/.gitignore @@ -68,6 +68,7 @@ cscope.* tags _debian odp-netlink.h +odp-netlink-macros.h OvsDpInterface.h /.vagrant/ testsuite.tmp.orig diff --git a/build-aux/extract-odp-netlink-macros-h b/build-aux/extract-odp-netlink-macros-h new file mode 100755 index 000000000..42be29ceb --- /dev/null +++ b/build-aux/extract-odp-netlink-macros-h @@ -0,0 +1,55 @@ +#!/bin/bash + +hfile=$1 + +function generate_fields_macros { + local struct_name=$1 + + # line_start is the line number where the definition of the struct begin + # line_end is the line number where the definition of the struct ends + line_start=`grep -nw $struct_name $hfile | grep { | cut -d ":" -f1` + num_lines=`tail -n +${line_start} $hfile | grep -n -m1 } | cut -d ":" -f1` + line_end=$((line_start+num_lines-1)) + + + STRUCT=`echo $struct_name | tr [a-z] [A-Z]` + echo "#define ${STRUCT}_OFFSETOF_SIZEOF_ARR { \\" + # for all the field lines, including the terminating }, remove ";" and + # replace with an item of {offsetof, sizeof}. + # 3 awk fields are for type struct <struct name> and field <field_name> + # 2 awk fields are for type <type> and field <field_name> + # else - terminating the array by item {0, 0} + awk_cmd="'{" + awk_cmd+=' if (NF == 3)' + awk_cmd+=' print " {offsetof(struct '"${struct_name}"', "$3"), sizeof("$1,$2")}, \\";' + awk_cmd+=' else if (NF == 2)' + awk_cmd+=' print " {offsetof(struct '"${struct_name}"', "$2"), sizeof(" $1")}, \\";' + awk_cmd+=' else' + awk_cmd+=' print " {0, 0}}";' + awk_cmd+="}'" + awk -F ";" "NR>${line_start} && NR<=${line_end}"' {print $1}' $hfile | eval "awk $awk_cmd" + + echo + echo +} + +echo "/* Generated automatically from <include/odp-netlink.h> -- do not modify! */" +echo "#ifndef ODP_NETLINK_MACROS_H" +echo "#define ODP_NETLINK_MACROS_H" +echo +echo + +generate_fields_macros "ovs_key_ethernet" +generate_fields_macros "ovs_key_ipv4" +generate_fields_macros "ovs_key_ipv6" +generate_fields_macros "ovs_key_tcp" +generate_fields_macros "ovs_key_udp" +generate_fields_macros "ovs_key_sctp" +generate_fields_macros "ovs_key_icmp" +generate_fields_macros "ovs_key_icmpv6" +generate_fields_macros "ovs_key_arp" +generate_fields_macros "ovs_key_nd" +generate_fields_macros "ovs_key_nd_extensions" + +echo +echo "#endif" diff --git a/include/automake.mk b/include/automake.mk index 3f3ed1ccd..f493feb93 100644 --- a/include/automake.mk +++ b/include/automake.mk @@ -1,10 +1,15 @@ -BUILT_SOURCES += include/odp-netlink.h +BUILT_SOURCES += include/odp-netlink.h include/odp-netlink-macros.h include/odp-netlink.h: datapath/linux/compat/include/linux/openvswitch.h \ build-aux/extract-odp-netlink-h $(AM_V_GEN)sed -f $(srcdir)/build-aux/extract-odp-netlink-h < $< > $@ -EXTRA_DIST += build-aux/extract-odp-netlink-h -CLEANFILES += include/odp-netlink.h + +include/odp-netlink-macros.h: include/odp-netlink.h \ + build-aux/extract-odp-netlink-macros-h + $(AM_V_GEN)bash -f $(srcdir)/build-aux/extract-odp-netlink-macros-h $< > $@ + +EXTRA_DIST += build-aux/extract-odp-netlink-h build-aux/extract-odp-netlink-macros-h +CLEANFILES += include/odp-netlink.h include/odp-netlink-macros.h include include/ovn/automake.mk include include/openflow/automake.mk