[LTP] [PATCH 0/9] metadata: improvements

[LTP] [PATCH 0/9] metadata: improvements

[Petr Vorel]

Hi all,

there are 3 parts:

1) The main motivation is to avoid having to fix docs manually,
implemented in commit "metaparse: Add missing blank line on the list".

2) Add CI job for generating metadata, to catch the problem early
(we test metadata in docker/podman CI jobs, but error there is quite
hidden. Having special job (which is quick) will show the problem
immediately.

3) Minor verbose output improvements.

Kind regards,
Petr

Petr Vorel (9):
  metaparse: Print parsing file on verbose
  metadata: parse.sh: Allow to pass list of files
  metadata: parse.sh: Pass -v to metaparse on VERBOSE=1
  metadata: test.sh: Print more info on VERBOSE=1
  metaparse: Verbose output on V=1
  metaparse: Add missing blank line on the list
  metadata: Add test for missing blank line in list
  ci/debian: Allow to install packages only for docparse
  ci: Add job for building metadata

 .github/workflows/metadata.yml                | 58 +++++++++++++++++++
 ci/debian.sh                                  | 17 +++++-
 metadata/Makefile                             |  4 +-
 metadata/data_storage.h                       | 36 +++++++++++-
 metadata/metaparse.c                          |  3 +
 metadata/parse.sh                             | 13 ++++-
 metadata/tests/list_missing_blank_line.c      | 14 +++++
 metadata/tests/list_missing_blank_line.c.json | 18 ++++++
 metadata/tests/test.sh                        |  2 +
 9 files changed, 159 insertions(+), 6 deletions(-)
 create mode 100644 .github/workflows/metadata.yml
 create mode 100644 metadata/tests/list_missing_blank_line.c
 create mode 100644 metadata/tests/list_missing_blank_line.c.json

-- 
2.43.0


-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

[LTP] [PATCH 1/9] metaparse: Print parsing file on verbose

[Petr Vorel]

Signed-off-by: Petr Vorel <pvorel@suse.cz>
---
 metadata/metaparse.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/metadata/metaparse.c b/metadata/metaparse.c
index 2384c73c8..fe6d68911 100644
--- a/metadata/metaparse.c
+++ b/metadata/metaparse.c
@@ -862,6 +862,9 @@ int main(int argc, char *argv[])
 		return 1;
 	}
 
+	if (verbose)
+		fprintf(stderr, "\n=== %s ===\n", argv[optind]);
+
 	res = parse_file(argv[optind]);
 	if (!res)
 		return 0;
-- 
2.43.0


-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

[LTP] [PATCH 2/9] metadata: parse.sh: Allow to pass list of files

[Petr Vorel]

Signed-off-by: Petr Vorel <pvorel@suse.cz>
---
 metadata/parse.sh | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/metadata/parse.sh b/metadata/parse.sh
index 69bf5db65..9dd097153 100755
--- a/metadata/parse.sh
+++ b/metadata/parse.sh
@@ -2,6 +2,7 @@
 # SPDX-License-Identifier: GPL-2.0-or-later
 # Copyright (c) 2019 Cyril Hrubis <chrubis@suse.cz>
 # Copyright (c) 2020 Petr Vorel <pvorel@suse.cz>
+# Copyright (c) Linux Test Project, 2022-2024
 set -e
 
 top_builddir=$PWD/..
@@ -29,7 +30,13 @@ echo ' "tests": {'
 
 first=1
 
