keymap: abstract a bit over the keymap format

Make it a bit easier to experiment with other formats.

Add a struct xkb_keymap_format_operations, which currently contains the
keymap compilation and _get_as_string functions. Each format can
implement whatever it wants from these.

The current public entry points become wrappers which do some error
reporting, allocation etc., and calling to the specific format. The
wrappers are all moved to src/keymap.c, so there are no XKB_EXPORT's
under src/xkbcomp/ anymore.

The only format available now is normal text_v1.

This is all not very KISS, and adds some indirection, but it is helpful
and somewhat cleaner.

Signed-off-by: Ran Benita <ran234@gmail.com>

10 files changed, 268 insertions, 121 deletions

diff --git a/Makefile.am b/Makefile.am
index 611e70b..027fbf3 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -38,6 +38,7 @@ libxkbcommon_la_SOURCES = \
 	src/xkbcomp/keycodes.c \
 	src/xkbcomp/keycodes.h \
 	src/xkbcomp/keymap.c \
+	src/xkbcomp/keymap-dump.c \
 	src/xkbcomp/parser.y \
 	src/xkbcomp/parser-priv.h \
 	src/xkbcomp/rules.c \
@@ -55,7 +56,6 @@ libxkbcommon_la_SOURCES = \
 	src/context.h \
 	src/compat.c \
 	src/darray.h \
-	src/keymap-dump.c \
 	src/keysym.c \
 	src/keysym.h \
 	src/keysym-utf.c \
diff --git a/src/context.h b/src/context.h
index 16bd321..fdfdd77 100644
--- a/src/context.h
+++ b/src/context.h
@@ -99,4 +99,14 @@ xkb_log(struct xkb_context *ctx, enum xkb_log_level level,
 #define log_vrb(ctx, vrb, ...) \
     xkb_log_cond_verbosity((ctx), XKB_LOG_LEVEL_WARNING, (vrb), __VA_ARGS__)
 
+/*
+ * Variants which are prefixed by the name of the function they're
+ * called from.
+ * Here we must have the silly 1 variant.
+ */
+#define log_err_func(ctx, fmt, ...) \
+    log_err(ctx, "%s: " fmt, __func__, __VA_ARGS__)
+#define log_err_func1(ctx, fmt) \
+    log_err(ctx, "%s: " fmt, __func__)
+
 #endif
diff --git a/src/keymap.c b/src/keymap.c
index e5bc2e6..9c581d5 100644
--- a/src/keymap.c
+++ b/src/keymap.c
@@ -53,7 +53,7 @@
 #include "keymap.h"
 #include "text.h"
 
-struct xkb_keymap *
+static struct xkb_keymap *
 xkb_keymap_new(struct xkb_context *ctx,
                enum xkb_keymap_format format,
                enum xkb_keymap_compile_flags flags)
@@ -119,6 +119,152 @@ xkb_keymap_unref(struct xkb_keymap *keymap)
     free(keymap);
 }
 
