On 3/19/25 21:09, Alexandra Rukomoinikova wrote:
> Added checksum which is calculated for pipeline stages.
> Also, the tool calculate-pipeline-cksum is introduced. This tool
> calculates the cksum of a pipeline stages.
>
> Signed-off-by: Alexandra Rukomoinikova <[email protected]>
> ---
> v2 -> v3: moved cksum from comment to define.
> ---
> Makefile.am | 6 ++++++
> build-aux/automake.mk | 2 ++
> build-aux/calculate-pipeline-cksum | 4 ++++
> build-aux/cksum-pipeline-check | 18 ++++++++++++++++++
> lib/ovn-util.c | 6 +++++-
> 5 files changed, 35 insertions(+), 1 deletion(-)
> create mode 100755 build-aux/calculate-pipeline-cksum
> create mode 100755 build-aux/cksum-pipeline-check
Hi, Alexandra. Thanks for the update!
Depending on the result of the conversation on v1 we may need to add actions
into the checksum calculation, but see some comments for the current patch
below.
Best regards, Ilya Maximets.
>
> diff --git a/Makefile.am b/Makefile.am
> index 6b0f1913a..ae6bb03f3 100644
> --- a/Makefile.am
> +++ b/Makefile.am
> @@ -379,6 +379,12 @@ check-ifconfig:
> fi
> .PHONY: check-ifconfig
>
> +# Check if northd stages cksum valid.
Missed "northd stages" update from the v1.
> +ALL_LOCAL += check-northd
> +.PHONY: check-northd
> +check-northd:
> + @$(srcdir)/build-aux/cksum-pipeline-check $(srcdir)/lib/ovn-util.c
> $(srcdir)/northd/northd.h || exit 1
This line is a little too long. Either wrap it or put the files into
the dependency list add pass them via $?.
> +
> if HAVE_GROFF
> ALL_LOCAL += manpage-check
> manpage-check: $(man_MANS) $(dist_man_MANS) $(noinst_man_MANS)
> diff --git a/build-aux/automake.mk b/build-aux/automake.mk
> index 255f573d7..046d7d84f 100644
> --- a/build-aux/automake.mk
> +++ b/build-aux/automake.mk
> @@ -1,6 +1,8 @@
> EXTRA_DIST += \
> + build-aux/calculate-pipeline-cksum \
> build-aux/calculate-schema-cksum \
> build-aux/cccl \
> + build-aux/cksum-pipeline-check \
> build-aux/cksum-schema-check \
> build-aux/dist-docs \
> build-aux/dpdkstrip.py \
> diff --git a/build-aux/calculate-pipeline-cksum
> b/build-aux/calculate-pipeline-cksum
> new file mode 100755
> index 000000000..bc0cdc7f4
> --- /dev/null
> +++ b/build-aux/calculate-pipeline-cksum
> @@ -0,0 +1,4 @@
> +#!/bin/sh
> +
> +file=$1
Maybe name this variable 'northd_h' to highlight which file is expected here.
> +grep '^\s*PIPELINE_STAGE(' $file | cksum
> diff --git a/build-aux/cksum-pipeline-check b/build-aux/cksum-pipeline-check
> new file mode 100755
> index 000000000..3d6b37960
> --- /dev/null
> +++ b/build-aux/cksum-pipeline-check
> @@ -0,0 +1,18 @@
> +#!/bin/sh
> +
> +file=$1
> +schema=$2
schema? These variables need more appropriate names.
> +
> +cksumcheckpath=`dirname $0`
`...` and $(...) are still mixed up in this file. Replace all the `...`
calls with $(...). Here and in one other place below.
> +sum=$("$cksumcheckpath/calculate-pipeline-cksum" "$schema")
> +checksum=$(sed -n \
> + 's/^#define OVN_NORTHD_PIPELINE_CSUM *"\([^"]*\)".*/\1/p' "$1")
Shouldn't use $1 when the variables are defined for the arguments.
Also, the pattern seems a little overcomplicated. Something like
this will probably suffice:
's/^#define OVN_NORTHD_PIPELINE_CSUM *"\(.*\)"/\1/p'
There shouldn't be any trailing garbage on that line. And if there is
something unexpected in the line, the checksum comparison will fail.
> +
> +if [ "$sum" != "$checksum" ]; then
> + ln=`grep -on 'cksum *= [0-9 ]*' -m1 lib/ovn-util.c | cut -d':' -f1`
$(...)
And this still fails during the distcheck. The variable should be used
instead of a file name. And there is no such line in the file, since
introduction of OVN_NORTHD_PIPELINE_CSUM. You may just grep -on for that
macro instead.
> + echo >&2 "$1:$ln: The checksum \"$sum\" was calculated from the" \
> + "pipeline stages and does not match cksum in $1:$ln -" \
> + "you should probably update the ovn version number and the" \
> + "checksum in $1."
Don't use $1, use the defined variable.
And this line prints the file name 3 times. May be re-worded to something
like:
The checksum \"$sum\" was calculated from the logical pipeline stages
and does not match the OVN_NORTHD_PIPELINE_CSUM - you should update the
checksum with the value listed here and consider updating the OVN internal
version number in $ovn_util_c as well.
> + exit 1
> +fi
> diff --git a/lib/ovn-util.c b/lib/ovn-util.c
> index c65b36bb5..91f9e5cfb 100644
> --- a/lib/ovn-util.c
> +++ b/lib/ovn-util.c
> @@ -896,7 +896,11 @@ ip_address_and_port_from_lb_key(const char *key, char
> **ip_address,
> * modified or a stage is added to a logical pipeline.
> *
> * This value is also used to handle some backward compatibility during
> - * upgrading. It should never decrease or rewind. */
> + * upgrading. It should never decrease or rewind.
> + *
> + * NOTE: if OVN_NORTHD_PIPELINE_CSUM is updated make sure to double check
> + * whether an update of OVN_INTERNAL_MINOR_VER is required. */
Please, update this comment according to the Dumitru's suggestion form the
v1 thread.
> +#define OVN_NORTHD_PIPELINE_CSUM "1317960254 5844"
> #define OVN_INTERNAL_MINOR_VER 6
>
> /* Returns the OVN version. The caller must free the returned value. */
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
