[Freeipa-devel] [PATCH 0077] Refactor settings subsystem
Petr Spacek
pspacek at redhat.com
Mon Mar 4 14:46:39 UTC 2013
Hello,
amended patch is attached.
On 4.3.2013 14:58, Adam Tkac wrote:
> On Tue, Feb 19, 2013 at 02:34:49PM +0100, Petr Spacek wrote:
>> >On 8.10.2012 15:29, Petr Spacek wrote:
>>> > >Hello,
>>> > >
>>> > >this patch refactors setting subsystem. After some experimenting I chosen
>>> > >simple implementation with static arrays, no RBT trees involved. Speed can be
>>> > >improved by reordering items in arrays.
>>> > >
>>> > >Commit message:
>>> > >
>>> > > Refactor settings subsystem.
>>> > >
>>> > > Settings are stored in tree of settings_set_t structures.
>>> > > All settings should be accessed only through setting* functions.
>>> > > Mutual exclusion during write is done by switching to single
>>> > > thread mode.
>>> > >
>>> > > Setting_get() function doesn't copy strings, so changing the
>>> > > original string can lead to obscure bugs.
>>> > > This way is okay as long as strings are not changed dynamically
>>> > > at run-time.
>>> > >
>>> > > Unknown setting in configuration file leads to failure rather than
>>> > > silent ignoring it.
>>> > >
>>> > > https://fedorahosted.org/bind-dyndb-ldap/ticket/53
>>> > > https://fedorahosted.org/bind-dyndb-ldap/ticket/81
>> >
>> >Re-based & amended & tested patch is attached. The main idea is the same.
>> >
>>> > >Adam, I'm still looking for way how to handle strings in settings. We have to
>>> > >prevent string change/deallocation as long as somebody has a pointer to the
>>> > >string (I mean pointer obtained through setting_get("name_of_setting") call).
>>> > >
>>> > >The only way which I can see is returning setting_string structure like
>>> > >
>>> > >setting_string struct {
>>> > > isc_refcount_t counter;
>>> > > isc_mem_t *mctx;
>>> > > char *str;
>>> > >};
>> >
>> >After some poking to isc_task_beginexclusive() I consider the
>> >"struct setting_string" idea redundant.
>> >
>>> > >Caller has to call setting_string_free() when string can be freed.
>>> > >Setting_string_free() will decrement counter by one and free whole structure
>>> > >if and only if counter reaches 0.
>>> > >
>>> > >Is it meaningful? Should I separate setting_get_* for each datatype? Or just
>>> > >for setting_get_string() and let setting_get() universal for integers and
>>> > >booleans?
>> >
>> >I separated setting_get_bool/uint/str from the universal
>> >setting_get() to enable type checks.
> Hi Peter, check my comments below, please. I found just some minor issues, the
> patch overally looks fine.
>
> Regards, Adam
>
>> > From ecff7be859f7c53c167b7ccd4d4d0cc2dfc990a6 Mon Sep 17 00:00:00 2001
>> >From: Petr Spacek<pspacek at redhat.com>
>> >Date: Thu, 4 Oct 2012 16:44:16 +0200
>> >Subject: [PATCH] Refactor settings subsystem.
>> >
>> >Settings are stored in tree of settings_set_t structures.
>> >All settings should be accessed only through setting* functions.
>> >Mutual exclusion during write is done by switching to single
>> >thread mode.
>> >
>> >setting_get_str() function doesn't copy strings, so original strings
>> >have to stay unchanged.
>> >
>> >Unknown setting and duplicate settings in configuration file
>> >lead to failure rather than silent ignoring them.
>> >
>> >https://fedorahosted.org/bind-dyndb-ldap/ticket/53
>> >https://fedorahosted.org/bind-dyndb-ldap/ticket/81
>> >
>> >Signed-off-by: Petr Spacek<pspacek at redhat.com>
<snip>
>> >+/**
>> >+ * Get value associated with a setting. Search starts in set of settings
>> >+ * passed by caller and continues in parent sets until the setting with defined
>> >+ * value is found.
>> >+ *
>> >+ * @warning
>> >+ * This function is not expected to fail because all settings should
>> >+ * have default value defined (in topmost set of settings).
>> >+ * Caller should always check the return value, regardless this assumption.
>> >+ *
>> >+ * @param[out] target Type of pointer must agree with requested setting type.
>> >+ * @retval ISC_R_SUCCESS Required value was found and target was filled in.
>> >+ * @retval ISC_R_NOTFOUND Value is not defined in specified set of
>> >+ * settings either in parent sets.
>> >+ * @retval ISC_R_UNEXPECTED Type mismatch between _get_type() function and type
>> >+ * of setting in settings tree. (I.e. programming
>> >+ * error.)
>> >+ */
>> >+/* This hack enables type checking for setting_get_ functions. */
>> >+#define setting_getter(func_name, c_type, enum_type, value_name) \
>> >+isc_result_t \
>> >+setting_get_##func_name(const char * const name, \
>> >+ const settings_set_t * const set, \
>> >+ c_type target) \
>> >+{ \
>> >+ isc_result_t result; \
>> >+ setting_t *setting = NULL; \
>> >+ \
>> >+ REQUIRE(name != NULL); \
>> >+ REQUIRE(target != NULL); \
>> >+ \
>> >+ CHECK(setting_find(name, set, ISC_TRUE, ISC_TRUE, &setting)); \
>> >+ \
>> >+ if (setting->type != enum_type) { \
>> >+ log_bug("incompatible setting data type requested " \
>> >+ "for name '%s' in set of settings '%s'", \
>> >+ name, set->name); \
>> >+ return ISC_R_UNEXPECTED; \
>> >+ } \
>> >+ *target = setting->value.value_name; \
>> >+ \
>> >+ return ISC_R_SUCCESS; \
>> >+ \
>> >+cleanup: \
>> >+ log_bug("setting '%s' was not found in settings tree", name); \
>> >+ return result; \
>> >+}
>> >+
>> >+setting_getter(uint, isc_uint32_t *, ST_UNSIGNED_INTEGER, value_uint)
>> >+setting_getter(str, const char **, ST_STRING, value_char)
>> >+setting_getter(bool, isc_boolean_t *, ST_BOOLEAN, value_boolean)
> I don't like this part of code much. When you declare setting_get* functions
> this way, you end with 3 copies of nearly same code which leads to bloat of
> ldap.so.
>
> In my opinion just one function with switch (enum_type) {} inside which returns
> void* might be better. After that we can declare wrappers which will re-type
> void* to appropriate type. Example:
>
> isc_result_t
> setting_get(const char *name, const settings_set_t *set, enum_type, void *target);
>
> #define setting_get_uint(name, set, target) setting_get(name, set, ST_UNSIGNED_INTEGER, target);
>
> This way we will end with only one copy of setting_get function.
Ok. I created one generic setting_get function and added one-line function for
each data type. It should allow compiler to do proper type checking and
compiler will in-line it, I guess.
<snip>
>> > struct setting {
>> > const char *name;
>> >- int set;
>> >- int has_a_default;
>> > setting_type_t type;
>> > union {
>> >- const char *value_char;
>> >- signed int value_sint;
>> >- unsigned int value_uint;
>> >+ char *value_char;
>> >+ isc_uint32_t value_uint;
>> > isc_boolean_t value_boolean;
>> >- } default_value;
>> >- void *target;
>> >+ } value;
>> >+ isc_boolean_t filled;
>> >+ isc_boolean_t allocated_dynamically;
> This is quite long name, isn't it? What about "dynamic" or "isdynamic"?
I vote for is_dynamic :-)
--
Petr^2 Spacek
-------------- next part --------------
A non-text attachment was scrubbed...
Name: bind-dyndb-ldap-pspacek-0077-3-Refactor-settings-subsystem.patch
Type: text/x-patch
Size: 90360 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20130304/88c1c8d3/attachment.bin>
More information about the Freeipa-devel
mailing list