linux-media.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v2] keytable: Add keymap test
@ 2019-06-27  8:13 Bastien Nocera
  2019-06-27 19:33 ` Sean Young
  0 siblings, 1 reply; 4+ messages in thread
From: Bastien Nocera @ 2019-06-27  8:13 UTC (permalink / raw)
  To: linux-media

This new test will try to parse all the ".toml" files in the directory
path passed to it, error'ing out on the first parsing problem.

Run as "make check" in the keytable directory.
---
Changes since v1:
- Fix patch formatting

At least 4 keymaps look broken in the current git:
it913x_v2.toml
pinnacle310e.toml
hisi_poplar.toml
imon_mce.toml

Let me know if you want patches to remove the duplicate entries from
those.

 utils/keytable/Makefile.am    |  6 ++++
 utils/keytable/test_keymaps.c | 68 +++++++++++++++++++++++++++++++++++
 2 files changed, 74 insertions(+)
 create mode 100644 utils/keytable/test_keymaps.c

diff --git a/utils/keytable/Makefile.am b/utils/keytable/Makefile.am
index 148b9446..086d53c2 100644
--- a/utils/keytable/Makefile.am
+++ b/utils/keytable/Makefile.am
@@ -1,9 +1,12 @@
 bin_PROGRAMS = ir-keytable
+noinst_PROGRAMS = test-keymaps
 man_MANS = ir-keytable.1 rc_keymap.5
 sysconf_DATA = rc_maps.cfg
 keytablesystem_DATA = $(srcdir)/rc_keymaps/*
 udevrules_DATA = 70-infrared.rules
 
+test_keymaps_SOURCES = toml.c toml.h test_keymaps.c
+
 ir_keytable_SOURCES = keytable.c parse.h ir-encode.c ir-encode.h toml.c toml.h
 
 if WITH_BPF
@@ -21,6 +24,9 @@ endif
 EXTRA_DIST = 70-infrared.rules rc_keymaps rc_keymaps_userspace gen_keytables.pl ir-keytable.1 rc_maps.cfg rc_keymap.5
 
 # custom target
+check: test-keymaps
+	$(builddir)/test-keymaps $(srcdir)/rc_keymaps/
+
 install-data-local:
 	$(install_sh) -d "$(DESTDIR)$(keytableuserdir)"
 
diff --git a/utils/keytable/test_keymaps.c b/utils/keytable/test_keymaps.c
new file mode 100644
index 00000000..23084331
--- /dev/null
+++ b/utils/keytable/test_keymaps.c
@@ -0,0 +1,68 @@
+#include <string.h>
+#include <errno.h>
+#include <stdio.h>
+#include <sys/types.h>
+#include <dirent.h>
+
+#include "toml.h"
+
+static int
+has_suffix(const char *str, const char *suffix)
+{
+	if (strlen(str) < strlen(suffix))
+		return 0;
+	if (strncmp(str + strlen(str) - strlen(suffix), suffix, strlen(suffix)) == 0)
+		return 1;
+	return 0;
+}
+
+int main (int argc, char **argv)
+{
+	DIR *dir;
+	struct dirent *entry;
+
+	if (argc != 2) {
+		fprintf(stderr, "Usage: %s KEYMAPS-DIRECTORY\n", argv[0]);
+		return 1;
+	}
+
+	dir = opendir(argv[1]);
+	if (!dir) {
+		perror("Could not open directory");
+		return 1;
+	}
+
+	while ((entry = readdir(dir)) != NULL) {
+		struct toml_table_t *root;
+		FILE *fin;
+		char buf[200];
+		char path[2048];
+
+		if (!has_suffix(entry->d_name, ".toml")) {
+			/* Skipping file */
+			continue;
+		}
+
+		memset(path, 0, sizeof(path));
+		strcpy(path, argv[1]);
+		strcpy(path + strlen(argv[1]), "/");
+		strcpy(path + strlen(argv[1]) + 1, entry->d_name);
+		strcpy(path + strlen(argv[1]) + 1 + strlen(entry->d_name), "\0");
+
+		fin = fopen(path, "r");
+		if (!fin) {
+			fprintf(stderr, "Could not open file %s: %s", path, strerror(errno));
+			return 1;
+		}
+
+		root = toml_parse_file(fin, buf, sizeof(buf));
+		fclose(fin);
+		if (!root) {
+			fprintf(stderr, "Failed to parse %s: %s\n", path, buf);
+			return 1;
+		}
+		toml_free(root);
+	}
+
+	return 0;
+}


^ permalink raw reply related	[flat|nested] 4+ messages in thread

* Re: [PATCH v2] keytable: Add keymap test
  2019-06-27  8:13 [PATCH v2] keytable: Add keymap test Bastien Nocera
@ 2019-06-27 19:33 ` Sean Young
  2019-07-01 14:27   ` Bastien Nocera
  0 siblings, 1 reply; 4+ messages in thread
From: Sean Young @ 2019-06-27 19:33 UTC (permalink / raw)
  To: Bastien Nocera; +Cc: linux-media

On Thu, Jun 27, 2019 at 10:13:56AM +0200, Bastien Nocera wrote:
> This new test will try to parse all the ".toml" files in the directory
> path passed to it, error'ing out on the first parsing problem.
> 
> Run as "make check" in the keytable directory.

Good catch, and I like your solution.

It needs a Signed-off-by.

> ---
> Changes since v1:
> - Fix patch formatting
> 
> At least 4 keymaps look broken in the current git:
> it913x_v2.toml
> pinnacle310e.toml
> hisi_poplar.toml
> imon_mce.toml
> 
> Let me know if you want patches to remove the duplicate entries from
> those.

That would be great. They have to be patched in the kernel tree, they
are generated from there.

>  utils/keytable/Makefile.am    |  6 ++++
>  utils/keytable/test_keymaps.c | 68 +++++++++++++++++++++++++++++++++++
>  2 files changed, 74 insertions(+)
>  create mode 100644 utils/keytable/test_keymaps.c
> 
> diff --git a/utils/keytable/Makefile.am b/utils/keytable/Makefile.am
> index 148b9446..086d53c2 100644
> --- a/utils/keytable/Makefile.am
> +++ b/utils/keytable/Makefile.am
> @@ -1,9 +1,12 @@
>  bin_PROGRAMS = ir-keytable
> +noinst_PROGRAMS = test-keymaps
>  man_MANS = ir-keytable.1 rc_keymap.5
>  sysconf_DATA = rc_maps.cfg
>  keytablesystem_DATA = $(srcdir)/rc_keymaps/*
>  udevrules_DATA = 70-infrared.rules
>  
> +test_keymaps_SOURCES = toml.c toml.h test_keymaps.c
> +

It could be called check keymaps in line with the make target.

>  ir_keytable_SOURCES = keytable.c parse.h ir-encode.c ir-encode.h toml.c toml.h
>  
>  if WITH_BPF
> @@ -21,6 +24,9 @@ endif
>  EXTRA_DIST = 70-infrared.rules rc_keymaps rc_keymaps_userspace gen_keytables.pl ir-keytable.1 rc_maps.cfg rc_keymap.5
>  
>  # custom target
> +check: test-keymaps
> +	$(builddir)/test-keymaps $(srcdir)/rc_keymaps/
> +
>  install-data-local:
>  	$(install_sh) -d "$(DESTDIR)$(keytableuserdir)"
>  
> diff --git a/utils/keytable/test_keymaps.c b/utils/keytable/test_keymaps.c
> new file mode 100644
> index 00000000..23084331
> --- /dev/null
> +++ b/utils/keytable/test_keymaps.c
> @@ -0,0 +1,68 @@
> +#include <string.h>
> +#include <errno.h>
> +#include <stdio.h>
> +#include <sys/types.h>
> +#include <dirent.h>
> +
> +#include "toml.h"
> +
> +static int
> +has_suffix(const char *str, const char *suffix)
> +{
> +	if (strlen(str) < strlen(suffix))
> +		return 0;
> +	if (strncmp(str + strlen(str) - strlen(suffix), suffix, strlen(suffix)) == 0)
> +		return 1;
> +	return 0;
> +}
> +
> +int main (int argc, char **argv)
> +{
> +	DIR *dir;
> +	struct dirent *entry;
> +
> +	if (argc != 2) {
> +		fprintf(stderr, "Usage: %s KEYMAPS-DIRECTORY\n", argv[0]);
> +		return 1;
> +	}
> +
> +	dir = opendir(argv[1]);
> +	if (!dir) {
> +		perror("Could not open directory");
> +		return 1;
> +	}
> +
> +	while ((entry = readdir(dir)) != NULL) {
> +		struct toml_table_t *root;
> +		FILE *fin;
> +		char buf[200];
> +		char path[2048];
> +
> +		if (!has_suffix(entry->d_name, ".toml")) {
> +			/* Skipping file */
> +			continue;
> +		}
> +
> +		memset(path, 0, sizeof(path));
> +		strcpy(path, argv[1]);
> +		strcpy(path + strlen(argv[1]), "/");
> +		strcpy(path + strlen(argv[1]) + 1, entry->d_name);
> +		strcpy(path + strlen(argv[1]) + 1 + strlen(entry->d_name), "\0");

These five lines could be replaced with a single snprintf().

> +
> +		fin = fopen(path, "r");
> +		if (!fin) {
> +			fprintf(stderr, "Could not open file %s: %s", path, strerror(errno));
> +			return 1;
> +		}
> +
> +		root = toml_parse_file(fin, buf, sizeof(buf));
> +		fclose(fin);
> +		if (!root) {
> +			fprintf(stderr, "Failed to parse %s: %s\n", path, buf);
> +			return 1;
> +		}
> +		toml_free(root);
> +	}
> +
> +	return 0;
> +}

Great idea!

Thanks
Sean

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH v2] keytable: Add keymap test
  2019-06-27 19:33 ` Sean Young
