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