-----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----- _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