+static const struct xkb_keymap_format_ops *
+get_keymap_format_ops(enum xkb_keymap_format format)
+{
+    static const struct xkb_keymap_format_ops *keymap_format_ops[] = {
+        [XKB_KEYMAP_FORMAT_TEXT_V1] = &text_v1_keymap_format_ops,
+    };
+
+    if ((int) format < 0 || (int) format >= ARRAY_SIZE(keymap_format_ops))
+        return NULL;
+
+    return keymap_format_ops[format];
+}
+
+XKB_EXPORT struct xkb_keymap *
+xkb_keymap_new_from_names(struct xkb_context *ctx,
+                          const struct xkb_rule_names *rmlvo_in,
+                          enum xkb_keymap_compile_flags flags)
+{
+    struct xkb_keymap *keymap;
+    struct xkb_rule_names rmlvo;
+    const enum xkb_keymap_format format = XKB_KEYMAP_FORMAT_TEXT_V1;
+    const struct xkb_keymap_format_ops *ops;
+
+    ops = get_keymap_format_ops(format);
+    if (!ops || !ops->keymap_new_from_names) {
+        log_err_func(ctx, "unsupported keymap format: %d\n", format);
+        return NULL;
+    }
+
+    if (flags & ~(XKB_MAP_COMPILE_PLACEHOLDER)) {
+        log_err_func(ctx, "unrecognized flags: %#x\n", flags);
+        return NULL;
+    }
+
+    rmlvo = *rmlvo_in;
+    if (isempty(rmlvo.rules))
+        rmlvo.rules = DEFAULT_XKB_RULES;
+    if (isempty(rmlvo.model))
+        rmlvo.model = DEFAULT_XKB_MODEL;
+    if (isempty(rmlvo.layout))
+        rmlvo.layout = DEFAULT_XKB_LAYOUT;
+
+    keymap = xkb_keymap_new(ctx, format, flags);
+    if (!keymap)
+        return NULL;
+
+    if (!ops->keymap_new_from_names(keymap, &rmlvo)) {
+        xkb_keymap_unref(keymap);
+        return NULL;
+    }
+
+    return keymap;
+}
+
+XKB_EXPORT struct xkb_keymap *
+xkb_keymap_new_from_string(struct xkb_context *ctx,
+                           const char *string,
+                           enum xkb_keymap_format format,
+                           enum xkb_keymap_compile_flags flags)
+{
+    struct xkb_keymap *keymap;
+    const struct xkb_keymap_format_ops *ops;
+
+    ops = get_keymap_format_ops(format);
+    if (!ops || !ops->keymap_new_from_string) {
+        log_err_func(ctx, "unsupported keymap format: %d\n", format);
+        return NULL;
+    }
+
+    if (flags & ~(XKB_MAP_COMPILE_PLACEHOLDER)) {
+        log_err_func(ctx, "unrecognized flags: %#x\n", flags);
+        return NULL;
+    }
+
+    if (!string) {
+        log_err_func1(ctx, "no string specified\n");
+        return NULL;
+    }
+
+    keymap = xkb_keymap_new(ctx, format, flags);
+    if (!keymap)
+        return NULL;
+
+    if (!ops->keymap_new_from_string(keymap, string)) {
+        xkb_keymap_unref(keymap);
+        return NULL;
+    }
+
+    return keymap;
+}
+
+XKB_EXPORT struct xkb_keymap *
+xkb_keymap_new_from_file(struct xkb_context *ctx,
+                         FILE *file,
+                         enum xkb_keymap_format format,
+                         enum xkb_keymap_compile_flags flags)
+{
+    struct xkb_keymap *keymap;
+    const struct xkb_keymap_format_ops *ops;
+
+    ops = get_keymap_format_ops(format);
+    if (!ops || !ops->keymap_new_from_file) {
+        log_err_func(ctx, "unsupported keymap format: %d\n", format);
+        return NULL;
+    }
+
+    if (flags & ~(XKB_MAP_COMPILE_PLACEHOLDER)) {
+        log_err_func(ctx, "unrecognized flags: %#x\n", flags);
+        return NULL;
+    }
+
+    if (!file) {
+        log_err_func1(ctx, "no file specified\n");
+        return NULL;
+    }
+
+    keymap = xkb_keymap_new(ctx, format, flags);
+    if (!keymap)
+        return NULL;
+
+    if (!ops->keymap_new_from_file(keymap, file)) {
+        xkb_keymap_unref(keymap);
+        return NULL;
+    }
+
+    return keymap;
+}
+
+XKB_EXPORT char *
+xkb_keymap_get_as_string(struct xkb_keymap *keymap,
+                         enum xkb_keymap_format format)
+{
+    const struct xkb_keymap_format_ops *ops;
+
+    if (format == XKB_KEYMAP_USE_ORIGINAL_FORMAT)
+        format = keymap->format;
+
+    ops = get_keymap_format_ops(format);
+    if (!ops || !ops->keymap_get_as_string) {
+        log_err_func(keymap->ctx, "unsupported keymap format: %d\n", format);
+        return NULL;
+    }
+
+    return ops->keymap_get_as_string(keymap);
+}
+
 /**
  * Returns the total number of modifiers active in the keymap.
  */