@ 2019-07-01 14:27   ` Bastien Nocera
  2019-07-01 16:40     ` Bastien Nocera
  0 siblings, 1 reply; 4+ messages in thread
From: Bastien Nocera @ 2019-07-01 14:27 UTC (permalink / raw)
  To: Sean Young; +Cc: linux-media

On Thu, 2019-06-27 at 20:33 +0100, Sean Young wrote:
> On Thu, Jun 27, 2019 at 10:13:56AM +0200, Bastien Nocera wrote:
> > This new test will try to parse all the ".toml" files in the
> > directory
> > path passed to it, error'ing out on the first parsing problem.
> > 
> > Run as "make check" in the keytable directory.
> 
> Good catch, and I like your solution.
> 
> It needs a Signed-off-by.
> 
> > ---
> > Changes since v1:
> > - Fix patch formatting
> > 
> > At least 4 keymaps look broken in the current git:
> > it913x_v2.toml
> > pinnacle310e.toml
> > hisi_poplar.toml
> > imon_mce.toml
> > 
> > Let me know if you want patches to remove the duplicate entries
> > from
> > those.
> 
> That would be great. They have to be patched in the kernel tree, they
> are generated from there.

It's customary to put a comment at the top of generated files
indicating that they shouldn't be modified and list the name and
version of the tools used to generate that file.

