Re: [Linuxwacom-devel] [PATCH libwacom] test: check for duplicate matching IDs
On Thu, Jan 31, 2013 at 09:36:30AM +0100, Olivier Fourdan wrote:
> 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?
works for me. applied, thanks.
Cheers,
Peter
> From 4811cd5255eaa095a4c9a37ddca909994b319cc2 Mon Sep 17 00:00:00 2001
> From: Peter Hutterer
> 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
> ---
> 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
Re: [Linuxwacom-devel] [PATCH libwacom] test: check for duplicate matching IDs
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
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
---
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
Re: [Linuxwacom-devel] [PATCH libwacom] test: check for duplicate matching IDs
On Wed, Jan 30, 2013 at 02:03:59PM +0100, Olivier Fourdan wrote:
>
> Hey PEter,
>
> Peter Hutterer said the following on 01/30/2013 05:27 AM:
> >No two device description files should match on the same ID.
> >
> >Since a tablet may be listed multiple times in the database if it has more
> >than one match, we can't assume the matches are unique, we need to compare
> >the device name.
> >
> >Build hashtable of {match string : device name}, then check for each new
> >existing match string if the device name is identical.
>
> I tried that patch by adding both the new definition from Pander for
> the CTH-661 and my own patch, expecting it to detect the duplicate
> definition and bail out (see the thread about adding support for the
> "libwacom CTH-661/L" and the discussion with Bastien).
>
> The case here is:
>
> - A tablet definition listing mutliple matches,
> DeviceMatch=usb:056a:00d3;usb:056a:00db under the name Name=Wacom
> Bamboo 2FG 6x8
> - Another tablet definition adding a single duplicate match,
> DeviceMatch=usb:056a:00db under a dfferent name, Name=Wacom Bamboo
> 4FG 6x8 SE NL
>
> I was expecting this to trigger a failure. But it did not happen,
> "make check" ended happily ever after, so I wondered why.
hah, you're right. somehow all my test-cases used the duplicated on a
multiple match and didn't pick up this simple case.
> When loading a tablet definition, libwacom_database_new_for_path()
> will add all the matches given for that tablet in a hash table whose
> key is the match.
>
> So if the duplicate file is loaded first, its entry in the hash
> table will get replaced by the other definition loaded later, no
> dupe in database, "make check" is happy (while I was expecting it
> not to be).
>
> If the duplicate file is loaded after, then only the match for that
> given /other/ entry is replaced, "make check" should fail.
>
> 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.
>From 330e89be2713d483ad72ef6b72743588f14c68b2 Mon Sep 17 00:00:00 2001
From: Peter Hutterer
Date: Thu, 31 Jan 2013 13:52:39 +1000
Subject: [PATCH libwacom] 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
---
libwacom/libwacom-database.c | 7 +++
1 file changed, 7 insertions(+)
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_critical("Duplicate match of '%s' on device
'%s'.",
+ matchstr, libwacom_get_name(d));
+ libwacom_database_destroy(db);
+ return NULL;
+ }
g_hash_table_insert (db->device_ht, g_strdup (matchstr), d);
d->refcnt++;
}
--
1.8.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
Re: [Linuxwacom-devel] [PATCH libwacom] test: check for duplicate matching IDs
Hey PEter,
Peter Hutterer said the following on 01/30/2013 05:27 AM:
> No two device description files should match on the same ID.
>
> Since a tablet may be listed multiple times in the database if it has more
> than one match, we can't assume the matches are unique, we need to compare
> the device name.
>
> Build hashtable of {match string : device name}, then check for each new
> existing match string if the device name is identical.
I tried that patch by adding both the new definition from Pander for
the CTH-661 and my own patch, expecting it to detect the duplicate
definition and bail out (see the thread about adding support for the
"libwacom CTH-661/L" and the discussion with Bastien).
The case here is:
- A tablet definition listing mutliple matches,
DeviceMatch=usb:056a:00d3;usb:056a:00db under the name Name=Wacom
Bamboo 2FG 6x8
- Another tablet definition adding a single duplicate match,
DeviceMatch=usb:056a:00db under a dfferent name, Name=Wacom Bamboo 4FG
6x8 SE NL
I was expecting this to trigger a failure. But it did not happen,
"make check" ended happily ever after, so I wondered why.
When loading a tablet definition, libwacom_database_new_for_path()
will add all the matches given for that tablet in a hash table whose
key is the match.
So if the duplicate file is loaded first, its entry in the hash table
will get replaced by the other definition loaded later, no dupe in
database, "make check" is happy (while I was expecting it not to be).
If the duplicate file is loaded after, then only the match for that
given /other/ entry is replaced, "make check" should fail.
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").
Cheers,
Olivier.
--
əɔıʌəp əɯos ɥʇıʍ əɹəɥʍəɯos ɯoɹɟ ʇuəs
--
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