-for test in `find testcases/ -name '*.c'|sort`; do
+if [ $# -gt 0 ]; then
+	tests=$*
+else
+	tests=$(find testcases/ -name '*.c' | sort)
+fi
+
+for test in $tests; do
 	a=$($top_builddir/metadata/metaparse -Iinclude -Itestcases/kernel/syscalls/utils/ "$test")
 	if [ -n "$a" ]; then
 		if [ -z "$first" ]; then
-- 
2.43.0


-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

[LTP] [PATCH 3/9] metadata: parse.sh: Pass -v to metaparse on VERBOSE=1

[Petr Vorel]

Signed-off-by: Petr Vorel <pvorel@suse.cz>
---
 metadata/parse.sh | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/metadata/parse.sh b/metadata/parse.sh
index 9dd097153..83a3683b5 100755
--- a/metadata/parse.sh
+++ b/metadata/parse.sh
@@ -36,8 +36,10 @@ else
 	tests=$(find testcases/ -name '*.c' | sort)
 fi
 
+[ "$VERBOSE" ] && v="-v"
+
 for test in $tests; do
-	a=$($top_builddir/metadata/metaparse -Iinclude -Itestcases/kernel/syscalls/utils/ "$test")
+	a=$($top_builddir/metadata/metaparse $v -Iinclude -Itestcases/kernel/syscalls/utils/ "$test")
 	if [ -n "$a" ]; then
 		if [ -z "$first" ]; then
 			echo ','
-- 
2.43.0


-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

[LTP] [PATCH 4/9] metadata: test.sh: Print more info on VERBOSE=1

[Petr Vorel]

Signed-off-by: Petr Vorel <pvorel@suse.cz>
---
 metadata/tests/test.sh | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/metadata/tests/test.sh b/metadata/tests/test.sh
index 475d721df..a00e32bb4 100755
--- a/metadata/tests/test.sh
+++ b/metadata/tests/test.sh
@@ -3,6 +3,7 @@
 fail=0
 
 for i in *.c; do
+	[ "$VERBOSE" ] && echo "$0: testing $i"
 	../metaparse $i > tmp.json
 	if ! diff tmp.json $i.json >/dev/null 2>&1; then
 		echo "***"
@@ -15,4 +16,5 @@ done
 
 rm -f tmp.json
 
+[ "$VERBOSE" ] && echo "$fail"
 exit $fail
-- 
2.43.0


-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

[LTP] [PATCH 5/9] metaparse: Verbose output on V=1

[Petr Vorel]

Pass VERBOSE variable to:
* parse.sh (=> pass -v to metaparse.c if VERBOSE=1)
* make test target (=> pass VERBOSE variable to test.sh)

Signed-off-by: Petr Vorel <pvorel@suse.cz>
---
 metadata/Makefile | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/metadata/Makefile b/metadata/Makefile
index 522af4270..750804175 100644
--- a/metadata/Makefile
+++ b/metadata/Makefile
@@ -13,7 +13,7 @@ INSTALL_DIR		= metadata
 .PHONY: ltp.json
 
 ltp.json: metaparse
-	$(abs_srcdir)/parse.sh > ltp.json
+	VERBOSE=$(VERBOSE) $(abs_srcdir)/parse.sh > ltp.json
 ifeq ($(WITH_METADATA),yes)
 	mkdir -p $(abs_top_builddir)/docparse
 	$(MAKE) -C $(abs_top_builddir)/docparse/ -f $(abs_top_srcdir)/docparse/Makefile
@@ -25,6 +25,6 @@ install:
 endif
 
 test:
-	$(MAKE) -C $(abs_srcdir)/tests/ test
+	$(MAKE) -C $(abs_srcdir)/tests/ VERBOSE=$(VERBOSE) test
 
 include $(top_srcdir)/include/mk/generic_leaf_target.mk
-- 
2.43.0


-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

[LTP] [PATCH 6/9] metaparse: Add missing blank line on the list

[Petr Vorel]

Correct list in JSON for correct HTML/PDFformatting via asciidoctor
requires extra space:

 * My CORRECT list (with extra blank line before first item):
 *
 * * foo2
 * * bar2

But people often forget to add it:

 * My BROKEN list (missing blank line before first item):
 * * foo
 * * bar

Therefore add this blank line to fix doc formatting.

Signed-off-by: Petr Vorel <pvorel@suse.cz>
---
 metadata/data_storage.h | 36 +++++++++++++++++++++++++++++++++++-
 1 file changed, 35 insertions(+), 1 deletion(-)

diff --git a/metadata/data_storage.h b/metadata/data_storage.h
index 91ea70a02..0f4d42a13 100644
--- a/metadata/data_storage.h
+++ b/metadata/data_storage.h
@@ -7,9 +7,10 @@
 #define DATA_STORAGE_H__
 
 #include <stdarg.h>
+#include <stdbool.h>
 #include <stdio.h>
-#include <string.h>
 #include <stdlib.h>
+#include <string.h>
 
 enum data_type {
 	DATA_ARRAY,
@@ -275,6 +276,29 @@ static inline void data_node_print_(struct data_node *self, unsigned int padd)
 	}
 }
 
