config: add the logdir option dynamic reloading support #368

lxbsz · 2018-02-02T04:16:07Z

NOTE: for now this couldn't support the glfs's logging setting, and
the glfs will use the old path .

mikechristie · 2018-02-05T22:18:42Z

libtcmu_config.c

-		tcmu_warn("The logdir option is not supported by dynamic reloading for now!\n");
+		/*
+		 * Here we asume that users want to change the
+		 * log_dir_path without considering tha priority


Change "tha" to "the".

mikechristie · 2018-02-05T23:45:47Z

libtcmu_config.c

+		tcmu_logdir_create(cfg->log_dir_path, true);
+		ret = tcmu_create_file_output(tcmu_get_log_level(), TCMU_LOG_FILENAME, true);
+		if (ret < 0)
+			tcmu_err("create file output error \n");


Let's change this to:

Could not change log path to %s.\n, new log dir path

I would say also report the error code, but create_file_output is a mess of -1 and -errno.

mikechristie · 2018-02-05T23:48:38Z

libtcmu_config.c

+		 * mentioned above.
+		 */
+		tcmu_logdir_create(cfg->log_dir_path, true);
+		ret = tcmu_create_file_output(tcmu_get_log_level(), TCMU_LOG_FILENAME, true);


80 char cols.

I think a general issue here is that we do

tcmu_destroy_log()
then
tcmu_destroy_config()

so if during the log resources are freed and destroyed then someone does a update, this would leak and access freed resources.

I think we need to change the ordering or add some new log function that handles checking if the log has been torn down before trying to update it.

Hey, did you handle this last comment? We need to change the shutdown ordering, or do something so if logging is shutdown while the config update is going on, we do not race?

mikechristie · 2018-02-05T23:55:25Z

libtcmu_log.c

+			if (output->close_fn != NULL)
+				output->close_fn(output->data);
+			if (output->name != NULL)
+				free(output->name);


Since this is the same as what's in log_cleanup, let's put it in a function.

mikechristie · 2018-02-06T00:02:20Z

libtcmu_log.c

+
+	/* This will just find the first one and disable it. */
+	darray_foreach(output, logbuf->outputs) {
+		if (output->dest == dest) {


Do we need to also check for enabled == true. This would work:

update 1.

something does tcmu_log()

update 2.

If there were 2 updates back to back

update 1.

update 2.

something does tcmu_log()

then this would only disable the first one twice and the later delayed removal in log_output would only remove the original.

lxbsz · 2018-02-06T03:43:21Z

Updated all. Thanks.

lxbsz · 2018-02-13T05:04:07Z

@mikechristie,
I think I have missed one old comment about the shutdown ordering of destory_log and destory_config and updated it again.

Thanks.

mikechristie · 2018-05-03T15:42:21Z

Sorry. I missed your update. I will fix up this patch and merge for you.

lxbsz · 2018-05-04T09:07:51Z

Hey Mike,
Thanks, I resolved the conflicts, test and update it again :-)

mikechristie · 2018-05-05T20:59:31Z

How about the attached patch?

log-reset-dir.txt

I have done zero testing.

lxbsz · 2018-05-07T02:04:29Z

How about the attached patch?

log-reset-dir.txt

I have done zero testing.

Yeah, looks good.

lxbsz · 2018-05-07T04:42:55Z

@mikechristie
Update with your patch and with 3 fixes.
1, reset to str default values after the options being set
2, reset to boolean default values after the options being set
3, fix about:

diff --git a/libtcmu_log.c b/libtcmu_log.c
index b232d42..70fa28a 100644
--- a/libtcmu_log.c
+++ b/libtcmu_log.c
@@ -727,14 +727,14 @@ int tcmu_logdir_resetup(char *log_dir_path)
        if (!tcmu_logbuf) {
                ret = -ESHUTDOWN;
                goto unlock;
-       }               
+       }
 
        if (!tcmu_logdir_create(log_dir_path, true)) {
                ret = -ENOENT;
                goto unlock;
        }
 
