----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviewboard-openlmi.rhcloud.com/r/842/ -----------------------------------------------------------
Review request for OpenLMI Developers.
Repository: openlmi-providers
Description -------
Account: Added associative thread locking.
Diffs -----
src/account/lock.c PRE-CREATION src/account/lock.h PRE-CREATION src/account/LMI_AccountProvider.c c7c16bb2beb1ef37e804e7cc7dce6bd182e6d55b src/account/CMakeLists.txt 495d7c6e3e1e083541d3ed004518dc4539f61d48
Diff: http://reviewboard-openlmi.rhcloud.com/r/842/diff/
Testing -------
Thanks,
Robin Hack
----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviewboard-openlmi.rhcloud.com/r/842/#review1125 -----------------------------------------------------------
src/account/LMI_AccountProvider.c http://reviewboard-openlmi.rhcloud.com/r/842/#comment653
Does this work? The function returns void, but CMReturn is returning CMPIStatus. I'm not sure if it is possible to do any checks here.
src/account/LMI_AccountProvider.c http://reviewboard-openlmi.rhcloud.com/r/842/#comment654
Is it good to abort here? It should return some error indication rather.
- Robin Hack
On Sept. 10, 2013, 7:29 a.m., Robin Hack wrote:
This is an automatically generated e-mail. To reply, visit: http://reviewboard-openlmi.rhcloud.com/r/842/
(Updated Sept. 10, 2013, 7:29 a.m.)
Review request for OpenLMI Developers.
Repository: openlmi-providers
Description
Account: Added associative thread locking.
Diffs
src/account/lock.c PRE-CREATION src/account/lock.h PRE-CREATION src/account/LMI_AccountProvider.c c7c16bb2beb1ef37e804e7cc7dce6bd182e6d55b src/account/CMakeLists.txt 495d7c6e3e1e083541d3ed004518dc4539f61d48
Diff: http://reviewboard-openlmi.rhcloud.com/r/842/diff/
Testing
Thanks,
Robin Hack
----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviewboard-openlmi.rhcloud.com/r/842/#review1128 -----------------------------------------------------------
src/account/lock.c http://reviewboard-openlmi.rhcloud.com/r/842/#comment657
I think that this function is not pure pure, because changing pointer outside of scope of this function.
- Robin Hack
On Sept. 10, 2013, 7:29 a.m., Robin Hack wrote:
This is an automatically generated e-mail. To reply, visit: http://reviewboard-openlmi.rhcloud.com/r/842/
(Updated Sept. 10, 2013, 7:29 a.m.)
Review request for OpenLMI Developers.
Repository: openlmi-providers
Description
Account: Added associative thread locking.
Diffs
src/account/lock.c PRE-CREATION src/account/lock.h PRE-CREATION src/account/LMI_AccountProvider.c c7c16bb2beb1ef37e804e7cc7dce6bd182e6d55b src/account/CMakeLists.txt 495d7c6e3e1e083541d3ed004518dc4539f61d48
Diff: http://reviewboard-openlmi.rhcloud.com/r/842/diff/
Testing
Thanks,
Robin Hack
----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviewboard-openlmi.rhcloud.com/r/842/#review1140 -----------------------------------------------------------
src/account/lock.c http://reviewboard-openlmi.rhcloud.com/r/842/#comment674
Why GQuark? Looking at its documentation, it copies all strings somewhere and never releases them, so we end up with provider with all user names in memory.
IMHO hash table with full user name as key and g_str_hash() would do the same and we could control where the user name is freed (in free_lock).
Just one issue, otherwise the patch looks good to me.
- Jan Safranek
On Sept. 10, 2013, 9:29 a.m., Robin Hack wrote:
This is an automatically generated e-mail. To reply, visit: http://reviewboard-openlmi.rhcloud.com/r/842/
(Updated Sept. 10, 2013, 9:29 a.m.)
Review request for OpenLMI Developers.
Repository: openlmi-providers
Description
Account: Added associative thread locking.
Diffs
src/account/lock.c PRE-CREATION src/account/lock.h PRE-CREATION src/account/LMI_AccountProvider.c c7c16bb2beb1ef37e804e7cc7dce6bd182e6d55b src/account/CMakeLists.txt 495d7c6e3e1e083541d3ed004518dc4539f61d48
Diff: http://reviewboard-openlmi.rhcloud.com/r/842/diff/
Testing
Thanks,
Robin Hack
On Sept. 10, 2013, 12:07 p.m., Robin Hack wrote:
Just one issue, otherwise the patch looks good to me.
Ok. This is real issue. Copying can be avoided by using g_quark_from_static_string () instead.
What I want is try to avoid hash collision because this is what i found in glib source code:
* Note that the hash functions provided by GLib have these qualities, * but are not particularly robust against manufactured keys that * cause hash collisions. Therefore, you should consider choosing * a more secure hash function when using a GHashTable with keys * that originate in untrusted data (such as HTTP requests). * Using g_str_hash() in that situation might make your application * vulerable to <ulink url="https://lwn.net/Articles/474912/">Algorithmic Complexity Attacks</ulink>.
I use quark to get unique string identifier and use them as key in hash table where pointer to lock_t structure is stored.
Now i know, that i must/should use something which is quark but not quark from glib. Because glib use hash lookup anyway.
quark_from_string (const gchar *string, gboolean duplicate) ... quark = quark_new (duplicate ? quark_strdup (string) : (gchar *)string); ...
- Robin
----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviewboard-openlmi.rhcloud.com/r/842/#review1140 -----------------------------------------------------------
On Sept. 10, 2013, 7:29 a.m., Robin Hack wrote:
This is an automatically generated e-mail. To reply, visit: http://reviewboard-openlmi.rhcloud.com/r/842/
(Updated Sept. 10, 2013, 7:29 a.m.)
Review request for OpenLMI Developers.
Repository: openlmi-providers
Description
Account: Added associative thread locking.
Diffs
src/account/lock.c PRE-CREATION src/account/lock.h PRE-CREATION src/account/LMI_AccountProvider.c c7c16bb2beb1ef37e804e7cc7dce6bd182e6d55b src/account/CMakeLists.txt 495d7c6e3e1e083541d3ed004518dc4539f61d48
Diff: http://reviewboard-openlmi.rhcloud.com/r/842/diff/
Testing
Thanks,
Robin Hack
On Sept. 10, 2013, 12:07 p.m., Robin Hack wrote:
Just one issue, otherwise the patch looks good to me.
Robin Hack wrote: Ok. This is real issue. Copying can be avoided by using g_quark_from_static_string () instead.
What I want is try to avoid hash collision because this is what i found in glib source code: * Note that the hash functions provided by GLib have these qualities, * but are not particularly robust against manufactured keys that * cause hash collisions. Therefore, you should consider choosing * a more secure hash function when using a GHashTable with keys * that originate in untrusted data (such as HTTP requests). * Using g_str_hash() in that situation might make your application * vulerable to <ulink url="https://lwn.net/Articles/474912/">Algorithmic Complexity Attacks</ulink>. I use quark to get unique string identifier and use them as key in hash table where pointer to lock_t structure is stored. Now i know, that i must/should use something which is quark but not quark from glib. Because glib use hash lookup anyway. quark_from_string (const gchar *string, gboolean duplicate) ... quark = quark_new (duplicate ? quark_strdup (string) : (gchar *)string); ...
Missing part of code: if (quark_ht) quark = GPOINTER_TO_UINT (g_hash_table_lookup (quark_ht, string));
- Robin
----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviewboard-openlmi.rhcloud.com/r/842/#review1140 -----------------------------------------------------------
On Sept. 10, 2013, 7:29 a.m., Robin Hack wrote:
This is an automatically generated e-mail. To reply, visit: http://reviewboard-openlmi.rhcloud.com/r/842/
(Updated Sept. 10, 2013, 7:29 a.m.)
Review request for OpenLMI Developers.
Repository: openlmi-providers
Description
Account: Added associative thread locking.
Diffs
src/account/lock.c PRE-CREATION src/account/lock.h PRE-CREATION src/account/LMI_AccountProvider.c c7c16bb2beb1ef37e804e7cc7dce6bd182e6d55b src/account/CMakeLists.txt 495d7c6e3e1e083541d3ed004518dc4539f61d48
Diff: http://reviewboard-openlmi.rhcloud.com/r/842/diff/
Testing
Thanks,
Robin Hack
On Sept. 10, 2013, 2:07 p.m., Robin Hack wrote:
Just one issue, otherwise the patch looks good to me.
Robin Hack wrote: Ok. This is real issue. Copying can be avoided by using g_quark_from_static_string () instead.
What I want is try to avoid hash collision because this is what i found in glib source code: * Note that the hash functions provided by GLib have these qualities, * but are not particularly robust against manufactured keys that * cause hash collisions. Therefore, you should consider choosing * a more secure hash function when using a GHashTable with keys * that originate in untrusted data (such as HTTP requests). * Using g_str_hash() in that situation might make your application * vulerable to <ulink url="https://lwn.net/Articles/474912/">Algorithmic Complexity Attacks</ulink>. I use quark to get unique string identifier and use them as key in hash table where pointer to lock_t structure is stored. Now i know, that i must/should use something which is quark but not quark from glib. Because glib use hash lookup anyway. quark_from_string (const gchar *string, gboolean duplicate) ... quark = quark_new (duplicate ? quark_strdup (string) : (gchar *)string); ...
Robin Hack wrote: Missing part of code: if (quark_ht) quark = GPOINTER_TO_UINT (g_hash_table_lookup (quark_ht, string));
/me studies the Algorithmic Complexity Attacks.
It looks to me that in the worst case GHashTable degrades to GSList (or whatever glib uses for "buckets"). Is it that bad?
- Jan
----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviewboard-openlmi.rhcloud.com/r/842/#review1140 -----------------------------------------------------------
On Sept. 10, 2013, 9:29 a.m., Robin Hack wrote:
This is an automatically generated e-mail. To reply, visit: http://reviewboard-openlmi.rhcloud.com/r/842/
(Updated Sept. 10, 2013, 9:29 a.m.)
Review request for OpenLMI Developers.
Repository: openlmi-providers
Description
Account: Added associative thread locking.
Diffs
src/account/lock.c PRE-CREATION src/account/lock.h PRE-CREATION src/account/LMI_AccountProvider.c c7c16bb2beb1ef37e804e7cc7dce6bd182e6d55b src/account/CMakeLists.txt 495d7c6e3e1e083541d3ed004518dc4539f61d48
Diff: http://reviewboard-openlmi.rhcloud.com/r/842/diff/
Testing
Thanks,
Robin Hack
----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviewboard-openlmi.rhcloud.com/r/842/#review1141 -----------------------------------------------------------
src/account/lock.c http://reviewboard-openlmi.rhcloud.com/r/842/#comment675
I don't see any char* here
src/account/lock.c http://reviewboard-openlmi.rhcloud.com/r/842/#comment676
It seems some code is missing here...
- Jan Safranek
On Sept. 10, 2013, 9:29 a.m., Robin Hack wrote:
This is an automatically generated e-mail. To reply, visit: http://reviewboard-openlmi.rhcloud.com/r/842/
(Updated Sept. 10, 2013, 9:29 a.m.)
Review request for OpenLMI Developers.
Repository: openlmi-providers
Description
Account: Added associative thread locking.
Diffs
src/account/lock.c PRE-CREATION src/account/lock.h PRE-CREATION src/account/LMI_AccountProvider.c c7c16bb2beb1ef37e804e7cc7dce6bd182e6d55b src/account/CMakeLists.txt 495d7c6e3e1e083541d3ed004518dc4539f61d48
Diff: http://reviewboard-openlmi.rhcloud.com/r/842/diff/
Testing
Thanks,
Robin Hack
----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviewboard-openlmi.rhcloud.com/r/842/ -----------------------------------------------------------
(Updated Sept. 10, 2013, 1:07 p.m.)
Review request for OpenLMI Developers.
Changes -------
Get rid of quarks. Back to hash.
Summary (updated) -----------------
[5/5] Account: Account: Added associative thread locking.
Repository: openlmi-providers
Description (updated) -------
Account: Account: Added associative thread locking.
Get rid of quarks. Use hash for strings.
Diffs (updated) -----
src/account/lock.h PRE-CREATION src/account/lock.c PRE-CREATION
Diff: http://reviewboard-openlmi.rhcloud.com/r/842/diff/
Testing -------
Thanks,
Robin Hack
----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviewboard-openlmi.rhcloud.com/r/842/ -----------------------------------------------------------
(Updated Sept. 10, 2013, 1:11 p.m.)
Review request for OpenLMI Developers.
Summary (updated) -----------------
[5/5] Account: Added associative thread locking.
Repository: openlmi-providers
Description -------
Account: Account: Added associative thread locking.
Get rid of quarks. Use hash for strings.
Diffs -----
src/account/lock.h PRE-CREATION src/account/lock.c PRE-CREATION
Diff: http://reviewboard-openlmi.rhcloud.com/r/842/diff/
Testing -------
Thanks,
Robin Hack
----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviewboard-openlmi.rhcloud.com/r/842/ -----------------------------------------------------------
(Updated Sept. 10, 2013, 1:26 p.m.)
Status ------
This change has been discarded.
Review request for OpenLMI Developers.
Repository: openlmi-providers
Description -------
Account: Account: Added associative thread locking.
Get rid of quarks. Use hash for strings.
Diffs -----
src/account/lock.h PRE-CREATION src/account/lock.c PRE-CREATION
Diff: http://reviewboard-openlmi.rhcloud.com/r/842/diff/
Testing -------
Thanks,
Robin Hack
----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviewboard-openlmi.rhcloud.com/r/842/#review1145 -----------------------------------------------------------
src/account/lock.c http://reviewboard-openlmi.rhcloud.com/r/842/#comment677
Why #undefine ???
- Jan Safranek
On Sept. 10, 2013, 3:26 p.m., Robin Hack wrote:
This is an automatically generated e-mail. To reply, visit: http://reviewboard-openlmi.rhcloud.com/r/842/
(Updated Sept. 10, 2013, 3:26 p.m.)
Review request for OpenLMI Developers.
Repository: openlmi-providers
Description
Account: Account: Added associative thread locking.
Get rid of quarks. Use hash for strings.
Diffs
src/account/lock.h PRE-CREATION src/account/lock.c PRE-CREATION
Diff: http://reviewboard-openlmi.rhcloud.com/r/842/diff/
Testing
Thanks,
Robin Hack
openlmi-reviews@lists.fedorahosted.org