+static inline bool item_is_str_list_member(struct data_node *self)
+{
+	if (self->type != DATA_STRING)
+		return false;
+
+	return self->string.val[0] == '*' && self->string.val[1] == ' ';
+}
+
+static inline bool item_is_str_empty(struct data_node *self)
+{
+	if (self->type != DATA_STRING)
+		return false;
+
+	return !strlen(self->string.val);
+}
+
+static inline bool missing_space_for_list(struct data_node *cur, struct
+						data_node *prev)
+{
+	return item_is_str_list_member(cur) && !item_is_str_empty(prev) &&
+	    !item_is_str_list_member(prev);
+}
+
 static inline void data_node_print(struct data_node *self)
 {
 	printf("{\n");
@@ -357,6 +381,16 @@ static inline void data_to_json_(struct data_node *self, FILE *f, unsigned int p
 	case DATA_ARRAY:
 		data_fprintf(f, do_padd ? padd : 0, "[\n");
 		for (i = 0; i < self->array.array_used; i++) {
+
+			if (i > 0 &&
+			    missing_space_for_list(self->array.array[i],
+						   self->array.array[i-1])) {
+				fprintf(stderr,
+					"%s:%d: WARNING: missing blank line before first list item, add it\n",
+					__FILE__, __LINE__);
+				data_fprintf(f, padd+1, "\"\",\n");
+			}
+
 			data_to_json_(self->array.array[i], f, padd+1, 1);
 			if (i < self->array.array_used - 1)
 				fprintf(f, ",\n");
-- 
2.43.0


-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

[LTP] [PATCH 7/9] metadata: Add test for missing blank line in list

[Petr Vorel]

This is a test for a fix in previous commit.

Signed-off-by: Petr Vorel <pvorel@suse.cz>
---
 metadata/tests/list_missing_blank_line.c      | 14 ++++++++++++++
 metadata/tests/list_missing_blank_line.c.json | 18 ++++++++++++++++++
 2 files changed, 32 insertions(+)
 create mode 100644 metadata/tests/list_missing_blank_line.c
 create mode 100644 metadata/tests/list_missing_blank_line.c.json

diff --git a/metadata/tests/list_missing_blank_line.c b/metadata/tests/list_missing_blank_line.c
new file mode 100644
index 000000000..9d88d6aac
--- /dev/null
+++ b/metadata/tests/list_missing_blank_line.c
@@ -0,0 +1,14 @@
+/*\
+ * [Description]
+ *
+ * Some description.
+ *
+ * My BROKEN list (missing blank line before first item):
+ * * foo
+ * * bar
+ *
+ * My CORRECT list (with extra blank line before first item):
+ *
+ * * foo2
+ * * bar2
+ */
diff --git a/metadata/tests/list_missing_blank_line.c.json b/metadata/tests/list_missing_blank_line.c.json
new file mode 100644
index 000000000..6ad326fb6
--- /dev/null
+++ b/metadata/tests/list_missing_blank_line.c.json
@@ -0,0 +1,18 @@
+  "list_missing_blank_line": {
+   "doc": [
+     "[Description]",
+     "",
+     "Some description.",
+     "",
+     "My BROKEN list (missing blank line before first item):",
+     "",
+     "* foo",
+     "* bar",
+     "",
+     "My CORRECT list (with extra blank line before first item):",
+     "",
+     "* foo2",
+     "* bar2"
+    ],
+   "fname": "list_missing_blank_line.c"
+  }
\ No newline at end of file
-- 
2.43.0


-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

[LTP] [PATCH 8/9] ci/debian: Allow to install packages only for docparse

[Petr Vorel]

Will be used in next commit.

NOTE: Add libwww-perl as explicit dependency (pulled by Debian, but not
by current Ubuntu).

Signed-off-by: Petr Vorel <pvorel@suse.cz>
---
 ci/debian.sh | 17 ++++++++++++++++-
 1 file changed, 16 insertions(+), 1 deletion(-)

diff --git a/ci/debian.sh b/ci/debian.sh
index 96b55a35b..1002bf17a 100755
--- a/ci/debian.sh
+++ b/ci/debian.sh
@@ -1,6 +1,6 @@
 #!/bin/sh -eux
 # SPDX-License-Identifier: GPL-2.0-or-later
-# Copyright (c) 2018-2021 Petr Vorel <pvorel@suse.cz>
+# Copyright (c) 2018-2024 Petr Vorel <pvorel@suse.cz>
 
 # workaround for missing oldstable-updates repository
 # W: Failed to fetch http://deb.debian.org/debian/dists/oldstable-updates/main/binary-amd64/Packages
@@ -13,6 +13,20 @@ export DEBIAN_FRONTEND="noninteractive"
 
 apt="apt install -y --no-install-recommends"
 
+# see .github/workflows/metadata.yml
+if [ "${PACKAGES_FOR_DOCPARSE_ONLY:-}" ]; then
+	$apt \
+		asciidoctor \
+		autoconf \
+		automake \
+		gcc \
+		libjson-perl \
+		libwww-perl \
+		ruby-asciidoctor-pdf
+
+	return 0
+fi
+
 $apt \
 	acl-dev \
 	asciidoc \
@@ -35,6 +49,7 @@ $apt \
 	libc6 \
 	libc6-dev \
 	libjson-perl \
+	libwww-perl \
 	libkeyutils-dev \
 	libkeyutils1 \
 	libmnl-dev \
-- 
2.43.0


-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

[LTP] [PATCH 0/9] metadata: improvements

[Petr Vorel]

From: Petr Vorel <pvorel@suse.cz>

Hi all,

there are 3 parts:

1) The main motivation is to avoid having to fix docs manually,
implemented in commit "metaparse: Add missing blank line on the list".

2) Add CI job for generating metadata, to catch the problem early
(we test metadata in docker/podman CI jobs, but error there is quite
hidden. Having special job (which is quick) will show the problem
immediately.

3) Minor verbose output improvements.

Kind regards,
Petr

Petr Vorel (9):
  metaparse: Print parsing file on verbose
  metadata: parse.sh: Allow to pass list of files
  metadata: parse.sh: Pass -v to metaparse on VERBOSE=1
  metadata: test.sh: Print more info on VERBOSE=1
  metaparse: Verbose output on V=1
  metaparse: Add missing blank line on the list
  metadata: Add test for missing blank line in list
  ci/debian: Allow to install packages only for docparse
  ci: Add job for building metadata

 .github/workflows/metadata.yml                | 58 +++++++++++++++++++
 ci/debian.sh                                  | 17 +++++-
 metadata/Makefile                             |  4 +-
 metadata/data_storage.h                       | 36 +++++++++++-
 metadata/metaparse.c                          |  3 +
 metadata/parse.sh                             | 13 ++++-
 metadata/tests/list_missing_blank_line.c      | 14 +++++
 metadata/tests/list_missing_blank_line.c.json | 18 ++++++
 metadata/tests/test.sh                        |  2 +
 9 files changed, 159 insertions(+), 6 deletions(-)
 create mode 100644 .github/workflows/metadata.yml
 create mode 100644 metadata/tests/list_missing_blank_line.c
 create mode 100644 metadata/tests/list_missing_blank_line.c.json

-- 
2.43.0


-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH 0/9] metadata: improvements