diff --git a/src/keymap.h b/src/keymap.h
index 1744b41..30733bf 100644
--- a/src/keymap.h
+++ b/src/keymap.h
@@ -78,8 +78,8 @@
  *         Dan Nicholson <dbn.lists@gmail.com>
  */
 
-#ifndef MAP_H
-#define MAP_H
+#ifndef KEYMAP_H
+#define KEYMAP_H
 
  /* Don't use compat names in internal code. */
 #define _XKBCOMMON_COMPAT_H
@@ -422,15 +422,21 @@ XkbKeyGroupWidth(const struct xkb_key *key, xkb_layout_index_t layout)
     return key->groups[layout].type->num_levels;
 }
 
-struct xkb_keymap *
-xkb_keymap_new(struct xkb_context *ctx,
-               enum xkb_keymap_format format,
-               enum xkb_keymap_compile_flags);
-
 xkb_layout_index_t
 wrap_group_into_range(int32_t group,
                       xkb_layout_index_t num_groups,
                       enum xkb_range_exceed_type out_of_range_group_action,
                       xkb_layout_index_t out_of_range_group_number);
 
+struct xkb_keymap_format_ops {
+    bool (*keymap_new_from_names)(struct xkb_keymap *keymap,
+                                  const struct xkb_rule_names *names);
+    bool (*keymap_new_from_string)(struct xkb_keymap *keymap,
+                                   const char *string);
+    bool (*keymap_new_from_file)(struct xkb_keymap *keymap, FILE *file);
+    char *(*keymap_get_as_string)(struct xkb_keymap *keymap);
+};
+
+extern const struct xkb_keymap_format_ops text_v1_keymap_format_ops;
+
 #endif
diff --git a/src/keymap-dump.c b/src/xkbcomp/keymap-dump.c
index 0ab228b..5ca2a10 100644
--- a/src/keymap-dump.c
+++ b/src/xkbcomp/keymap-dump.c
@@ -49,7 +49,7 @@
  * Author: Daniel Stone <daniel@fooishbar.org>
  */
 
-#include "keymap.h"
+#include "xkbcomp-priv.h"
 #include "text.h"
 
 #define BUF_CHUNK_SIZE 4096
@@ -638,29 +638,26 @@ write_symbols(struct xkb_keymap *keymap, struct buf *buf)
     return true;
 }
 
-XKB_EXPORT char *
-xkb_keymap_get_as_string(struct xkb_keymap *keymap,
-                         enum xkb_keymap_format format)
+static bool
+write_keymap(struct xkb_keymap *keymap, struct buf *buf)
 {
-    bool ok;
-    struct buf buf = { NULL, 0, 0 };
+    return (check_write_buf(buf, "xkb_keymap {\n") &&
+            write_keycodes(keymap, buf) &&
+            write_types(keymap, buf) &&
+            write_compat(keymap, buf) &&
+            write_symbols(keymap, buf) &&
+            check_write_buf(buf, "};\n"));
+}
 
