Hey Peter,
Peter Hutterer said the following on 01/31/2013 04:55 AM:
On Wed, Jan 30, 2013 at 02:03:59PM +0100, Olivier Fourdan wrote:
[...]
So I think this adding this check here is not sufficient. Ideally,
this should be done when loading the database (also because people
could add new definitions without rebuilding and running "make
check").
right, and that's where we have to do it anyway since we otherwise won't
notice (as one gets overwritten). How about the following patch then?
I opted against g_assert() to give the caller a chance to clean up before
exiting rather than just killing it.
Agreed, although we return a NULL db so the caller is most likely to
dislike that it ;-)
diff --git a/libwacom/libwacom-database.c b/libwacom/libwacom-database.c
index 9813a4e..3eb65df 100644
--- a/libwacom/libwacom-database.c
+++ b/libwacom/libwacom-database.c
@@ -564,6 +564,13 @@ libwacom_database_new_for_path (const char *datadir)
for (match = matches; *match; match++) {
const char *matchstr;
matchstr = libwacom_match_get_match_string(*match);
+ /* no duplicate matches allowed */
+ if (g_hash_table_contains(db->device_ht, matchstr)) {
g_hash_table_contains() was added in glib 2.32 which is still fairly recent.
I'd rather use g_hash_table_lookup() which is available in a much wider range
of versions of glib.
Performance wise, it's the same, both g_hash_table_contains() and
g_hash_table_lookup() end up calling g_hash_table_lookup_node() internally.
+ g_critical("Duplicate match of '%s' on device
'%s'.",
+ matchstr, libwacom_get_name(d));
+ libwacom_database_destroy(db);
+ return NULL;
+ }
Aren't you leaking files here?
What about the attached variant of your patch?
Cheers,
Olivier.
--
əɔıʌəp əɯos ɥʇıʍ əɹəɥʍəɯos ɯoɹɟ ʇuəs
>From 4811cd5255eaa095a4c9a37ddca909994b319cc2 Mon Sep 17 00:00:00 2001
From: Peter Hutterer <peter.hutte...@who-t.net>
Date: Thu, 31 Jan 2013 09:32:50 +0100
Subject: [PATCH] libwacom: bail out if a duplicate match string is found
If two devices use the same match string, something is wrong.
Log a critical error and return a NULL database.
Signed-off-by: Peter Hutterer <peter.hutte...@who-t.net>
---
libwacom/libwacom-database.c | 19 ++++++++++++++-----
1 files changed, 14 insertions(+), 5 deletions(-)
diff --git a/libwacom/libwacom-database.c b/libwacom/libwacom-database.c
index 9813a4e..14b5a4a 100644
--- a/libwacom/libwacom-database.c
+++ b/libwacom/libwacom-database.c
@@ -564,6 +564,14 @@ libwacom_database_new_for_path (const char *datadir)
for (match = matches; *match; match++) {
const char *matchstr;
matchstr = libwacom_match_get_match_string(*match);
+ /* no duplicate matches allowed */
+ if (g_hash_table_lookup(db->device_ht, matchstr) != NULL) {
+ g_critical("Duplicate match of '%s' on device '%s'.",
+ matchstr, libwacom_get_name(d));
+ libwacom_database_destroy(db);
+ db = NULL;
+ goto end;
+ }
g_hash_table_insert (db->device_ht, g_strdup (matchstr), d);
d->refcnt++;
}
@@ -591,17 +599,18 @@ libwacom_database_new_for_path (const char *datadir)
g_free(path);
}
- while(nfiles--)
- free(files[nfiles]);
- free(files);
-
/* If we couldn't load _anything_ then something's wrong */
if (g_hash_table_size (db->device_ht) == 0 &&
g_hash_table_size (db->stylus_ht) == 0) {
libwacom_database_destroy(db);
- return NULL;
+ db = NULL;
}
+end:
+ while(nfiles--)
+ free(files[nfiles]);
+ free(files);
+
return db;
}
--
1.7.1
------------------------------------------------------------------------------
Everyone hates slow websites. So do we.
Make your web apps faster with AppDynamics
Download AppDynamics Lite for free today:
http://p.sf.net/sfu/appdyn_d2d_jan
_______________________________________________
Linuxwacom-devel mailing list
Linuxwacom-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxwacom-devel