So, what's the name of the tool used, and where does it live? :)

> >  utils/keytable/Makefile.am    |  6 ++++
> >  utils/keytable/test_keymaps.c | 68
> > +++++++++++++++++++++++++++++++++++
> >  2 files changed, 74 insertions(+)
> >  create mode 100644 utils/keytable/test_keymaps.c
> > 
> > diff --git a/utils/keytable/Makefile.am
> > b/utils/keytable/Makefile.am
> > index 148b9446..086d53c2 100644
> > --- a/utils/keytable/Makefile.am
> > +++ b/utils/keytable/Makefile.am
> > @@ -1,9 +1,12 @@
> >  bin_PROGRAMS = ir-keytable
> > +noinst_PROGRAMS = test-keymaps
> >  man_MANS = ir-keytable.1 rc_keymap.5
> >  sysconf_DATA = rc_maps.cfg
> >  keytablesystem_DATA = $(srcdir)/rc_keymaps/*
> >  udevrules_DATA = 70-infrared.rules
> >  
> > +test_keymaps_SOURCES = toml.c toml.h test_keymaps.c
> > +
> 
> It could be called check keymaps in line with the make target.

I usually call tests "test" as they aren't beholden to the "check"
target to be run, but up to you.

> >  ir_keytable_SOURCES = keytable.c parse.h ir-encode.c ir-encode.h
> > toml.c toml.h
> >  
> >  if WITH_BPF
> > @@ -21,6 +24,9 @@ endif
> >  EXTRA_DIST = 70-infrared.rules rc_keymaps rc_keymaps_userspace
> > gen_keytables.pl ir-keytable.1 rc_maps.cfg rc_keymap.5
> >  
> >  # custom target
> > +check: test-keymaps
> > +	$(builddir)/test-keymaps $(srcdir)/rc_keymaps/
> > +
> >  install-data-local:
> >  	$(install_sh) -d "$(DESTDIR)$(keytableuserdir)"
> >  
> > diff --git a/utils/keytable/test_keymaps.c
> > b/utils/keytable/test_keymaps.c
> > new file mode 100644
> > index 00000000..23084331
> > --- /dev/null
> > +++ b/utils/keytable/test_keymaps.c
> > @@ -0,0 +1,68 @@
> > +#include <string.h>
> > +#include <errno.h>
> > +#include <stdio.h>
> > +#include <sys/types.h>
> > +#include <dirent.h>
> > +
> > +#include "toml.h"
> > +
> > +static int
> > +has_suffix(const char *str, const char *suffix)
> > +{
> > +	if (strlen(str) < strlen(suffix))
> > +		return 0;
> > +	if (strncmp(str + strlen(str) - strlen(suffix), suffix,
> > strlen(suffix)) == 0)
> > +		return 1;
> > +	return 0;
> > +}
> > +
> > +int main (int argc, char **argv)
> > +{
> > +	DIR *dir;
> > +	struct dirent *entry;
> > +
> > +	if (argc != 2) {
> > +		fprintf(stderr, "Usage: %s KEYMAPS-DIRECTORY\n",
> > argv[0]);
> > +		return 1;
> > +	}
> > +
> > +	dir = opendir(argv[1]);
> > +	if (!dir) {
> > +		perror("Could not open directory");
> > +		return 1;
> > +	}
> > +
> > +	while ((entry = readdir(dir)) != NULL) {
> > +		struct toml_table_t *root;
> > +		FILE *fin;
> > +		char buf[200];
> > +		char path[2048];
> > +
> > +		if (!has_suffix(entry->d_name, ".toml")) {
> > +			/* Skipping file */
> > +			continue;
> > +		}
> > +
> > +		memset(path, 0, sizeof(path));
> > +		strcpy(path, argv[1]);
> > +		strcpy(path + strlen(argv[1]), "/");
> > +		strcpy(path + strlen(argv[1]) + 1, entry->d_name);
> > +		strcpy(path + strlen(argv[1]) + 1 + strlen(entry-
> > >d_name), "\0");
> 
> These five lines could be replaced with a single snprintf().

