From 0f2e01a503d5763087da95d7a001cf84f190d19b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Tue, 29 Nov 2016 14:42:57 -0500 Subject: [PATCH 01/11] Two small cleanups --- src/basic/util.c | 2 +- src/libsystemd/sd-hwdb/sd-hwdb.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/basic/util.c b/src/basic/util.c index c1b5ca1ef7..d0424e5392 100644 --- a/src/basic/util.c +++ b/src/basic/util.c @@ -493,7 +493,7 @@ void *xbsearch_r(const void *key, const void *base, size_t nmemb, size_t size, u = nmemb; while (l < u) { idx = (l + u) / 2; - p = (void *)(((const char *) base) + (idx * size)); + p = (const char *) base + idx * size; comparison = compar(key, p, arg); if (comparison < 0) u = idx; diff --git a/src/libsystemd/sd-hwdb/sd-hwdb.c b/src/libsystemd/sd-hwdb/sd-hwdb.c index 719e3505c1..811a60f0c3 100644 --- a/src/libsystemd/sd-hwdb/sd-hwdb.c +++ b/src/libsystemd/sd-hwdb/sd-hwdb.c @@ -234,7 +234,7 @@ static int trie_search_f(sd_hwdb *hwdb, const char *search) { uint8_t c; for (; (c = trie_string(hwdb, node->prefix_off)[p]); p++) { - if (c == '*' || c == '?' || c == '[') + if (IN_SET(c, '*', '?', '[')) return trie_fnmatch_f(hwdb, node, p, &buf, search + i + p); if (c != search[i + p]) return 0; From 389be927b485ceb7351963986232fb6975860680 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Tue, 29 Nov 2016 14:46:40 -0500 Subject: [PATCH 02/11] hwdb: remove path comparison which broke overriding of properties Partial fix for #4750. We would compare strings like "/usr/lib/udev/hwdb.d/something.hwdb" and "/etc/udev/hwdb.db/something.hwdb" and conclude that the first has higher priority. Since we process files in order (higher priority later), no comparison is necessary when loading. This partially undoes 3a04b789c6f17dff2000a3cdbeaaf86baa604524 (not in spirit, but in the implementation). --- src/hwdb/hwdb.c | 13 ++----------- 1 file changed, 2 insertions(+), 11 deletions(-) diff --git a/src/hwdb/hwdb.c b/src/hwdb/hwdb.c index 1d16d9f8aa..af31578e11 100644 --- a/src/hwdb/hwdb.c +++ b/src/hwdb/hwdb.c @@ -163,7 +163,6 @@ static int trie_node_add_value(struct trie *trie, struct trie_node *node, const char *filename, size_t line_number) { ssize_t k, v, fn; struct trie_value_entry *val; - int r; k = strbuf_add_string(trie->strings, key, strlen(key)); if (k < 0) @@ -183,17 +182,9 @@ static int trie_node_add_value(struct trie *trie, struct trie_node *node, val = xbsearch_r(&search, node->values, node->values_count, sizeof(struct trie_value_entry), trie_values_cmp, trie); if (val) { - /* - * At this point we have 2 identical properties on the same match-string. We - * strictly order them by filename+line-number, since we know the dynamic - * runtime lookup does the same for multiple matching nodes. + /* At this point we have 2 identical properties on the same match-string. + * Since we process files in order, we just replace the previous value. */ - r = strcmp(filename, trie->strings->buf + val->filename_off); - if (r < 0 || - (r == 0 && line_number < val->line_number)) - return 0; - - /* replace existing earlier key with new value */ val->value_off = v; val->filename_off = fn; val->line_number = line_number; From d8646d05724a658cc60fb03f105db8ead454eba3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Tue, 29 Nov 2016 20:26:35 -0500 Subject: [PATCH 03/11] hwdb, sd-hwdb: rework priority comparison when loading properties We cannot compare filenames directly, because paths are not sortable lexicographically, e.g. /etc/udev is "later" (has higher priority) than /usr/lib/udev. The on-disk format is changed to have a separate field for "file priority", which is stored when writing the binary file, and then loaded and used in comparisons. For data in the previous format (as generated by systemd 232), this information is not available, and we use a trick where the offset into the string table is used as a proxy for priority. Most of the time strings are stored in the order in which the files were processed. This is not entirely reliable, but is good enough to properly order /usr/lib and /etc/, which are the two most common cases. This hack is included because it allows proper parsing of files until the binary hwdb is regenerated. Instead of adding a new field, I reduced the size of line_number from 64 to 32 bits, and added a 16 bit priority field, and 16 bits of padding. Adding a new field of 16 bytes would significantly screw up alignment and increase file size, and line number realistically don't need more than ~20 bits. Fixes #4750. --- src/hwdb/hwdb.c | 31 ++++++++++++---------- src/libsystemd/sd-hwdb/hwdb-internal.h | 4 ++- src/libsystemd/sd-hwdb/sd-hwdb.c | 36 +++++++++++++++++++++++--- 3 files changed, 53 insertions(+), 18 deletions(-) diff --git a/src/hwdb/hwdb.c b/src/hwdb/hwdb.c index af31578e11..41f74ad133 100644 --- a/src/hwdb/hwdb.c +++ b/src/hwdb/hwdb.c @@ -86,7 +86,8 @@ struct trie_value_entry { size_t key_off; size_t value_off; size_t filename_off; - size_t line_number; + uint32_t line_number; + uint16_t file_priority; }; static int trie_children_cmp(const void *v1, const void *v2) { @@ -160,7 +161,7 @@ static int trie_values_cmp(const void *v1, const void *v2, void *arg) { static int trie_node_add_value(struct trie *trie, struct trie_node *node, const char *key, const char *value, - const char *filename, size_t line_number) { + const char *filename, uint16_t file_priority, uint32_t line_number) { ssize_t k, v, fn; struct trie_value_entry *val; @@ -187,6 +188,7 @@ static int trie_node_add_value(struct trie *trie, struct trie_node *node, */ val->value_off = v; val->filename_off = fn; + val->file_priority = file_priority; val->line_number = line_number; return 0; } @@ -201,6 +203,7 @@ static int trie_node_add_value(struct trie *trie, struct trie_node *node, node->values[node->values_count].key_off = k; node->values[node->values_count].value_off = v; node->values[node->values_count].filename_off = fn; + node->values[node->values_count].file_priority = file_priority; node->values[node->values_count].line_number = line_number; node->values_count++; qsort_r(node->values, node->values_count, sizeof(struct trie_value_entry), trie_values_cmp, trie); @@ -209,7 +212,7 @@ static int trie_node_add_value(struct trie *trie, struct trie_node *node, static int trie_insert(struct trie *trie, struct trie_node *node, const char *search, const char *key, const char *value, - const char *filename, uint64_t line_number) { + const char *filename, uint16_t file_priority, uint32_t line_number) { size_t i = 0; int err = 0; @@ -263,7 +266,7 @@ static int trie_insert(struct trie *trie, struct trie_node *node, const char *se c = search[i]; if (c == '\0') - return trie_node_add_value(trie, node, key, value, filename, line_number); + return trie_node_add_value(trie, node, key, value, filename, file_priority, line_number); child = node_lookup(node, c); if (!child) { @@ -287,7 +290,7 @@ static int trie_insert(struct trie *trie, struct trie_node *node, const char *se return err; } - return trie_node_add_value(trie, child, key, value, filename, line_number); + return trie_node_add_value(trie, child, key, value, filename, file_priority, line_number); } node = child; @@ -366,7 +369,8 @@ static int64_t trie_store_nodes(struct trie_f *trie, struct trie_node *node) { .key_off = htole64(trie->strings_off + node->values[i].key_off), .value_off = htole64(trie->strings_off + node->values[i].value_off), .filename_off = htole64(trie->strings_off + node->values[i].filename_off), - .line_number = htole64(node->values[i].line_number), + .line_number = htole32(node->values[i].line_number), + .file_priority = htole16(node->values[i].file_priority), }; fwrite(&v, sizeof(struct trie_value_entry2_f), 1, trie->f); @@ -454,7 +458,7 @@ static int trie_store(struct trie *trie, const char *filename) { } static int insert_data(struct trie *trie, char **match_list, char *line, - const char *filename, size_t line_number) { + const char *filename, uint16_t file_priority, uint32_t line_number) { char *value, **entry; value = strchr(line, '='); @@ -476,12 +480,12 @@ static int insert_data(struct trie *trie, char **match_list, char *line, } STRV_FOREACH(entry, match_list) - trie_insert(trie, trie->root, *entry, line, value, filename, line_number); + trie_insert(trie, trie->root, *entry, line, value, filename, file_priority, line_number); return 0; } -static int import_file(struct trie *trie, const char *filename) { +static int import_file(struct trie *trie, const char *filename, uint16_t file_priority) { enum { HW_NONE, HW_MATCH, @@ -490,7 +494,7 @@ static int import_file(struct trie *trie, const char *filename) { _cleanup_fclose_ FILE *f = NULL; char line[LINE_MAX]; _cleanup_strv_free_ char **match_list = NULL; - size_t line_number = 0; + uint32_t line_number = 0; char *match = NULL; int r; @@ -565,7 +569,7 @@ static int import_file(struct trie *trie, const char *filename) { /* first data */ state = HW_DATA; - insert_data(trie, match_list, line, filename, line_number); + insert_data(trie, match_list, line, filename, file_priority, line_number); break; case HW_DATA: @@ -583,7 +587,7 @@ static int import_file(struct trie *trie, const char *filename) { break; } - insert_data(trie, match_list, line, filename, line_number); + insert_data(trie, match_list, line, filename, file_priority, line_number); break; }; } @@ -616,6 +620,7 @@ static int hwdb_update(int argc, char *argv[], void *userdata) { _cleanup_free_ char *hwdb_bin = NULL; _cleanup_(trie_freep) struct trie *trie = NULL; char **files, **f; + uint16_t file_priority = 1; int r; trie = new0(struct trie, 1); @@ -640,7 +645,7 @@ static int hwdb_update(int argc, char *argv[], void *userdata) { STRV_FOREACH(f, files) { log_debug("reading file '%s'", *f); - import_file(trie, *f); + import_file(trie, *f, file_priority++); } strv_free(files); diff --git a/src/libsystemd/sd-hwdb/hwdb-internal.h b/src/libsystemd/sd-hwdb/hwdb-internal.h index 4fff94ec76..2f4934ae8b 100644 --- a/src/libsystemd/sd-hwdb/hwdb-internal.h +++ b/src/libsystemd/sd-hwdb/hwdb-internal.h @@ -76,5 +76,7 @@ struct trie_value_entry2_f { le64_t key_off; le64_t value_off; le64_t filename_off; - le64_t line_number; + le32_t line_number; + le16_t file_priority; + le16_t padding; } _packed_; diff --git a/src/libsystemd/sd-hwdb/sd-hwdb.c b/src/libsystemd/sd-hwdb/sd-hwdb.c index 811a60f0c3..a8c6376302 100644 --- a/src/libsystemd/sd-hwdb/sd-hwdb.c +++ b/src/libsystemd/sd-hwdb/sd-hwdb.c @@ -164,10 +164,38 @@ static int hwdb_add_property(sd_hwdb *hwdb, const struct trie_value_entry_f *ent entry2 = (const struct trie_value_entry2_f *)entry; old = ordered_hashmap_get(hwdb->properties, key); if (old) { - /* on duplicates, we order by filename and line-number */ - r = strcmp(trie_string(hwdb, entry2->filename_off), trie_string(hwdb, old->filename_off)); - if (r < 0 || - (r == 0 && entry2->line_number < old->line_number)) + /* On duplicates, we order by filename priority and line-number. + * + * + * v2 of the format had 64 bits for the line number. + * v3 reuses top 32 bits of line_number to store the priority. + * We check the top bits — if they are zero we have v2 format. + * This means that v2 clients will print wrong line numbers with + * v3 data. + * + * For v3 data: we compare the priority (of the source file) + * and the line number. + * + * For v2 data: we rely on the fact that the filenames in the hwdb + * are added in the order of priority (higher later), because they + * are *processed* in the order of priority. So we compare the + * indices to determine which file had higher priority. Comparing + * the strings alphabetically would be useless, because those are + * full paths, and e.g. /usr/lib would sort after /etc, even + * though it has lower priority. This is not reliable because of + * suffix compression, but should work for the most common case of + * /usr/lib/udev/hwbd.d and /etc/udev/hwdb.d, and is better than + * not doing the comparison at all. + */ + bool lower; + + if (entry2->file_priority == 0) + lower = entry2->filename_off < old->filename_off || + (entry2->filename_off == old->filename_off && entry2->line_number < old->line_number); + else + lower = entry2->file_priority < old->file_priority || + (entry2->file_priority == old->file_priority && entry2->line_number < old->line_number); + if (lower) return 0; } } From 1cd592b91679f3a04fe884d51dd2b9d65eb5f2f9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Tue, 29 Nov 2016 15:17:19 -0500 Subject: [PATCH 04/11] sd-hwdb: drop caching of search string We have only two callers, and for neither this "optimization" is useful. So let's drop it an save some code and a malloc. --- src/libsystemd/sd-hwdb/sd-hwdb.c | 24 +----------------------- 1 file changed, 1 insertion(+), 23 deletions(-) diff --git a/src/libsystemd/sd-hwdb/sd-hwdb.c b/src/libsystemd/sd-hwdb/sd-hwdb.c index a8c6376302..c155c70162 100644 --- a/src/libsystemd/sd-hwdb/sd-hwdb.c +++ b/src/libsystemd/sd-hwdb/sd-hwdb.c @@ -48,8 +48,6 @@ struct sd_hwdb { const char *map; }; - char *modalias; - OrderedHashmap *properties; Iterator properties_iterator; bool properties_modified; @@ -393,7 +391,6 @@ _public_ sd_hwdb *sd_hwdb_unref(sd_hwdb *hwdb) { if (hwdb->map) munmap((void *)hwdb->map, hwdb->st.st_size); safe_fclose(hwdb->f); - free(hwdb->modalias); ordered_hashmap_free(hwdb->properties); free(hwdb); } @@ -427,32 +424,13 @@ bool hwdb_validate(sd_hwdb *hwdb) { } static int properties_prepare(sd_hwdb *hwdb, const char *modalias) { - _cleanup_free_ char *mod = NULL; - int r; - assert(hwdb); assert(modalias); - if (streq_ptr(modalias, hwdb->modalias)) - return 0; - - mod = strdup(modalias); - if (!mod) - return -ENOMEM; - ordered_hashmap_clear(hwdb->properties); - hwdb->properties_modified = true; - r = trie_search_f(hwdb, modalias); - if (r < 0) - return r; - - free(hwdb->modalias); - hwdb->modalias = mod; - mod = NULL; - - return 0; + return trie_search_f(hwdb, modalias); } _public_ int sd_hwdb_get(sd_hwdb *hwdb, const char *modalias, const char *key, const char **_value) { From d702dcdfef04512fa1529acece6d84ea58740d74 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Tue, 29 Nov 2016 15:27:40 -0500 Subject: [PATCH 05/11] hwdb: simplify error handling in trie_store fclose() can also set errno, so the attempts to protect errno that the code made were not successful. Simplify things by immediately saving errno to r. --- src/hwdb/hwdb.c | 41 ++++++++++++++++++----------------------- 1 file changed, 18 insertions(+), 23 deletions(-) diff --git a/src/hwdb/hwdb.c b/src/hwdb/hwdb.c index 41f74ad133..fba0c309c5 100644 --- a/src/hwdb/hwdb.c +++ b/src/hwdb/hwdb.c @@ -396,24 +396,21 @@ static int trie_store(struct trie *trie, const char *filename) { .child_entry_size = htole64(sizeof(struct trie_child_entry_f)), .value_entry_size = htole64(sizeof(struct trie_value_entry2_f)), }; - int err; + int r; /* calculate size of header, nodes, children entries, value entries */ t.strings_off = sizeof(struct trie_header_f); trie_store_nodes_size(&t, trie->root); - err = fopen_temporary(filename , &t.f, &filename_tmp); - if (err < 0) - return err; + r = fopen_temporary(filename , &t.f, &filename_tmp); + if (r < 0) + return r; fchmod(fileno(t.f), 0444); /* write nodes */ - err = fseeko(t.f, sizeof(struct trie_header_f), SEEK_SET); - if (err < 0) { - fclose(t.f); - unlink_noerrno(filename_tmp); - return -errno; - } + if (fseeko(t.f, sizeof(struct trie_header_f), SEEK_SET) < 0) + goto error; + root_off = trie_store_nodes(&t, trie->root); h.nodes_root_off = htole64(root_off); pos = ftello(t.f); @@ -426,21 +423,14 @@ static int trie_store(struct trie *trie, const char *filename) { /* write header */ size = ftello(t.f); h.file_size = htole64(size); - err = fseeko(t.f, 0, SEEK_SET); - if (err < 0) { - fclose(t.f); + if (fseeko(t.f, 0, SEEK_SET) < 0) + goto error; + + fwrite(&h, sizeof(struct trie_header_f), 1, t.f); + if (fclose(t.f) < 0 || rename(filename_tmp, filename) < 0) { unlink_noerrno(filename_tmp); return -errno; } - fwrite(&h, sizeof(struct trie_header_f), 1, t.f); - err = ferror(t.f); - if (err) - err = -errno; - fclose(t.f); - if (err < 0 || rename(filename_tmp, filename) < 0) { - unlink_noerrno(filename_tmp); - return err < 0 ? err : -errno; - } log_debug("=== trie on-disk ==="); log_debug("size: %8"PRIi64" bytes", size); @@ -453,8 +443,13 @@ static int trie_store(struct trie *trie, const char *filename) { t.values_count * sizeof(struct trie_value_entry2_f), t.values_count); log_debug("string store: %8zu bytes", trie->strings->len); log_debug("strings start: %8"PRIu64, t.strings_off); - return 0; + + error: + r = -errno; + fclose(t.f); + unlink(filename_tmp); + return r; } static int insert_data(struct trie *trie, char **match_list, char *line, From ca0e88efbb86211aa15325c69675fffb1b8ad5c5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Tue, 29 Nov 2016 20:44:22 -0500 Subject: [PATCH 06/11] hwdb: rename err to r and use _cleanup_ in two more places --- src/hwdb/hwdb.c | 40 ++++++++++++++++++++-------------------- 1 file changed, 20 insertions(+), 20 deletions(-) diff --git a/src/hwdb/hwdb.c b/src/hwdb/hwdb.c index fba0c309c5..0fbde28d10 100644 --- a/src/hwdb/hwdb.c +++ b/src/hwdb/hwdb.c @@ -214,7 +214,7 @@ static int trie_insert(struct trie *trie, struct trie_node *node, const char *se const char *key, const char *value, const char *filename, uint16_t file_priority, uint32_t line_number) { size_t i = 0; - int err = 0; + int r = 0; for (;;) { size_t p; @@ -255,9 +255,9 @@ static int trie_insert(struct trie *trie, struct trie_node *node, const char *se node->children_count = 0; node->values = NULL; node->values_count = 0; - err = node_add_child(trie, node, new_child, c); - if (err < 0) - return err; + r = node_add_child(trie, node, new_child, c); + if (r < 0) + return r; new_child = NULL; /* avoid cleanup */ break; @@ -284,10 +284,10 @@ static int trie_insert(struct trie *trie, struct trie_node *node, const char *se } child->prefix_off = off; - err = node_add_child(trie, node, child, c); - if (err < 0) { + r = node_add_child(trie, node, child, c); + if (r < 0) { free(child); - return err; + return r; } return trie_node_add_value(trie, child, key, value, filename, file_priority, line_number); @@ -329,11 +329,11 @@ static int64_t trie_store_nodes(struct trie_f *trie, struct trie_node *node) { .children_count = node->children_count, .values_count = htole64(node->values_count), }; - struct trie_child_entry_f *children = NULL; + _cleanup_free_ struct trie_child_entry_f *children = NULL; int64_t node_off; if (node->children_count) { - children = new0(struct trie_child_entry_f, node->children_count); + children = new(struct trie_child_entry_f, node->children_count); if (!children) return -ENOMEM; } @@ -343,12 +343,13 @@ static int64_t trie_store_nodes(struct trie_f *trie, struct trie_node *node) { int64_t child_off; child_off = trie_store_nodes(trie, node->children[i].child); - if (child_off < 0) { - free(children); + if (child_off < 0) return child_off; - } - children[i].c = node->children[i].c; - children[i].child_off = htole64(child_off); + + children[i] = (struct trie_child_entry_f) { + .c = node->children[i].c, + .child_off = htole64(child_off), + }; } /* write node */ @@ -360,7 +361,6 @@ static int64_t trie_store_nodes(struct trie_f *trie, struct trie_node *node) { if (node->children_count) { fwrite(children, sizeof(struct trie_child_entry_f), node->children_count, trie->f); trie->children_count += node->children_count; - free(children); } /* append values array */ @@ -374,8 +374,8 @@ static int64_t trie_store_nodes(struct trie_f *trie, struct trie_node *node) { }; fwrite(&v, sizeof(struct trie_value_entry2_f), 1, trie->f); - trie->values_count++; } + trie->values_count += node->values_count; return node_off; } @@ -614,7 +614,8 @@ static int hwdb_query(int argc, char *argv[], void *userdata) { static int hwdb_update(int argc, char *argv[], void *userdata) { _cleanup_free_ char *hwdb_bin = NULL; _cleanup_(trie_freep) struct trie *trie = NULL; - char **files, **f; + _cleanup_strv_free_ char **files = NULL; + char **f; uint16_t file_priority = 1; int r; @@ -636,13 +637,12 @@ static int hwdb_update(int argc, char *argv[], void *userdata) { r = conf_files_list_strv(&files, ".hwdb", arg_root, conf_file_dirs); if (r < 0) - return log_error_errno(r, "failed to enumerate hwdb files: %m"); + return log_error_errno(r, "Failed to enumerate hwdb files: %m"); STRV_FOREACH(f, files) { - log_debug("reading file '%s'", *f); + log_debug("Reading file \"%s\"", *f); import_file(trie, *f, file_priority++); } - strv_free(files); strbuf_complete(trie->strings); From dc17ee3d6eb1c642d3518e23c7a1cdabf7225d98 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Tue, 29 Nov 2016 22:29:31 -0500 Subject: [PATCH 07/11] man: describe overriding of entries in hwdb files --- man/hwdb.xml | 90 +++++++++++++++++++++++++++++++++++++++++++--------- 1 file changed, 75 insertions(+), 15 deletions(-) diff --git a/man/hwdb.xml b/man/hwdb.xml index 2b1e60fb22..ae5ddb1a4d 100644 --- a/man/hwdb.xml +++ b/man/hwdb.xml @@ -50,28 +50,88 @@ system-supplied hwdb file with a local file if needed; a symlink in /etc with the same name as a hwdb file in /usr/lib, pointing to /dev/null, - disables the hwdb file entirely. hwdb files must have the extension + disables that hwdb file entirely. hwdb files must have the extension .hwdb; other extensions are ignored. - The hwdb file contains data records consisting of matches and - associated key-value pairs. Every record in the hwdb starts with one or - more match strings, specifying a shell glob to compare the database - lookup string against. Multiple match lines are specified in additional - consecutive lines. Every match line is compared individually, and they are - combined by OR. Every match line must start at the first character of - the line. + Each hwdb file contains data records consisting of matches and associated + key-value pairs. Every record in the hwdb starts with one or more match strings, + specifying a shell glob to compare the lookup string against. Multiple match lines + are specified in consecutive lines. Every match line is compared individually, and + they are combined by OR. Every match line must start at the first character of the + line. - The match lines are followed by one or more key-value pair lines, which - are recognized by a leading space character. The key name and value are separated - by =. An empty line signifies the end - of a record. Lines beginning with # are ignored. + The match lines are followed by one or more key-value pair lines, which are + recognized by a leading space character. The key name and value are separated by + =. An empty line signifies the end of a record. Lines beginning + with # are ignored. + + In case multiple records match a given lookup string, the key-value pairs + from all records are combined. If a key is specified multiple times, the value + from the record with the highest priority is used (each key can have only a single + value). The priority is higher when the record is in a file that sorts later + lexicographically, and in case of records in the same file, later records have + higher priority. The content of all hwdb files is read by systemd-hwdb8 and compiled to a binary database located at /etc/udev/hwdb.bin, - or alternatively /usr/lib/udev/hwdb.bin if you want ship the compiled - database in an immutable image. - During runtime, only the binary database is used. + or alternatively /usr/lib/udev/hwdb.bin if you want ship the + compiled database in an immutable image. During runtime, only the binary database + is used. + + + + Examples + + + General syntax of hwdb files + + # /usr/lib/udev/hwdb.d/example.hwdb +# Comments can be placed before any records. This is a good spot +# to describe what that file is used for, what kind of properties +# it defines, and the ordering convention. + +# A record with three matches and one property +mouse:*:name:*Trackball*: +mouse:*:name:*trackball*: +mouse:*:name:*TrackBall*: + ID_INPUT_TRACKBALL=1 + +# A record with a single match and five properties +mouse:usb:v046dp4041:name:Logitech MX Master: + MOUSE_DPI=1000@166 + MOUSE_WHEEL_CLICK_ANGLE=15 + MOUSE_WHEEL_CLICK_ANGLE_HORIZONTAL=26 + MOUSE_WHEEL_CLICK_COUNT=24 + MOUSE_WHEEL_CLICK_COUNT_HORIZONTAL=14 + + + + + Overriding of properties + + # /usr/lib/udev/hwdb.d/60-keyboard.hwdb +evdev:atkbd:dmi:bvn*:bvr*:bd*:svnAcer*:pn* + KEYBOARD_KEY_a1=help + KEYBOARD_KEY_a2=setup + KEYBOARD_KEY_a3=battery + +evdev:atkbd:dmi:bvn*:bvr*:bd*:svnAcer*:pn123* + KEYBOARD_KEY_a2=wlan + +# /etc/udev/hwdb.d/70-keyboard.hwdb +# disable wlan key on all at keyboards +evdev:atkbd:* + KEYBOARD_KEY_a2=reserved + + If the hwdb consists of those two files, a keyboard with the lookup string + evdev:atkbd:dmi:bvnAcer:bdXXXXX:bd08/05/2010:svnAcer:pn123 + will match all three records, and end up with the following properties: + + KEYBOARD_KEY_a1=help +KEYBOARD_KEY_a2=reserved +KEYBOARD_KEY_a3=battery + From 8ce2069263a5ed9237ca03179a760d3023e12fbe Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Tue, 29 Nov 2016 22:41:52 -0500 Subject: [PATCH 08/11] hwdb-test: properly print error output from systemd-hwdb update Because -e was set, we'd exit immediately when systemd-hwdb failed and the captured error output was never printed. We want to test two things: that 0 is returned, and that nothing is printed to stderr (to catch syntax errors in the hwdb). v2: stop capturing stderr to a variable v3: capture stderr to a variable and test both error types separately --- test/hwdb-test.sh | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/test/hwdb-test.sh b/test/hwdb-test.sh index 4638328059..bfdafce998 100755 --- a/test/hwdb-test.sh +++ b/test/hwdb-test.sh @@ -32,11 +32,17 @@ D=$(mktemp --directory) trap "rm -rf '$D'" EXIT INT QUIT PIPE mkdir -p "$D/etc/udev" ln -s "$ROOTDIR/hwdb" "$D/etc/udev/hwdb.d" -err=$("$SYSTEMD_HWDB" update --root "$D" 2>&1 >/dev/null) + +err=$("$SYSTEMD_HWDB" update --root "$D" 2>&1 >/dev/null) && rc= || rc=$? if [ -n "$err" ]; then echo "$err" - exit 1 + exit ${rc:-1} fi +if [ -n "$rc" ]; then + echo "$SYSTEMD_HWDB returned $rc" + exit $rc +fi + if [ ! -e "$D/etc/udev/hwdb.bin" ]; then echo "$D/etc/udev/hwdb.bin was not generated" exit 1 From 49141e0cd1a56e5f97e81e13a6d7be85138c134c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Thu, 1 Dec 2016 10:55:32 -0500 Subject: [PATCH 09/11] hwdb: improve and test syntax error messages Since syntax error are non-fatal, downgrade them to warnings. Use log_syntax to have uniform formatting including the line number. State machine states like DATA and MATCH are internal, user-facing messages should use the names from hwdb(7): match, property, record. Also change "key/value" to "key-value", since there's no alternative here, both parts must be present. [/tmp/tmp.KFwEhm74n4/etc/udev/hwdb.d/10-bad.hwdb:2] Property expected, ignoring record with no properties [/tmp/tmp.KFwEhm74n4/etc/udev/hwdb.d/10-bad.hwdb:5] Property expected, ignoring record with no properties [/tmp/tmp.KFwEhm74n4/etc/udev/hwdb.d/10-bad.hwdb:9] Property expected, ignoring record with no properties [/tmp/tmp.KFwEhm74n4/etc/udev/hwdb.d/10-bad.hwdb:11] Key/value pair expected but got " NO_VALUE", ignoring [/tmp/tmp.KFwEhm74n4/etc/udev/hwdb.d/10-bad.hwdb:18] Property or empty line expected, got "BAD:7:match at wrong place", ignoring record [/tmp/tmp.KFwEhm74n4/etc/udev/hwdb.d/10-bad.hwdb:22] Property or empty line expected, got "BAD:8:match at wrong place", ignoring record [/tmp/tmp.KFwEhm74n4/etc/udev/hwdb.d/10-bad.hwdb:23] Match expected but got indented property " Z=z", ignoring line squash! hwdb: improve syntax error messages --- Makefile.am | 3 ++- src/hwdb/hwdb.c | 30 ++++++++++++++++-------------- test/hwdb-test.sh | 23 +++++++++++++++++++++++ test/hwdb/10-bad.hwdb | 26 ++++++++++++++++++++++++++ 4 files changed, 67 insertions(+), 15 deletions(-) create mode 100644 test/hwdb/10-bad.hwdb diff --git a/Makefile.am b/Makefile.am index 124e1867cd..cad4f050af 100644 --- a/Makefile.am +++ b/Makefile.am @@ -1700,7 +1700,8 @@ EXTRA_DIST += \ test/bus-policy/signals.conf \ test/bus-policy/check-own-rules.conf \ test/bus-policy/many-rules.conf \ - test/bus-policy/test.conf + test/bus-policy/test.conf \ + test/hwdb/10-bad.hwdb EXTRA_DIST += \ diff --git a/src/hwdb/hwdb.c b/src/hwdb/hwdb.c index 0fbde28d10..6d4a03bedb 100644 --- a/src/hwdb/hwdb.c +++ b/src/hwdb/hwdb.c @@ -39,7 +39,7 @@ #include "verbs.h" /* - * Generic udev properties, key/value database based on modalias strings. + * Generic udev properties, key-value database based on modalias strings. * Uses a Patricia/radix trie to index all matches for efficient lookup. */ @@ -70,7 +70,7 @@ struct trie_node { struct trie_child_entry *children; uint8_t children_count; - /* sorted array of key/value pairs */ + /* sorted array of key-value pairs */ struct trie_value_entry *values; size_t values_count; }; @@ -81,7 +81,7 @@ struct trie_child_entry { struct trie_node *child; }; -/* value array item with key/value pairs */ +/* value array item with key-value pairs */ struct trie_value_entry { size_t key_off; size_t value_off; @@ -457,10 +457,9 @@ static int insert_data(struct trie *trie, char **match_list, char *line, char *value, **entry; value = strchr(line, '='); - if (!value) { - log_error("Error, key/value pair expected but got '%s' in '%s':", line, filename); - return -EINVAL; - } + if (!value) + return log_syntax(NULL, LOG_WARNING, filename, line_number, EINVAL, + "Key-value pair expected but got \"%s\", ignoring", line); value[0] = '\0'; value++; @@ -469,10 +468,9 @@ static int insert_data(struct trie *trie, char **match_list, char *line, while (isblank(line[0]) && isblank(line[1])) line++; - if (line[0] == '\0' || value[0] == '\0') { - log_error("Error, empty key or value '%s' in '%s':", line, filename); - return -EINVAL; - } + if (line[0] == '\0' || value[0] == '\0') + return log_syntax(NULL, LOG_WARNING, filename, line_number, EINVAL, + "Empty %s in \"%s\", ignoring", line[0] == '\0' ? "key" : "value", line); STRV_FOREACH(entry, match_list) trie_insert(trie, trie->root, *entry, line, value, filename, file_priority, line_number); @@ -524,7 +522,8 @@ static int import_file(struct trie *trie, const char *filename, uint16_t file_pr break; if (line[0] == ' ') { - log_error("Error, MATCH expected but got '%s' in '%s':", line, filename); + log_syntax(NULL, LOG_WARNING, filename, line_number, EINVAL, + "Match expected but got indented property \"%s\", ignoring line", line); break; } @@ -543,7 +542,9 @@ static int import_file(struct trie *trie, const char *filename, uint16_t file_pr case HW_MATCH: if (len == 0) { - log_error("Error, DATA expected but got empty line in '%s':", filename); + log_syntax(NULL, LOG_WARNING, filename, line_number, EINVAL, + "Property expected, ignoring record with no properties"); + state = HW_NONE; strv_clear(match_list); break; @@ -576,7 +577,8 @@ static int import_file(struct trie *trie, const char *filename, uint16_t file_pr } if (line[0] != ' ') { - log_error("Error, DATA expected but got '%s' in '%s':", line, filename); + log_syntax(NULL, LOG_WARNING, filename, line_number, EINVAL, + "Property or empty line expected, got \"%s\", ignoring record", line); state = HW_NONE; strv_clear(match_list); break; diff --git a/test/hwdb-test.sh b/test/hwdb-test.sh index bfdafce998..5373930df4 100755 --- a/test/hwdb-test.sh +++ b/test/hwdb-test.sh @@ -33,6 +33,7 @@ trap "rm -rf '$D'" EXIT INT QUIT PIPE mkdir -p "$D/etc/udev" ln -s "$ROOTDIR/hwdb" "$D/etc/udev/hwdb.d" +# Test "good" properties" — no warnings or errors allowed err=$("$SYSTEMD_HWDB" update --root "$D" 2>&1 >/dev/null) && rc= || rc=$? if [ -n "$err" ]; then echo "$err" @@ -47,3 +48,25 @@ if [ ! -e "$D/etc/udev/hwdb.bin" ]; then echo "$D/etc/udev/hwdb.bin was not generated" exit 1 fi + +# Test "bad" properties" — warnings required, errors not allowed +rm -f "$D/etc/udev/hwdb.bin" "$D/etc/udev/hwdb.d" + +ln -s "$ROOTDIR/test/hwdb" "$D/etc/udev/hwdb.d" +err=$("$SYSTEMD_HWDB" update --root "$D" 2>&1 >/dev/null) && rc= || rc=$? +if [ -n "$rc" ]; then + echo "$SYSTEMD_HWDB returned $rc" + exit $rc +fi +if [ -n "$err" ]; then + echo "Expected warnings" + echo "$err" +else + echo "$SYSTEMD_HWDB unexpectedly printed no warnings" + exit 1 +fi + +if [ ! -e "$D/etc/udev/hwdb.bin" ]; then + echo "$D/etc/udev/hwdb.bin was not generated" + exit 1 +fi diff --git a/test/hwdb/10-bad.hwdb b/test/hwdb/10-bad.hwdb new file mode 100644 index 0000000000..0e1e147323 --- /dev/null +++ b/test/hwdb/10-bad.hwdb @@ -0,0 +1,26 @@ +BAD:1:no properties + +BAD:2:no properties +BAD:2:no properties + +BAD:3:no properties +BAD:3:no properties +BAD:3:no properties + +GOOD:5:bad property + NO_VALUE + +GOOD:6:bad property + =NO_NAME + NO_VALUE= + +BAD:7:match at wrong place + X=Y +BAD:7:match at wrong place + +BAD:8:match at wrong place + X=Y +BAD:8:match at wrong place + Z=z + +BAD:8:match at EOF From a6a2f018bec9087427ba202691db7b663cce932c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Thu, 1 Dec 2016 11:32:41 -0500 Subject: [PATCH 10/11] hwdb: fix detection of assignments with no key The code was trying to detect an empty key, but property lines always start with a space, so the condition was wrong. Now: [/tmp/tmp.YWeKax4fMI/etc/udev/hwdb.d/10-bad.hwdb:14] Empty key in " =NO_NAME", ignoring [/tmp/tmp.YWeKax4fMI/etc/udev/hwdb.d/10-bad.hwdb:15] Empty value in " NO_VALUE=", ignoring --- src/hwdb/hwdb.c | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/src/hwdb/hwdb.c b/src/hwdb/hwdb.c index 6d4a03bedb..d2fe75fc79 100644 --- a/src/hwdb/hwdb.c +++ b/src/hwdb/hwdb.c @@ -456,6 +456,8 @@ static int insert_data(struct trie *trie, char **match_list, char *line, const char *filename, uint16_t file_priority, uint32_t line_number) { char *value, **entry; + assert(line[0] == ' '); + value = strchr(line, '='); if (!value) return log_syntax(NULL, LOG_WARNING, filename, line_number, EINVAL, @@ -464,13 +466,15 @@ static int insert_data(struct trie *trie, char **match_list, char *line, value[0] = '\0'; value++; - /* libudev requires properties to start with a space */ + /* Replace multiple leading spaces by a single space */ while (isblank(line[0]) && isblank(line[1])) line++; - if (line[0] == '\0' || value[0] == '\0') + if (isempty(line + 1) || isempty(value)) return log_syntax(NULL, LOG_WARNING, filename, line_number, EINVAL, - "Empty %s in \"%s\", ignoring", line[0] == '\0' ? "key" : "value", line); + "Empty %s in \"%s=%s\", ignoring", + isempty(line + 1) ? "key" : "value", + line, value); STRV_FOREACH(entry, match_list) trie_insert(trie, trie->root, *entry, line, value, filename, file_priority, line_number); From 7a100dce9db33006888156876ff8aeb27e77eed2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Thu, 1 Dec 2016 11:46:40 -0500 Subject: [PATCH 11/11] hwdb: emit warning when matches are specified at the very end of file This is also an error, but it wasn't caught. [/tmp/tmp.YWeKax4fMI/etc/udev/hwdb.d/10-bad.hwdb:26] Property expected, ignoring record with no properties --- src/hwdb/hwdb.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/src/hwdb/hwdb.c b/src/hwdb/hwdb.c index d2fe75fc79..a23b614791 100644 --- a/src/hwdb/hwdb.c +++ b/src/hwdb/hwdb.c @@ -554,8 +554,8 @@ static int import_file(struct trie *trie, const char *filename, uint16_t file_pr break; } - /* another match */ if (line[0] != ' ') { + /* another match */ match = strdup(line); if (!match) return -ENOMEM; @@ -573,8 +573,8 @@ static int import_file(struct trie *trie, const char *filename, uint16_t file_pr break; case HW_DATA: - /* end of record */ if (len == 0) { + /* end of record */ state = HW_NONE; strv_clear(match_list); break; @@ -593,6 +593,10 @@ static int import_file(struct trie *trie, const char *filename, uint16_t file_pr }; } + if (state == HW_MATCH) + log_syntax(NULL, LOG_WARNING, filename, line_number, EINVAL, + "Property expected, ignoring record with no properties"); + return 0; }