On (07/04/14 14:20), 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. If we want to be secure; we should: 1. allocate array with length "data_len + 1" 2. call memcopy 3. terminate our internal_array with NUL '\0' Because there is still possibility that data_buf needn't be null-terminated even if it is written in documentation.
But it will be the internal change and API wil be the same. >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. No it isn't necessary. yelley explained his use case in another mail https://lists.fedorahosted.org/pipermail/sssd-devel/2014-April/019023.html LS _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel