diff options
author | Kaleb S. KEITHLEY <kkeithle@redhat.com> | 2012-06-13 09:13:04 -0400 |
---|---|---|
committer | Anand Avati <avati@redhat.com> | 2012-06-29 14:08:14 -0700 |
commit | 5672e77d3102a990a2aa11e7e56ebfe6a0eee369 (patch) | |
tree | c92612a433383c70a9e4557ddaffbcdc348e0921 /cli | |
parent | d87bd36040128c6553e8ee06a363eeb60d16e72c (diff) |
localtime and ctime are not MT-SAFE
There are a number of nit-level issues throughout the source with
the use of localtime and ctime. While they apparently aren't causing
too many problems, apart from the one in bz 828058, they ought to be
fixed. Among the "real" problems that are fixed in this patch:
1) general localtime and ctime not MT-SAFE. There's a non-zero chance
that another thread calling localtime (or ctime) will over-write
the static data about to be used in another thread
2) localtime(& <64-bit-type>) or ctime(& <64-bit-type>) generally
not a problem on 64-bit or little-endian 32-bit. But even though
we probably have zero users on big-ending 32-bit platforms, it's
still incorrect.
3) multiple nested calls passed as params. Last one wins, i.e. over-
writes result of prior calls.
4) Inconsistent error handling. Most of these calls are for logging,
tracing, or dumping. I submit that if an error somehow occurs in
the call to localtime or ctime, the log/trace/dump still should
still occur.
5) Appliances should all have their clocks set to UTC, and all log
entries, traces, and dumps should use GMT.
6) fix strtok(), change to strtok_r()
Other things this patch fixes/changes (that aren't bugs per se):
1) Change "%Y-%m-%d %H:%M:%S" and similar to their equivalent shorthand,
e.g. "%F %T"
2) change sizeof(timestr) to sizeof timestr. sizeof is an operator,
not a function. You don't use i +(32), why use sizeof(<var>).
(And yes, you do use parens with sizeof(<type>).)
3) change 'char timestr[256]' to 'char timestr[32]' where appropriate.
Per-thread stack is limited. Time strings are never longer than ~20
characters, so why waste 220+ bytes on the stack?
Things this patch doesn't fix:
1) hodgepodge of %Y-%m-%d %H:%M:%S versus %Y/%m/%d-%H%M%S and other
variations. It's not clear to me whether this ever matters, not to
mention 3rd party log filtering tools may already rely on a
particular format. Still it would be nice to have a single manifest
constant and have every call to localtime/strftime consistently use
the same format.
Change-Id: I827cad7bf53e57b69c0173f67abe72884249c1a9
BUG: 832173
Signed-off-by: Kaleb S. KEITHLEY <kkeithle@redhat.com>
Reviewed-on: http://review.gluster.com/3568
Tested-by: Gluster Build System <jenkins@build.gluster.com>
Reviewed-by: Anand Avati <avati@redhat.com>
Diffstat (limited to 'cli')
-rw-r--r-- | cli/src/cli-cmd-parser.c | 10 | ||||
-rw-r--r-- | cli/src/cli-rpc-ops.c | 54 | ||||
-rw-r--r-- | cli/src/cli-xml-output.c | 24 | ||||
-rw-r--r-- | cli/src/cli.c | 6 |
4 files changed, 36 insertions, 58 deletions
diff --git a/cli/src/cli-cmd-parser.c b/cli/src/cli-cmd-parser.c index 931d89ae1..8c16588ae 100644 --- a/cli/src/cli-cmd-parser.c +++ b/cli/src/cli-cmd-parser.c @@ -1581,12 +1581,10 @@ cli_cmd_gsync_set_parse (const char **words, int wordcount, dict_t **options) if (strcmp (words[cmdi + 1], "checkpoint") == 0 && strcmp (append_str, "now") == 0) { struct timeval tv = {0,}; - struct tm *tm = NULL; ret = gettimeofday (&tv, NULL); if (ret == -1) - goto out; - tm = localtime (&tv.tv_sec); + goto out; /* FIXME: free append_str? */ GF_FREE (append_str); append_str = GF_CALLOC (1, 300, cli_mt_append_str); @@ -1595,9 +1593,9 @@ cli_cmd_gsync_set_parse (const char **words, int wordcount, dict_t **options) goto out; } strcpy (append_str, "as of "); - strftime (append_str + strlen ("as of "), - 300 - strlen ("as of "), - "%Y-%m-%d %H:%M:%S", tm); + gf_time_fmt (append_str + strlen ("as of "), + 300 - strlen ("as of "), + tv.tv_sec, gf_timefmt_FT); } ret = dict_set_dynstr (dict, "op_value", append_str); diff --git a/cli/src/cli-rpc-ops.c b/cli/src/cli-rpc-ops.c index 0b64758fa..492be4388 100644 --- a/cli/src/cli-rpc-ops.c +++ b/cli/src/cli-rpc-ops.c @@ -4069,15 +4069,15 @@ gf_cli3_1_top_volume_cbk (struct rpc_req *req, struct iovec *iov, { gf_cli_rsp rsp = {0,}; int ret = -1; - dict_t *dict = NULL; + dict_t *dict = NULL; gf1_cli_stats_op op = GF_CLI_STATS_NONE; char key[256] = {0}; int i = 0; int32_t brick_count = 0; char brick[1024]; int32_t members = 0; - char *filename; - char *bricks; + char *filename; + char *bricks; uint64_t value = 0; int32_t j = 0; gf1_cli_top_op top_op = GF_CLI_TOP_NONE; @@ -4085,11 +4085,10 @@ gf_cli3_1_top_volume_cbk (struct rpc_req *req, struct iovec *iov, uint64_t max_nr_open = 0; double throughput = 0; double time = 0; - long int time_sec = 0; + int32_t time_sec = 0; long int time_usec = 0; - struct tm *tm = NULL; char timestr[256] = {0, }; - char *openfd_str = NULL; + char *openfd_str = NULL; gf_boolean_t nfs = _gf_false; gf_boolean_t clear_stats = _gf_false; int stats_cleared = 0; @@ -4262,11 +4261,9 @@ gf_cli3_1_top_volume_cbk (struct rpc_req *req, struct iovec *iov, ret = dict_get_int32 (dict, key, (int32_t *)&time_usec); if (ret) goto out; - tm = localtime (&time_sec); - if (!tm) - goto out; - strftime (timestr, 256, "%Y-%m-%d %H:%M:%S", tm); - snprintf (timestr + strlen (timestr), 256 - strlen (timestr), + gf_time_fmt (timestr, sizeof timestr, + time_sec, gf_timefmt_FT); + snprintf (timestr + strlen (timestr), sizeof timestr - strlen (timestr), ".%"GF_PRI_SUSECONDS, time_usec); if (strlen (filename) < VOL_TOP_PERF_FILENAME_DEF_WIDTH) cli_out ("%*"PRIu64" %-*s %-*s", @@ -5854,52 +5851,43 @@ cmd_heal_volume_brick_out (dict_t *dict, int brick) uint64_t num_entries = 0; int ret = 0; char key[256] = {0}; - char *hostname = NULL; - char *path = NULL; - char *status = NULL; + char *hostname = NULL; + char *path = NULL; + char *status = NULL; uint64_t i = 0; uint32_t time = 0; - char timestr[256] = {0}; - struct tm tm = {0}; - time_t ltime = 0; + char timestr[32] = {0}; - snprintf (key, sizeof (key), "%d-hostname", brick); + snprintf (key, sizeof key, "%d-hostname", brick); ret = dict_get_str (dict, key, &hostname); if (ret) goto out; - snprintf (key, sizeof (key), "%d-path", brick); + snprintf (key, sizeof key, "%d-path", brick); ret = dict_get_str (dict, key, &path); if (ret) goto out; cli_out ("\nBrick %s:%s", hostname, path); - snprintf (key, sizeof (key), "%d-count", brick); + snprintf (key, sizeof key, "%d-count", brick); ret = dict_get_uint64 (dict, key, &num_entries); cli_out ("Number of entries: %"PRIu64, num_entries); - snprintf (key, sizeof (key), "%d-status", brick); + snprintf (key, sizeof key, "%d-status", brick); ret = dict_get_str (dict, key, &status); if (status && strlen (status)) cli_out ("Status: %s", status); for (i = 0; i < num_entries; i++) { - snprintf (key, sizeof (key), "%d-%"PRIu64, brick, i); + snprintf (key, sizeof key, "%d-%"PRIu64, brick, i); ret = dict_get_str (dict, key, &path); if (ret) continue; time = 0; - snprintf (key, sizeof (key), "%d-%"PRIu64"-time", brick, i); + snprintf (key, sizeof key, "%d-%"PRIu64"-time", brick, i); ret = dict_get_uint32 (dict, key, &time); if (!time) { cli_out ("%s", path); } else { - ltime = time; - memset (&tm, 0, sizeof (tm)); - if (!localtime_r (<ime, &tm)) { - snprintf (timestr, sizeof (timestr), - "Invalid time"); - } else { - strftime (timestr, sizeof (timestr), - "%Y-%m-%d %H:%M:%S", &tm); - } - if (i ==0) { + gf_time_fmt (timestr, sizeof timestr, + time, gf_timefmt_FT); + if (i == 0) { cli_out ("at path on brick"); cli_out ("-----------------------------------"); } diff --git a/cli/src/cli-xml-output.c b/cli/src/cli-xml-output.c index 702a7f7fe..510ad0103 100644 --- a/cli/src/cli-xml-output.c +++ b/cli/src/cli-xml-output.c @@ -1429,14 +1429,13 @@ int cli_xml_output_vol_top_rw_perf (xmlTextWriterPtr writer, dict_t *dict, int brick_index, int member_index) { - int ret = -1; - char *filename = NULL; - uint64_t throughput = 0; - long int time_sec = 0; - long int time_usec = 0; - struct tm *tm = NULL; - char timestr[256] = {0,}; - char key[1024] = {0,}; + int ret = -1; + char *filename = NULL; + uint64_t throughput = 0; + long int time_sec = 0; + long int time_usec = 0; + char timestr[256] = {0,}; + char key[1024] = {0,}; /* <file> */ ret = xmlTextWriterStartElement (writer, (xmlChar *)"file"); @@ -1474,14 +1473,9 @@ cli_xml_output_vol_top_rw_perf (xmlTextWriterPtr writer, dict_t *dict, if (ret) goto out; - tm = localtime (&time_sec); - if (!tm) { - ret = -1; - goto out; - } - strftime (timestr, sizeof (timestr), "%Y-%m-%d %H:%M:%S", tm); + gf_time_fmt (timestr, sizeof timestr, time_sec, gf_timefmt_FT); snprintf (timestr + strlen (timestr), - sizeof (timestr) - strlen (timestr), + sizeof timestr - strlen (timestr), ".%"GF_PRI_SUSECONDS, time_usec); ret = xmlTextWriterWriteFormatElement (writer, (xmlChar *)"time", "%s", timestr); diff --git a/cli/src/cli.c b/cli/src/cli.c index 0d66ff282..16e434f0f 100644 --- a/cli/src/cli.c +++ b/cli/src/cli.c @@ -108,7 +108,6 @@ generate_uuid () char tmp_str[1024] = {0,}; char hostname[256] = {0,}; struct timeval tv = {0,}; - struct tm now = {0, }; char now_str[32]; if (gettimeofday (&tv, NULL) == -1) { @@ -123,9 +122,8 @@ generate_uuid () strerror (errno)); } - localtime_r (&tv.tv_sec, &now); - strftime (now_str, 32, "%Y/%m/%d-%H:%M:%S", &now); - snprintf (tmp_str, 1024, "%s-%d-%s:%" + gf_time_fmt (now_str, sizeof now_str, tv.tv_sec, gf_timefmt_Ymd_T); + snprintf (tmp_str, sizeof tmp_str, "%s-%d-%s:%" #ifdef GF_DARWIN_HOST_OS PRId32, #else |