You're right, it's marginally better.

> > +
> > +		fin = fopen(path, "r");
> > +		if (!fin) {
> > +			fprintf(stderr, "Could not open file %s: %s",
> > path, strerror(errno));
> > +			return 1;
> > +		}
> > +
> > +		root = toml_parse_file(fin, buf, sizeof(buf));
> > +		fclose(fin);
> > +		if (!root) {
> > +			fprintf(stderr, "Failed to parse %s: %s\n",
> > path, buf);
> > +			return 1;
> > +		}
> > +		toml_free(root);
> > +	}
> > +
> > +	return 0;
> > +}
> 
> Great idea!
> 
> Thanks
> Sean


^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH v2] keytable: Add keymap test
  2019-07-01 14:27   ` Bastien Nocera
@ 2019-07-01 16:40     ` Bastien Nocera
  0 siblings, 0 replies; 4+ messages in thread
From: Bastien Nocera @ 2019-07-01 16:40 UTC (permalink / raw)
  To: Sean Young; +Cc: linux-media

On Mon, 2019-07-01 at 16:27 +0200, Bastien Nocera wrote:
> > That would be great. They have to be patched in the kernel tree,
> > they
> > are generated from there.
> 
> It's customary to put a comment at the top of generated files
> indicating that they shouldn't be modified and list the name and
> version of the tools used to generate that file.
> 
> So, what's the name of the tool used, and where does it live? :)

Apparently it's called gen_keytables.pl, and I've sent a couple of
patches to it because it didn't ignore comments, and then sent a couple
of mails to broken definitions in the kernel.

Once the 3 broken definitions in the kernel are fixed, fixing all the
keymaps in v4l-utils will be as easy as regenerating the keymaps, and
I'll send the updated patch for the "check" target.

Don't forget to remove the files before generating them again, a number
of keymaps were removed from the upstream kernel.

Cheers


^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2019-07-01 16:40 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-06-27  8:13 [PATCH v2] keytable: Add keymap test Bastien Nocera
2019-06-27 19:33 ` Sean Young
2019-07-01 14:27   ` Bastien Nocera
2019-07-01 16:40     ` Bastien Nocera

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).