[SSSD] [ding-libs] Patches for #2267, build issue and tight loop
Stephen Gallagher
sgallagh at redhat.com
Mon Apr 7 18:41:26 UTC 2014
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
On 04/07/2014 02:20 PM, Dmitri Pal wrote:
> On 04/07/2014 05:47 AM, Lukas Slebodnik wrote:
>> On (04/04/14 17:47), Dmitri Pal wrote:
>>>>>>> Just a couple minor edits to the language of the
>>>>>>> doxygen:
>>>>>>>
>>>>>>>
>>>>>>> diff --git a/ini/ini_configobj.h b/ini/ini_configobj.h
>>>>>>> index 9a4eb58..4ef98dc 100644 - ---
>>>>>>> a/ini/ini_configobj.h +++ b/ini/ini_configobj.h @@
>>>>>>> -483,6 +483,31 @@ int ini_config_file_open(const char
>>>>>>> *filename, struct ini_cfgfile **file_ctx);
>>>>>>>
>>>>>>> /** + * @brief Create a configuration file object using
>>>>>>> memory buffer.
>>>>>>>
>>>>>>> @brief Create a configuration file object from a
>>>>>>> NULL-terminated buffer.
>>>>>>>
>>>>>>> + * + * Create a file object for parsing a
>>>>>>> configuration file. + * Configuration will use provided
>>>>>>> memory instead of the actual file.
>>>>>>>
>>>>>>> Configuration will use the provided memory buffer
>>>>>>> instead of a real file.
>>>>>>>
>>>>>>> + * + * A "configuration file object" is different
>>>>>>> from + * a "configuration object". The former stores
>>>>>>> metadata + * about a file the configuration data is
>>>>>>> read from,
>>>>>>>
>>>>>>> about a file from which the configuration data is
>>>>>>> read, (this is a very esoteric bit of English)
>>>>>>>
>>>>>>> + * while the latter holds the configuration itself.
>>>>>>>
>>>>>>> while the latter holds the configuration data itself.
>>> I split it into a separate ticket.
>>> https://fedorahosted.org/sssd/ticket/2307 I suspect that there
>>> are other places like this that require some polish. Somone
>>> needs to review doxygen comments and ding-libs and make them
>>> use better English.
>>>
>>>
>>>> + * + * @param[in] data_buf In memory configuration
>>>> data.
>>>>
>>>> In-memory
>>>>
>>>> + * Needs to be NULL
>>>> terminated.
>>>>
>>>> NULL-terminated
>>>>
>>>> + * @param[in] data_len Length of memory data + *
>>>> not including terminating NULL. + * @param[out] file_ctx
>>>> Configuration file object. + * + * @return 0 - Success. + *
>>>> @return EINVAL - Invalid parameter. + * @return ENOMEM - No
>>>> memory. + */ +int ini_config_file_from_mem(void *data_buf, +
>>>> uint32_t data_len, + struct
>>>> ini_cfgfile **file_ctx); +
>>>>
>>>>
>>>>
>>>>
>>>> One additional question: is it expected that data_buf will
>>>> contain NUL values in it other than the terminator? If not,
>>>> why does data_len exist?
>>> Interesting question. Here is how I see it.
>>>
>>> Man page for fmemopen says:
>>>
>>> FILE *fmemopen(void *buf, size_t size, const char *mode);
>>>
>>> ...
>>>
>>> DESCRIPTION The fmemopen() function opens a stream that
>>> permits the access speci- fied by mode. The stream allows I/O
>>> to be performed on the string or memory buffer pointed to
>>> by buf. This buffer must be at least size bytes long.
>>>
>>>
>>> At least! But in reality if it is of the same value the
>>> function does not work. I checked it twice and valgrind
>>> confirmed that it is trying to read past the end of the
>>> buffer.
>>>
>>> So the NULL termination is the attempt to work around the bug
>>> in fmemopen. The caller of the function needs to make sure that
>>> the function would work thus we say to NULL terminate the
>>> buffer.
>>>
>>> I think I need to add another test. This test would use part of
>>> the buffer not the whole one.
>>> https://fedorahosted.org/sssd/ticket/2306
>>>
>>>
>>> Can we push the patches as the thread gets long and hard to
>>> deal with. All comments are either addressed or tracked as
>>> separate tickets.
>>>
>> I am fine with current version but I have another idea. (I know
>> it is little bit late but it is better to ask before pushing
>> patches to public repo)
>>
>> Could we make next function internal: int
>> ini_config_file_from_mem(void *data_buf, uint32_t data_len,
>> struct ini_cfgfile **file_ctx);
>>
>> and public function will be int ini_config_file_from_string(const
>> char *string_config struct ini_cfgfile **file_ctx);
>>
>> I do not know requirements from yelley; so my idea needn't be
>> acceptable.
>>
>> LS
> I prefer not to for a reason. This function would require to call
> strlen() to find the end of the buffer. We do not know anything
> about the caller or how he allocated buffer so we can't call
> strnlen() which is safer. The caller however knows the size of the
> buffer so it is easier for it to get the length in as safe way. In
> many case when you read data from a socket of file you already
> know the size of the data so you do not need to count bytes and
> thus can save cycles.
>
> I am OK of exposing both though I think the second one is extra
> work with not much of a value. Feel free to file an RFE to add the
> second function as a wrapper.
>
I agree with Dmitri here. Certainly not necessary for this patch set.
I'm also fine with getting these patches pushed and the docstrings
cleaned up in a later patch.
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/
iEYEARECAAYFAlNC8VYACgkQeiVVYja6o6OxpgCfSJH4z4rNfkWKeXP3dGcaEdAA
4wkAnje/rmmGfjis+f278c3y0q/fdGGe
=09mF
-----END PGP SIGNATURE-----
More information about the sssd-devel
mailing list