[lttng-dev] [GSOC14]Can I Fix Bug #709 in this way?

[Jérémie Galarneau]

The fix looks good!
Note that the "path" variable must be declared at the beginning of the
function to comply with our coding standards.

Also, please have a look at the contributor's guide[1] and run the
checkpatch.pl script to submit the patch in the appropriate format.

Regards,
Jérémie

[1] https://raw.github.com/jgalar/lttng-doc/master/contributor-guide.txt

On Sun, Mar 16, 2014 at 8:25 AM, Yue Liu <liuyue0310 at gmail.com> wrote:
> Hi,
>     I want to submit some patches before writing my GSOC14 Proposals.
>     For Bug #709, I have read the source code, and found there may be more
> bugs in the func config_get_section_entries()(In src/common/config/config.c
> line 191.):
>         1.No fclose(FILE*) after fopen(FILE*).
>         2.May be the second DEFAULT_DAEMON_HOME_CONFIGPATH macro (config.c
> line 230) is incorrectly written. It should be
> DEFAULT_DAEMON_SYSTEM_CONFIGPATH.
>
>     According the description of Bug #709, I wrote the patches of this bug
> as follows:
>     1.I used func "ini_parse()" instead of "ini_parse_file()",  ini_parse()
> will close the file when it's finished.
>     2.The order of the configuration files is 1) System Wide
> (/etc/lttng/lttng.conf); 2) Local ($HOME/.lttng/lttng.conf); 3) Command line
> argument. Simply use the ini_parese() func to get the config info, and write
> back to filter struct, and update. I hope it will work.
>     3.I move the DBG() and ERR()  to func "ini_parse()"(ini.c), for debug
> and error infos. Just because I don't know where to put it will be better.
>     I don't know whether it is correct and I hope to get some suggestion and
> guidance from you:
>
> diff --git a/src/common/config/config.c b/src/common/config/config.c
> index 04bd2fd..40ae52d 100644
> --- a/src/common/config/config.c
> +++ b/src/common/config/config.c
> @@ -192,53 +192,35 @@ int config_get_section_entries(const char
> *override_path, const char *section,
>   config_entry_handler_cb handler, void *user_data)
>  {
>   int ret = 0;
> - FILE *config_file = NULL;
>   struct handler_filter_args filter = { section, handler, user_data };
>
> - if (override_path) {
> - config_file = fopen(override_path, "r");
> - if (config_file) {
> - DBG("Loaded daemon configuration file at %s",
> - override_path);
> - } else {
> - ERR("Failed to open daemon configuration file at %s",
> - override_path);
> - ret = -ENOENT;
> - goto end;
> - }
> - } else {
> - char *path = utils_get_home_dir();
> +    /* Try to open the system daemon configuration file */
> +    if (access(DEFAULT_DAEMON_SYSTEM_CONFIGPATH, R_OK) == 0) {
> +        ret = ini_parse(DEFAULT_DAEMON_SYSTEM_CONFIGPATH,
> +                (ini_entry_handler) config_entry_handler_filter, (void *)
> &filter);
> +    }
>
> - /* Try to open the user's daemon configuration file */
> - if (path) {
> - ret = asprintf(&path, DEFAULT_DAEMON_HOME_CONFIGPATH, path);
> - if (ret < 0) {
> - goto end;
> - }
> +    char *path = utils_get_home_dir();
>
> - ret = 0;
> - config_file = fopen(path, "r");
> - if (config_file) {
> - DBG("Loaded daemon configuration file at %s", path);
> - }
> +    /* Try to open the user's daemon configuration file */
> +    if (path) {
> +        ret = asprintf(&path, DEFAULT_DAEMON_HOME_CONFIGPATH, path);
> +        if (ret < 0) {
> +            goto end;
> +        }
>
> - free(path);
> - }
> +        if (access(path, R_OK) == 0) {
> +            ret = ini_parse(path,
> +                    (ini_entry_handler) config_entry_handler_filter, (void
> *) &filter);
> +        }
> +        free(path);
> +    }
>
> - /* Try to open the system daemon configuration file */
> - if (!config_file) {
> - config_file = fopen(DEFAULT_DAEMON_HOME_CONFIGPATH, "r");
> - }
> + if (override_path && access(override_path, R_OK) == 0) {
> +        ret = ini_parse(override_path,
> +                (ini_entry_handler) config_entry_handler_filter, (void *)
> &filter);
>   }
>
> - if (!config_file) {
> - DBG("No daemon configuration file found.");
> - goto end;
> - }
> -
> - ret = ini_parse_file(config_file,
> - (ini_entry_handler) config_entry_handler_filter, (void *) &filter);
> -
>  end:
>   return ret;
>  }
> diff --git a/src/common/config/ini.c b/src/common/config/ini.c
> index 7462aeb..f56e3e1 100644
> --- a/src/common/config/ini.c
> +++ b/src/common/config/ini.c
> @@ -35,6 +35,7 @@
>  #include <ctype.h>
>  #include <string.h>
>
> +#include <common/error.h>
>  #include "ini.h"
>
>  #if !INI_USE_STACK
> @@ -206,8 +207,10 @@ int ini_parse(const char* filename, ini_entry_handler
> handler, void* user)
>
>   file = fopen(filename, "r");
>   if (!file) {
> +        ERR("Failed to open daemon configuration file at %s", filename);
>   return -1;
>   }
> +    DBG("Loaded daemon configuration file at %s", filename);
>
>   error = ini_parse_file(file, handler, user);
>   fclose(file);
>
>
> Best Regards,
> Yue Liu
> ----------------------------------------------------------
> Tsinghua University NISL-Lab(FIT-1-213)
> TEL: 156-5279-8191
> Mail: liuyue0310 at gmail.com
>
>
> _______________________________________________
> lttng-dev mailing list
> lttng-dev at lists.lttng.org
> http://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev
>



-- 
Jérémie Galarneau
EfficiOS Inc.
http://www.efficios.com