[Petr Vorel]

Hi,

I'm very sorry for the noise (I sent part of this patchset 2x).

Kind regards,
Petr

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH 0/9] metadata: improvements

[Petr Vorel]

Hi,

tested:

https://github.com/pevik/ltp/actions/runs/7414522840
https://github.com/pevik/ltp/actions/runs/7414522847

Kind regards,
Petr

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH 1/9] metaparse: Print parsing file on verbose

[Petr Vorel]

Hi all,

> Signed-off-by: Petr Vorel <pvorel@suse.cz>
> ---
>  metadata/metaparse.c | 3 +++
>  1 file changed, 3 insertions(+)

> diff --git a/metadata/metaparse.c b/metadata/metaparse.c
> index 2384c73c8..fe6d68911 100644
> --- a/metadata/metaparse.c
> +++ b/metadata/metaparse.c
> @@ -862,6 +862,9 @@ int main(int argc, char *argv[])
>  		return 1;
>  	}

> +	if (verbose)
> +		fprintf(stderr, "\n=== %s ===\n", argv[optind]);
> +

This introduces warning:

$ make metaparse
In file included from metaparse.c:17:
In function ‘data_node_string’,
    inlined from ‘main’ at metaparse.c:894:6:
data_storage.h:84:20: warning: array subscript ‘struct data_node[0]’ is partly outside array bounds of ‘unsigned char[6]’ [-Warray-bounds=]
   84 |         node->type = DATA_STRING;
      |         ~~~~~~~~~~~^~~~~~~~~~~~~
data_storage.h:79:34: note: object of size 6 allocated by ‘malloc’
   79 |         struct data_node *node = malloc(size);
      |                                  ^~~~~~~~~~~~
HOSTCC metadata/metaparse

What am I missing?

>  	res = parse_file(argv[optind]);
>  	if (!res)
>  		return 0;

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH 1/9] metaparse: Print parsing file on verbose

[Cyril Hrubis]

Hi!
> This introduces warning:
> 
> $ make metaparse
> In file included from metaparse.c:17:
> In function ‘data_node_string’,
>     inlined from ‘main’ at metaparse.c:894:6:
> data_storage.h:84:20: warning: array subscript ‘struct data_node[0]’ is partly outside array bounds of ‘unsigned char[6]’ [-Warray-bounds=]
>    84 |         node->type = DATA_STRING;
>       |         ~~~~~~~~~~~^~~~~~~~~~~~~
> data_storage.h:79:34: note: object of size 6 allocated by ‘malloc’
>    79 |         struct data_node *node = malloc(size);
>       |                                  ^~~~~~~~~~~~
> HOSTCC metadata/metaparse
> 
> What am I missing?

This looks like the compiler is confused by the union and flexible array
and static analysis produces gibberish. The very fact that this is
triggered by addition of unrelated piece of code supports that hypotesis
as well.

-- 
Cyril Hrubis
chrubis@suse.cz

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH 1/9] metaparse: Print parsing file on verbose

[Cyril Hrubis]

Hi!
Reviewed-by: Cyril Hrubis <chrubis@suse.cz>

-- 
Cyril Hrubis
chrubis@suse.cz

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH 2/9] metadata: parse.sh: Allow to pass list of files

[Cyril Hrubis]

Hi!
> +if [ $# -gt 0 ]; then
> +	tests=$*
> +else
> +	tests=$(find testcases/ -name '*.c' | sort)
> +fi

This unfortunately does not work when there are unexpected characters in
the paths. Which shouldn't happen unless you pass an absoulte path to
the script which contains for example space.

I do not think that we can safely pass a list in a variable without
breaking it in that case. E.g. it works directly with $* or $@ if it's
quoted properly as:

for test in "$@"; do
	...

But as long as you pass $@ indirectly it breaks on spaces.


Note that the subshell $() with find has the same problem, but there is
much less room for breaking something because that is passed relative
paths inside of LTP.

And yes I hate argument parsing in shell..

> +for test in $tests; do
>  	a=$($top_builddir/metadata/metaparse -Iinclude -Itestcases/kernel/syscalls/utils/ "$test")
>  	if [ -n "$a" ]; then
>  		if [ -z "$first" ]; then

-- 
Cyril Hrubis
chrubis@suse.cz

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH 3/9] metadata: parse.sh: Pass -v to metaparse on VERBOSE=1

[Cyril Hrubis]

Hi!
> +[ "$VERBOSE" ] && v="-v"

The build system uses just V=1 for a verbose mode so we should probably
be consistent...

Other than that:

Reviewed-by: Cyril Hrubis <chrubis@suse.cz>

>  for test in $tests; do
> -	a=$($top_builddir/metadata/metaparse -Iinclude -Itestcases/kernel/syscalls/utils/ "$test")
> +	a=$($top_builddir/metadata/metaparse $v -Iinclude -Itestcases/kernel/syscalls/utils/ "$test")
>  	if [ -n "$a" ]; then
>  		if [ -z "$first" ]; then
>  			echo ','
> -- 
> 2.43.0
> 
> 
> -- 
> Mailing list info: https://lists.linux.it/listinfo/ltp

-- 
Cyril Hrubis
chrubis@suse.cz

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH 5/9] metaparse: Verbose output on V=1

[Cyril Hrubis]

Hi!

If we just use the V=1 variable directly I we shouldn't need to pass the
VERBOSE= variable here, or do I miss something?

