Re: [PATCH v3] keytable: Add keymap test

2019-07-04 Thread Sean Young
On Thu, Jul 04, 2019 at 03:24:54PM +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.

That is no longer true. It reads all files and does not error out after
the first parse problem.

> 
> Run as "make check" in the keytable directory.
> 
> Signed-off-by: Bastien Nocera 
> ---
>  utils/keytable/Makefile.am |  6 +++
>  utils/keytable/check_keymaps.c | 67 ++
>  2 files changed, 73 insertions(+)
>  create mode 100644 utils/keytable/check_keymaps.c
> 
> diff --git a/utils/keytable/Makefile.am b/utils/keytable/Makefile.am
> index 148b9446..eb296475 100644
> --- a/utils/keytable/Makefile.am
> +++ b/utils/keytable/Makefile.am
> @@ -1,9 +1,12 @@
>  bin_PROGRAMS = ir-keytable
> +noinst_PROGRAMS = check-keymaps
>  man_MANS = ir-keytable.1 rc_keymap.5
>  sysconf_DATA = rc_maps.cfg
>  keytablesystem_DATA = $(srcdir)/rc_keymaps/*
>  udevrules_DATA = 70-infrared.rules
>  
> +check_keymaps_SOURCES = toml.c toml.h check_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: check-keymaps
> + $(builddir)/check-keymaps $(srcdir)/rc_keymaps/
> +
>  install-data-local:
>   $(install_sh) -d "$(DESTDIR)$(keytableuserdir)"
>  
> diff --git a/utils/keytable/check_keymaps.c b/utils/keytable/check_keymaps.c
> new file mode 100644
> index ..eb8e3e8f
> --- /dev/null
> +++ b/utils/keytable/check_keymaps.c
> @@ -0,0 +1,67 @@
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +#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)

strcmp would work here.

> + return 1;
> + return 0;
> +}
> +
> +int main (int argc, char **argv)
> +{
> + DIR *dir;
> + struct dirent *entry;
> + int ret = 0;
> +
> + 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;
> + }
> +
> + snprintf(path, sizeof(path), "%s/%s", argv[1], entry->d_name);
> + path[sizeof(path) - 1] = '\0';

snprintf() always adds a zero terminator, so the last line is not needed. I
know some implementations of snprintf() on platforms other than Linux are
broken, but we don't care about that.

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


Re: [PATCH v3] keytable: Add keymap test

2019-07-01 Thread Bastien Nocera
On Mon, 2019-07-01 at 12:28 +0100, Sean Young wrote:
> On Fri, Jun 28, 2019 at 11:45:29AM +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.
> > 
> > Signed-off-by: Bastien Nocera 
> > ---
> > Changes since v2:
> > - Added SoB
> 
> What about the other comments?

Please mention as early as possible in your review mails that there's
more comments below. I can't guess that from the mail you sent, see the
screenshot attached.


Re: [PATCH v3] keytable: Add keymap test

2019-07-01 Thread Sean Young
On Fri, Jun 28, 2019 at 11:45:29AM +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.
> 
> Signed-off-by: Bastien Nocera 
> ---
> Changes since v2:
> - Added SoB

What about the other comments?

> 
> 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.

Actually we need those patches before we can merge this.

> 
>  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 ..23084331
> --- /dev/null
> +++ b/utils/keytable/test_keymaps.c
> @@ -0,0 +1,68 @@
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +#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;
> +}