[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