-- 
Cyril Hrubis
chrubis@suse.cz

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH 4/9] metadata: test.sh: Print more info on VERBOSE=1

[Cyril Hrubis]

Hi!
> diff --git a/metadata/tests/test.sh b/metadata/tests/test.sh
> index 475d721df..a00e32bb4 100755
> --- a/metadata/tests/test.sh
> +++ b/metadata/tests/test.sh
> @@ -3,6 +3,7 @@
>  fail=0
>  
>  for i in *.c; do
> +	[ "$VERBOSE" ] && echo "$0: testing $i"

Here as well, just use $V instead, and maybe it does not make sense to
print the $0. Possibly just "parsing $i".

>  	../metaparse $i > tmp.json
>  	if ! diff tmp.json $i.json >/dev/null 2>&1; then
>  		echo "***"
> @@ -15,4 +16,5 @@ done
>  
>  rm -f tmp.json
>  
> +[ "$VERBOSE" ] && echo "$fail"

Maybe it would make more sense to print pass/fail for each file, i.e.

Parsing array_size01.c Pass
Parsing array_size02.c Pass
Parsing array_size03.c Fail
...

>  exit $fail
> -- 
> 2.43.0
> 
> 
> -- 
> Mailing list info: https://lists.linux.it/listinfo/ltp

-- 
Cyril Hrubis
chrubis@suse.cz

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH 6/9] metaparse: Add missing blank line on the list

[Cyril Hrubis]

Hi!
> +static inline bool item_is_str_list_member(struct data_node *self)
> +{
> +	if (self->type != DATA_STRING)
> +		return false;
> +
> +	return self->string.val[0] == '*' && self->string.val[1] == ' ';

The lists in asciidoc may also start with '-' and I tend to use it
instead of asterisk because it's easier to see inside the C style
comments.

> +}
> +
> +static inline bool item_is_str_empty(struct data_node *self)
> +{
> +	if (self->type != DATA_STRING)
> +		return false;
> +
> +	return !strlen(self->string.val);

This is fancy way of doing !self->string.val[0]

> +}
> +
> +static inline bool missing_space_for_list(struct data_node *cur, struct
> +						data_node *prev)
> +{
> +	return item_is_str_list_member(cur) && !item_is_str_empty(prev) &&
> +	    !item_is_str_list_member(prev);
> +}
> +
>  static inline void data_node_print(struct data_node *self)
>  {
>  	printf("{\n");
> @@ -357,6 +381,16 @@ static inline void data_to_json_(struct data_node *self, FILE *f, unsigned int p
>  	case DATA_ARRAY:
>  		data_fprintf(f, do_padd ? padd : 0, "[\n");
>  		for (i = 0; i < self->array.array_used; i++) {
> +
> +			if (i > 0 &&
> +			    missing_space_for_list(self->array.array[i],
> +						   self->array.array[i-1])) {
> +				fprintf(stderr,
> +					"%s:%d: WARNING: missing blank line before first list item, add it\n",
> +					__FILE__, __LINE__);
> +				data_fprintf(f, padd+1, "\"\",\n");
> +			}
> +
>  			data_to_json_(self->array.array[i], f, padd+1, 1);
>  			if (i < self->array.array_used - 1)
>  				fprintf(f, ",\n");

I'm not sure if we should add the asciidoc validation into the metadata
parser. It feels like this could have been done easier in a perl script,
especially if we are going to add more checks.

-- 
Cyril Hrubis
chrubis@suse.cz

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH 8/9] ci/debian: Allow to install packages only for docparse

[Cyril Hrubis]

Hi!
This looks OK, but patch 9/9 seems to be missing from the patchset.

-- 
Cyril Hrubis
chrubis@suse.cz

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH 8/9] ci/debian: Allow to install packages only for docparse

[Petr Vorel]

Hi Cyril,

> Hi!
> This looks OK, but patch 9/9 seems to be missing from the patchset.

Indeed, I reposted it, but looking your comments there will be v2 anyway :).

Kind regards,
Petr

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH 1/9] metaparse: Print parsing file on verbose

[Petr Vorel]

> Hi!
> > This introduces warning:

> > $ make metaparse
> > In file included from metaparse.c:17:
> > In function ‘data_node_string’,
> >     inlined from ‘main’ at metaparse.c:894:6:
> > data_storage.h:84:20: warning: array subscript ‘struct data_node[0]’ is partly outside array bounds of ‘unsigned char[6]’ [-Warray-bounds=]
> >    84 |         node->type = DATA_STRING;
> >       |         ~~~~~~~~~~~^~~~~~~~~~~~~
> > data_storage.h:79:34: note: object of size 6 allocated by ‘malloc’
> >    79 |         struct data_node *node = malloc(size);
> >       |                                  ^~~~~~~~~~~~
> > HOSTCC metadata/metaparse

> > What am I missing?

> This looks like the compiler is confused by the union and flexible array
> and static analysis produces gibberish. The very fact that this is
> triggered by addition of unrelated piece of code supports that hypotesis
> as well.

Thanks for info. So really false positive? Therefore we have nothing to improve
and we should report to gcc?

Kind regards,
Petr

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH 2/9] metadata: parse.sh: Allow to pass list of files

[Petr Vorel]

HI Cyril,

> Hi!
> > +if [ $# -gt 0 ]; then
> > +	tests=$*
> > +else
> > +	tests=$(find testcases/ -name '*.c' | sort)
> > +fi

> This unfortunately does not work when there are unexpected characters in
> the paths. Which shouldn't happen unless you pass an absoulte path to
> the script which contains for example space.

Ah :(.

> I do not think that we can safely pass a list in a variable without
> breaking it in that case. E.g. it works directly with $* or $@ if it's
> quoted properly as:

> for test in "$@"; do
> 	...

OK, we can either drop it entirely, or use something like this (I'm not happy
about global):

parse()
{
	local test="$1"

	a=$($top_builddir/metadata/metaparse -Iinclude -Itestcases/kernel/syscalls/utils/ "$test")
	if [ -n "$a" ]; then
		if [ -z "$first" ]; then
			echo ','
		fi
		first=
		cat <<EOF
$a
EOF
	fi
}

first=1

if [ $# -gt 0 ]; then
	for test in "$@"; do
		parse "$test"
	done
else
	for test in $(find testcases/ -name '*.c' | sort); do
		parse "$test"
	done
fi

> But as long as you pass $@ indirectly it breaks on spaces.


> Note that the subshell $() with find has the same problem, but there is
> much less room for breaking something because that is passed relative
> paths inside of LTP.

> And yes I hate argument parsing in shell..

Yeah, we all love shell pitfalls :).

Kind regards,
Petr

> > +for test in $tests; do
> >  	a=$($top_builddir/metadata/metaparse -Iinclude -Itestcases/kernel/syscalls/utils/ "$test")
> >  	if [ -n "$a" ]; then
> >  		if [ -z "$first" ]; then

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH 3/9] metadata: parse.sh: Pass -v to metaparse on VERBOSE=1

[Petr Vorel]

Hi Cyril,

> Hi!
> > +[ "$VERBOSE" ] && v="-v"

> The build system uses just V=1 for a verbose mode so we should probably
> be consistent...

We actually use both, see include/mk/env_pre.mk

ifeq ($V,1)
VERBOSE=1
endif

Therefore should I use both here as well?

[ "$VERBOSE" -o "$V" ] && v="-v"

Kind regards,
Petr

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH 3/9] metadata: parse.sh: Pass -v to metaparse on VERBOSE=1

[Petr Vorel]

> Hi Cyril,

> > Hi!
> > > +[ "$VERBOSE" ] && v="-v"

> > The build system uses just V=1 for a verbose mode so we should probably
> > be consistent...

> We actually use both, see include/mk/env_pre.mk

> ifeq ($V,1)
> VERBOSE=1
> endif

> Therefore should I use both here as well?

> [ "$VERBOSE" -o "$V" ] && v="-v"

[ "$V" ] && VERBOSE=1
...
a=$($top_builddir/metadata/metaparse $VERBOSE -Iinclude -Itestcases/kernel/syscalls/utils/ "$test")

(for global scope variable is better to use upper case)

Kind regards,
Petr

> Kind regards,
> Petr

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH 5/9] metaparse: Verbose output on V=1

[Petr Vorel]

Hi Cyril,

> Hi!

> If we just use the V=1 variable directly I we shouldn't need to pass the
> VERBOSE= variable here, or do I miss something?