-       ret = tcmu_create_file_output(tcmu_get_log_level(), TCMU_LOG_FILENAME,
+       ret = tcmu_create_file_output(TCMU_LOG_DEBUG, TCMU_LOG_FILENAME,
                                      true);
        if (ret < 0)
                tcmu_err("Could not change log path to %s, ret:%d.\n",

mikechristie · 2018-05-08T16:54:39Z

libtcmu_config.c

 	option = tcmu_get_option(#key); \
 	if (option) { \
-		cfg->key = strdup(option->opt_str); } \
+		cfg->key = strdup(option->opt_str); \
+		if (option->opt_str) \


Do you remember what TCMU_FREE_CFG_STR_KEY was for?

Yeah, there is a memory leak bug introduced ahead of this PR. I will fix it together with this.
Thanks,

lxbsz · 2018-05-24T01:11:25Z

@mikechristie Please review. Thanks.

mikechristie · 2018-05-30T18:34:59Z

libtcmu_config.c

@@ -180,6 +180,7 @@ static void tcmu_conf_free_str_keys(struct tcmu_config *cfg)
 	 * For example:
 	 * TCMU_FREE_CFG_STR_KEY(cfg, 'STR KEY');
 	 */
+	 TCMU_FREE_CFG_STR_KEY(cfg, log_dir_path);


I think if you do the above then you need to fix tcmu_destroy_config:

tcmu_conf_free_str_keys(cfg); if (cfg->log_dir_path) free(cfg->log_dir_path);

because tcmu_conf_free_str_keys call will delete cfg->log-dir_path and then the call above will try to delete the same pointer.

I actually thought the leak was if tcmu_conf_set_options is called twice. The first call would do:

cfg->key = strdup(option->opt_str); \

but then the second call would see the cfg->key is set and we would end up allocating memory again without freeing and leak the old memory at cfg->key. I thought we needed a

if (!cfg->key) cfg->key = strdup(option->opt_str);

Yeah, updated all.
Thanks,

Signed-off-by: Xiubo Li <xiubli@redhat.com>

NOTE: for now this couldn't support the glfs's logging setting, and the glfs will use the old path. Signed-off-by: Xiubo Li <xiubli@redhat.com> Signed-off-by: Mike Christie <mchristi@redhat.com>

Since the syslog won't logged the CDB debug logs, so there is no need to insert them in ringbuffer. Signed-off-by: Xiubo Li <xiubli@redhat.com>

mikechristie reviewed Feb 5, 2018

View reviewed changes

mikechristie reviewed Feb 6, 2018

View reviewed changes

lxbsz force-pushed the logdir_dyn branch 2 times, most recently from e830f34 to 8c08e80 Compare February 6, 2018 03:40

lxbsz force-pushed the logdir_dyn branch from 8c08e80 to 29c68f1 Compare February 13, 2018 04:57

lxbsz force-pushed the logdir_dyn branch from d14d22d to 4c418b7 Compare May 4, 2018 09:05

lxbsz force-pushed the logdir_dyn branch from 4c418b7 to 010e288 Compare May 7, 2018 04:40

lxbsz force-pushed the logdir_dyn branch from 010e288 to 52108c5 Compare May 7, 2018 07:16

mikechristie reviewed May 8, 2018

View reviewed changes

lxbsz force-pushed the logdir_dyn branch from 52108c5 to 4e8f9eb Compare May 9, 2018 01:28

mikechristie reviewed May 30, 2018

View reviewed changes

lxbsz force-pushed the logdir_dyn branch from 4e8f9eb to 8dc1c96 Compare May 31, 2018 04:14

lxbsz added 2 commits May 31, 2018 00:16

config: reset to str default values after the options being set

4e280e0

Signed-off-by: Xiubo Li <xiubli@redhat.com>

config: reset to boolean default values after the options being set

4278522

Signed-off-by: Xiubo Li <xiubli@redhat.com>

lxbsz force-pushed the logdir_dyn branch from 8dc1c96 to c325a7b Compare May 31, 2018 04:16

lxbsz added 3 commits May 31, 2018 00:24

config: fix memory leak of cfg->log_dir_path

6d09e8b

Signed-off-by: Xiubo Li <xiubli@redhat.com>

config: add the logdir option dynamic reloading support

b4caa54

NOTE: for now this couldn't support the glfs's logging setting, and the glfs will use the old path. Signed-off-by: Xiubo Li <xiubli@redhat.com> Signed-off-by: Mike Christie <mchristi@redhat.com>

log: do not insert the CDB debug logs to ringbuffer

1b6e652

Since the syslog won't logged the CDB debug logs, so there is no need to insert them in ringbuffer. Signed-off-by: Xiubo Li <xiubli@redhat.com>

lxbsz force-pushed the logdir_dyn branch from c325a7b to 1b6e652 Compare May 31, 2018 04:24

mikechristie merged commit bb7279b into open-iscsi:master Jun 1, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

config: add the logdir option dynamic reloading support #368

config: add the logdir option dynamic reloading support #368

lxbsz commented Feb 2, 2018 •

edited

mikechristie Feb 5, 2018

mikechristie Feb 5, 2018

mikechristie Feb 5, 2018

mikechristie Feb 6, 2018

mikechristie Feb 12, 2018

mikechristie Feb 5, 2018

mikechristie Feb 6, 2018

lxbsz commented Feb 6, 2018

lxbsz commented Feb 13, 2018

mikechristie commented May 3, 2018

lxbsz commented May 4, 2018

mikechristie commented May 5, 2018 •

edited

lxbsz commented May 7, 2018

lxbsz commented May 7, 2018 •

edited

mikechristie May 8, 2018

lxbsz May 9, 2018

lxbsz commented May 24, 2018

mikechristie May 30, 2018

lxbsz May 31, 2018

config: add the logdir option dynamic reloading support #368

config: add the logdir option dynamic reloading support #368

Conversation

lxbsz commented Feb 2, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lxbsz commented Feb 6, 2018

lxbsz commented Feb 13, 2018

mikechristie commented May 3, 2018

lxbsz commented May 4, 2018

mikechristie commented May 5, 2018 • edited

lxbsz commented May 7, 2018

lxbsz commented May 7, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lxbsz commented May 24, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lxbsz commented Feb 2, 2018 •

edited

mikechristie commented May 5, 2018 •

edited

lxbsz commented May 7, 2018 •

edited