-    if (format == XKB_KEYMAP_USE_ORIGINAL_FORMAT)
-        format = keymap->format;
+char *
+text_v1_keymap_get_as_string(struct xkb_keymap *keymap)
+{
+    struct buf buf = { NULL, 0, 0 };
 
-    if (format != XKB_KEYMAP_FORMAT_TEXT_V1) {
-        log_err(keymap->ctx,
-                "Trying to get a keymap as a string in an unsupported format (%d)\n",
-                format);
+    if (!write_keymap(keymap, &buf)) {
+        free(buf.buf);
         return NULL;
     }
 
-    ok = (check_write_buf(&buf, "xkb_keymap {\n") &&
-          write_keycodes(keymap, &buf) &&
-          write_types(keymap, &buf) &&
-          write_compat(keymap, &buf) &&
-          write_symbols(keymap, &buf) &&
-          check_write_buf(&buf, "};\n"));
-
-    return (ok ? buf.buf : NULL);
+    return buf.buf;
 }
diff --git a/src/xkbcomp/xkbcomp-priv.h b/src/xkbcomp/xkbcomp-priv.h
index 97f8e21..51c033f 100644
--- a/src/xkbcomp/xkbcomp-priv.h
+++ b/src/xkbcomp/xkbcomp-priv.h
@@ -37,6 +37,9 @@ struct xkb_component_names {
     char *symbols;
 };
 
+char *
+text_v1_keymap_get_as_string(struct xkb_keymap *keymap);
+
 XkbFile *
 XkbParseFile(struct xkb_context *ctx, FILE *file,
              const char *file_name, const char *map);
diff --git a/src/xkbcomp/xkbcomp.c b/src/xkbcomp/xkbcomp.c
index 5025dc6..cc4b3ef 100644
--- a/src/xkbcomp/xkbcomp.c
+++ b/src/xkbcomp/xkbcomp.c
@@ -30,77 +30,55 @@
 #include "xkbcomp-priv.h"
 #include "rules.h"
 
-static struct xkb_keymap *
-compile_keymap_file(struct xkb_context *ctx, XkbFile *file,
-                    enum xkb_keymap_format format,
-                    enum xkb_keymap_compile_flags flags)
+static bool
+compile_keymap_file(struct xkb_keymap *keymap, XkbFile *file)
 {
-    struct xkb_keymap *keymap;
-
-    keymap = xkb_keymap_new(ctx, format, flags);
-    if (!keymap)
-        goto err;
-
     if (file->file_type != FILE_TYPE_KEYMAP) {
-        log_err(ctx, "Cannot compile a %s file alone into a keymap\n",
+        log_err(keymap->ctx,
+                "Cannot compile a %s file alone into a keymap\n",
                 xkb_file_type_to_string(file->file_type));
-        goto err;
+        return false;
     }
 
     if (!CompileKeymap(file, keymap, MERGE_OVERRIDE)) {
-        log_err(ctx, "Failed to compile keymap\n");
-        goto err;
+        log_err(keymap->ctx,
+                "Failed to compile keymap\n");
+        return false;
     }
 
-    return keymap;
-
-err:
-    xkb_keymap_unref(keymap);
-    return NULL;
+    return true;
 }
 
-XKB_EXPORT struct xkb_keymap *
-xkb_keymap_new_from_names(struct xkb_context *ctx,
-                          const struct xkb_rule_names *rmlvo_in,
-                          enum xkb_keymap_compile_flags flags)
+static bool
+text_v1_keymap_new_from_names(struct xkb_keymap *keymap,
+                              const struct xkb_rule_names *rmlvo)
 {
     bool ok;
     struct xkb_component_names kccgst;
-    struct xkb_rule_names rmlvo = *rmlvo_in;
     XkbFile *file;
-    struct xkb_keymap *keymap;
-
-    if (isempty(rmlvo.rules))
-        rmlvo.rules = DEFAULT_XKB_RULES;
-    if (isempty(rmlvo.model))
-        rmlvo.model = DEFAULT_XKB_MODEL;
-    if (isempty(rmlvo.layout))
-        rmlvo.layout = DEFAULT_XKB_LAYOUT;
 
-    log_dbg(ctx,
+    log_dbg(keymap->ctx,
             "Compiling from RMLVO: rules '%s', model '%s', layout '%s', "
             "variant '%s', options '%s'\n",
-            strnull(rmlvo.rules), strnull(rmlvo.model),
-            strnull(rmlvo.layout), strnull(rmlvo.variant),
-            strnull(rmlvo.options));
+            rmlvo->rules, rmlvo->model, rmlvo->layout, rmlvo->variant,
+            rmlvo->options);
 
-    ok = xkb_components_from_rules(ctx, &rmlvo, &kccgst);
+    ok = xkb_components_from_rules(keymap->ctx, rmlvo, &kccgst);
     if (!ok) {
-        log_err(ctx,
+        log_err(keymap->ctx,
                 "Couldn't look up rules '%s', model '%s', layout '%s', "
                 "variant '%s', options '%s'\n",
-                strnull(rmlvo.rules), strnull(rmlvo.model),
-                strnull(rmlvo.layout), strnull(rmlvo.variant),
-                strnull(rmlvo.options));
-        return NULL;
+                rmlvo->rules, rmlvo->model, rmlvo->layout, rmlvo->variant,
+                rmlvo->options);
+        return false;
     }
 
-    log_dbg(ctx,
+    log_dbg(keymap->ctx,
             "Compiling from KcCGST: keycodes '%s', types '%s', "
             "compat '%s', symbols '%s'\n",
             kccgst.keycodes, kccgst.types, kccgst.compat, kccgst.symbols);
 
-    file = XkbFileFromComponents(ctx, &kccgst);
+    file = XkbFileFromComponents(keymap->ctx, &kccgst);
 
     free(kccgst.keycodes);
     free(kccgst.types);
@@ -108,72 +86,53 @@ xkb_keymap_new_from_names(struct xkb_context *ctx,
     free(kccgst.symbols);
 
     if (!file) {
-        log_err(ctx,
+        log_err(keymap->ctx,
                 "Failed to generate parsed XKB file from components\n");
-        return NULL;
+        return false;
     }
 
-    keymap = compile_keymap_file(ctx, file, XKB_KEYMAP_FORMAT_TEXT_V1, flags);
+    ok = compile_keymap_file(keymap, file);
     FreeXkbFile(file);
-    return keymap;
+    return ok;
 }
 
-XKB_EXPORT struct xkb_keymap *
-xkb_keymap_new_from_string(struct xkb_context *ctx,
-                           const char *string,
-                           enum xkb_keymap_format format,
-                           enum xkb_keymap_compile_flags flags)
+static bool
+text_v1_keymap_new_from_string(struct xkb_keymap *keymap, const char *string)
 {
-    XkbFile *file;
-    struct xkb_keymap *keymap;
-
-    if (format != XKB_KEYMAP_FORMAT_TEXT_V1) {
-        log_err(ctx, "Unsupported keymap format %d\n", format);
-        return NULL;
-    }
-
-    if (!string) {
-        log_err(ctx, "No string specified to generate XKB keymap\n");
-        return NULL;
-    }
+    bool ok;
+    XkbFile *xkb_file;
 
-    file = XkbParseString(ctx, string, "input");
-    if (!file) {
-        log_err(ctx, "Failed to parse input xkb file\n");
+    xkb_file = XkbParseString(keymap->ctx, string, "(input string)");
+    if (!xkb_file) {
+        log_err(keymap->ctx, "Failed to parse input xkb string\n");
         return NULL;
     }
 
-    keymap = compile_keymap_file(ctx, file, format, flags);
-    FreeXkbFile(file);
-    return keymap;
+    ok = compile_keymap_file(keymap, xkb_file);
+    FreeXkbFile(xkb_file);
+    return ok;
 }
 
-XKB_EXPORT struct xkb_keymap *
-xkb_keymap_new_from_file(struct xkb_context *ctx,
-                         FILE *file,
-                         enum xkb_keymap_format format,
-                         enum xkb_keymap_compile_flags flags)
+static bool
+text_v1_keymap_new_from_file(struct xkb_keymap *keymap, FILE *file)
 {
+    bool ok;
     XkbFile *xkb_file;
-    struct xkb_keymap *keymap;
-
-    if (format != XKB_KEYMAP_FORMAT_TEXT_V1) {
-        log_err(ctx, "Unsupported keymap format %d\n", format);
-        return NULL;
-    }
 
-    if (!file) {
-        log_err(ctx, "No file specified to generate XKB keymap\n");
-        return NULL;
-    }
-
-    xkb_file = XkbParseFile(ctx, file, "(unknown file)", NULL);
+    xkb_file = XkbParseFile(keymap->ctx, file, "(unknown file)", NULL);
     if (!xkb_file) {
-        log_err(ctx, "Failed to parse input xkb file\n");
-        return NULL;
+        log_err(keymap->ctx, "Failed to parse input xkb file\n");
+        return false;
     }
 
-    keymap = compile_keymap_file(ctx, xkb_file, format, flags);
+    ok = compile_keymap_file(keymap, xkb_file);
     FreeXkbFile(xkb_file);
-    return keymap;
+    return ok;
 }
+
+const struct xkb_keymap_format_ops text_v1_keymap_format_ops = {
+    .keymap_new_from_names = text_v1_keymap_new_from_names,
+    .keymap_new_from_string = text_v1_keymap_new_from_string,
+    .keymap_new_from_file = text_v1_keymap_new_from_file,
+    .keymap_get_as_string = text_v1_keymap_get_as_string,
+};
diff --git a/test/filecomp.c b/test/filecomp.c
index 0c1111a..eafe568 100644
--- a/test/filecomp.c
+++ b/test/filecomp.c
@@ -48,6 +48,15 @@ main(void)
     assert(!test_file(ctx, "keymaps/bad.xkb"));
     assert(!test_file(ctx, "does not exist"));
 
+    /* Test response to invalid flags and formats. */
+    fclose(stdin);
+    assert(!xkb_keymap_new_from_file(ctx, NULL, XKB_KEYMAP_FORMAT_TEXT_V1, 0));
+    assert(!xkb_keymap_new_from_file(ctx, stdin, 0, 0));
+    assert(!xkb_keymap_new_from_file(ctx, stdin, XKB_KEYMAP_USE_ORIGINAL_FORMAT, 0));
+    assert(!xkb_keymap_new_from_file(ctx, stdin, 1234, 0));
+    assert(!xkb_keymap_new_from_file(ctx, stdin, XKB_KEYMAP_FORMAT_TEXT_V1, -1));
+    assert(!xkb_keymap_new_from_file(ctx, stdin, XKB_KEYMAP_FORMAT_TEXT_V1, 1234));
+
     xkb_context_unref(ctx);
 
     return 0;
diff --git a/test/rulescomp.c b/test/rulescomp.c
index 7318f75..55113f6 100644
--- a/test/rulescomp.c
+++ b/test/rulescomp.c
@@ -118,5 +118,12 @@ int main(int argc, char *argv[])
 
     assert(!test_rmlvo(ctx, "does-not-exist", "", "", "", ""));
 
+    /* Test response to invalid flags. */
+    {
+        struct xkb_rule_names rmlvo = { NULL };
+        assert(!xkb_keymap_new_from_names(ctx, &rmlvo, -1));
+        assert(!xkb_keymap_new_from_names(ctx, &rmlvo, 5453));
+    }
+
     xkb_context_unref(ctx);
 }
diff --git a/test/stringcomp.c b/test/stringcomp.c
index 7d13340..ab5bbef 100644
--- a/test/stringcomp.c
+++ b/test/stringcomp.c
@@ -81,6 +81,16 @@ main(int argc, char *argv[])
     xkb_keymap_unref(keymap);
     keymap = test_compile_string(ctx, dump);
     assert(keymap);
+
+    /* Test response to invalid formats and flags. */
+    assert(!xkb_keymap_new_from_string(ctx, dump, 0, 0));
+    assert(!xkb_keymap_new_from_string(ctx, dump, -1, 0));
+    assert(!xkb_keymap_new_from_string(ctx, dump, XKB_KEYMAP_FORMAT_TEXT_V1+1, 0));
+    assert(!xkb_keymap_new_from_string(ctx, dump, XKB_KEYMAP_FORMAT_TEXT_V1, -1));
+    assert(!xkb_keymap_new_from_string(ctx, dump, XKB_KEYMAP_FORMAT_TEXT_V1, 1414));
+    assert(!xkb_keymap_get_as_string(keymap, 0));
+    assert(!xkb_keymap_get_as_string(keymap, 4893));
+
     xkb_keymap_unref(keymap);
     free(dump);