As I noted elsewhere we support both VERBOSE=1 and V=1. We can decide to switch
to only V=1 if you want (in make build in include/mk/env_pre.mk).

Kind regards,
Petr

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH 3/9] metadata: parse.sh: Pass -v to metaparse on VERBOSE=1

[Cyril Hrubis]

Hi!
> Therefore should I use both here as well?

I guess checking for both in shell scripts is the easiest solution.

-- 
Cyril Hrubis
chrubis@suse.cz

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH 6/9] metaparse: Add missing blank line on the list

[Cyril Hrubis]

Hi!
> I'm not sure if we should add the asciidoc validation into the metadata
> parser. It feels like this could have been done easier in a perl script,
> especially if we are going to add more checks.

To make this clear, maybe this would be better as a perl script that is
executed as a part of 'make check'.

-- 
Cyril Hrubis
chrubis@suse.cz

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH 4/9] metadata: test.sh: Print more info on VERBOSE=1

[Petr Vorel]

> Hi!
> > diff --git a/metadata/tests/test.sh b/metadata/tests/test.sh
> > index 475d721df..a00e32bb4 100755
> > --- a/metadata/tests/test.sh
> > +++ b/metadata/tests/test.sh
> > @@ -3,6 +3,7 @@
> >  fail=0

> >  for i in *.c; do
> > +	[ "$VERBOSE" ] && echo "$0: testing $i"

> Here as well, just use $V instead, and maybe it does not make sense to
> print the $0. Possibly just "parsing $i".

Make sense.

> >  	../metaparse $i > tmp.json
> >  	if ! diff tmp.json $i.json >/dev/null 2>&1; then
> >  		echo "***"
> > @@ -15,4 +16,5 @@ done

> >  rm -f tmp.json

> > +[ "$VERBOSE" ] && echo "$fail"

> Maybe it would make more sense to print pass/fail for each file, i.e.

> Parsing array_size01.c Pass
> Parsing array_size02.c Pass
> Parsing array_size03.c Fail
+1

for i in *.c; do
	../metaparse $i > tmp.json
	[ "$VERBOSE" ] && echo "***** Parsing $i *****"

	if ! diff tmp.json $i.json >/dev/null 2>&1; then
		echo "***"
		echo "$i output differs!"
		diff -u tmp.json $i.json
		echo "***"
		fail=1
		[ "$VERBOSE" ] && echo "$i: FAIL"
	else
		[ "$VERBOSE" ] && echo "$i: PASS"
	fi
done

Kind regards,
Petr

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH 6/9] metaparse: Add missing blank line on the list

[Petr Vorel]

> Hi!
> > +static inline bool item_is_str_list_member(struct data_node *self)
> > +{
> > +	if (self->type != DATA_STRING)
> > +		return false;
> > +
> > +	return self->string.val[0] == '*' && self->string.val[1] == ' ';

> The lists in asciidoc may also start with '-' and I tend to use it
> instead of asterisk because it's easier to see inside the C style
> comments.

+1

> > +}
> > +
> > +static inline bool item_is_str_empty(struct data_node *self)
> > +{
> > +	if (self->type != DATA_STRING)
> > +		return false;
> > +
> > +	return !strlen(self->string.val);

> This is fancy way of doing !self->string.val[0]

+2 (thanks for teaching me proper C :))

...
> >  		for (i = 0; i < self->array.array_used; i++) {
> > +
> > +			if (i > 0 &&
> > +			    missing_space_for_list(self->array.array[i],
> > +						   self->array.array[i-1])) {
> > +				fprintf(stderr,
> > +					"%s:%d: WARNING: missing blank line before first list item, add it\n",
> > +					__FILE__, __LINE__);
> > +				data_fprintf(f, padd+1, "\"\",\n");
> > +			}
> > +
> >  			data_to_json_(self->array.array[i], f, padd+1, 1);
> >  			if (i < self->array.array_used - 1)
> >  				fprintf(f, ",\n");

> I'm not sure if we should add the asciidoc validation into the metadata
> parser. It feels like this could have been done easier in a perl script,
> especially if we are going to add more checks.

You're right. I thought it'd be out of context in perl script, but we works with
arrays there as well, detection should be as easy as here in metadata parser.

I'll change this in v2.

Kind regards,
Petr

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp