From 72a4e08864b97765b281e7d034d78bfa03773f93 Mon Sep 17 00:00:00 2001 From: Tom Cherry Date: Fri, 6 Dec 2019 10:25:37 -0800 Subject: [PATCH] logcat: more error printing clean-up * The --help text is way too long to print after each error, so simply print the errors and exit. * Report errno in a few cases where it was previously not reported * Fix more punctuation * Fix '?' and ':' getopt_long() return cases for long options. Test: errors look better Change-Id: I57058a2250e9f3c3431f104e43f0eb5ec60d8c8a --- logcat/logcat.cpp | 137 +++++++++++++++++++--------------------------- 1 file changed, 57 insertions(+), 80 deletions(-) diff --git a/logcat/logcat.cpp b/logcat/logcat.cpp index 6b85f4b32..831b4e343 100644 --- a/logcat/logcat.cpp +++ b/logcat/logcat.cpp @@ -17,6 +17,7 @@ #include #include #include +#include #include #include #include @@ -108,10 +109,6 @@ class Logcat { enum helpType { HELP_FALSE, HELP_TRUE, HELP_FORMAT }; -// if show_help is set, newline required in fmt statement to transition to usage -static void LogcatPanic(enum helpType showHelp, const char* fmt, ...) __printflike(2, 3) - __attribute__((__noreturn__)); - #ifndef F2FS_IOC_SET_PIN_FILE #define F2FS_IOCTL_MAGIC 0xf5 #define F2FS_IOC_SET_PIN_FILE _IOW(F2FS_IOCTL_MAGIC, 13, __u32) @@ -170,7 +167,7 @@ void Logcat::RotateLogs() { output_fd_.reset(openLogFile(output_file_name_, log_rotate_size_kb_)); if (!output_fd_.ok()) { - LogcatPanic(HELP_FALSE, "couldn't open output file"); + error(EXIT_FAILURE, errno, "Couldn't open output file"); } out_byte_count_ = 0; @@ -209,7 +206,7 @@ void Logcat::ProcessBuffer(struct log_msg* buf) { bytesWritten = android_log_printLogLine(logformat_.get(), output_fd_.get(), &entry); if (bytesWritten < 0) { - LogcatPanic(HELP_FALSE, "output error"); + error(EXIT_FAILURE, 0, "Output error."); } } } @@ -229,7 +226,7 @@ void Logcat::PrintDividers(log_id_t log_id, bool print_dividers) { if (dprintf(output_fd_.get(), "--------- %s %s\n", printed_start_[log_id] ? "switch to" : "beginning of", android_log_id_to_name(log_id)) < 0) { - LogcatPanic(HELP_FALSE, "output error"); + error(EXIT_FAILURE, errno, "Output error"); } } last_printed_id_ = log_id; @@ -259,18 +256,16 @@ void Logcat::SetupOutputAndSchedulingPolicy(bool blocking) { output_fd_.reset(openLogFile(output_file_name_, log_rotate_size_kb_)); if (!output_fd_.ok()) { - LogcatPanic(HELP_FALSE, "couldn't open output file"); + error(EXIT_FAILURE, errno, "Couldn't open output file"); } struct stat statbuf; if (fstat(output_fd_.get(), &statbuf) == -1) { - output_fd_.reset(); - LogcatPanic(HELP_FALSE, "couldn't get output file stat\n"); + error(EXIT_FAILURE, errno, "Couldn't get output file stat"); } if ((size_t)statbuf.st_size > SIZE_MAX || statbuf.st_size < 0) { - output_fd_.reset(); - LogcatPanic(HELP_FALSE, "invalid output file stat\n"); + error(EXIT_FAILURE, 0, "Invalid output file stat."); } out_byte_count_ = statbuf.st_size; @@ -427,27 +422,6 @@ static std::pair format_of_size(unsigned long value) return std::make_pair(value, multipliers[i]); } -static void LogcatPanic(enum helpType showHelp, const char* fmt, ...) { - va_list args; - va_start(args, fmt); - vfprintf(stderr, fmt, args); - va_end(args); - - switch (showHelp) { - case HELP_TRUE: - show_help(); - break; - case HELP_FORMAT: - show_format_help(); - break; - case HELP_FALSE: - default: - break; - } - - exit(EXIT_FAILURE); -} - static char* parseTime(log_time& t, const char* cp) { char* ep = t.strptime(cp, "%m-%d %H:%M:%S.%q"); if (ep) return ep; @@ -612,13 +586,12 @@ int Logcat::Run(int argc, char** argv) { // only long options if (long_options[option_index].name == pid_str) { if (pid != 0) { - LogcatPanic(HELP_TRUE, "Only supports one PID argument.\n"); + error(EXIT_FAILURE, 0, "Only one --pid argument can be provided."); } - // ToDo: determine runtime PID_MAX? if (!ParseUint(optarg, &pid) || pid < 1) { - LogcatPanic(HELP_TRUE, "%s %s out of range\n", - long_options[option_index].name, optarg); + error(EXIT_FAILURE, 0, "%s %s out of range.", + long_options[option_index].name, optarg); } break; } @@ -628,8 +601,8 @@ int Logcat::Run(int argc, char** argv) { // ToDo: implement API that supports setting a wrap timeout size_t dummy = ANDROID_LOG_WRAP_DEFAULT_TIMEOUT; if (optarg && (!ParseUint(optarg, &dummy) || dummy < 1)) { - LogcatPanic(HELP_TRUE, "%s %s out of range\n", - long_options[option_index].name, optarg); + error(EXIT_FAILURE, 0, "%s %s out of range.", + long_options[option_index].name, optarg); } if (dummy != ANDROID_LOG_WRAP_DEFAULT_TIMEOUT) { fprintf(stderr, "WARNING: %s %u seconds, ignoring %zu\n", @@ -678,13 +651,13 @@ int Logcat::Run(int argc, char** argv) { if (strspn(optarg, "0123456789") != strlen(optarg)) { char* cp = parseTime(tail_time, optarg); if (!cp) { - LogcatPanic(HELP_FALSE, "-%c \"%s\" not in time format\n", c, optarg); + error(EXIT_FAILURE, 0, "-%c '%s' not in time format.", c, optarg); } if (*cp) { char ch = *cp; *cp = '\0'; - fprintf(stderr, "WARNING: -%c \"%s\"\"%c%s\" time truncated\n", c, optarg, - ch, cp + 1); + fprintf(stderr, "WARNING: -%c '%s' '%c%s' time truncated\n", c, optarg, ch, + cp + 1); *cp = ch; } } else { @@ -705,8 +678,8 @@ int Logcat::Run(int argc, char** argv) { case 'm': { if (!ParseUint(optarg, &max_count_) || max_count_ < 1) { - LogcatPanic(HELP_FALSE, "-%c \"%s\" isn't an integer greater than zero\n", c, - optarg); + error(EXIT_FAILURE, 0, "-%c '%s' isn't an integer greater than zero.", c, + optarg); } } break; @@ -719,7 +692,7 @@ int Logcat::Run(int argc, char** argv) { case 'G': { if (!ParseByteCount(optarg, &setLogSize) || setLogSize < 1) { - LogcatPanic(HELP_FALSE, "ERROR: -G \n"); + error(EXIT_FAILURE, 0, "-G must be specified as ."); } } break; @@ -743,7 +716,8 @@ int Logcat::Run(int argc, char** argv) { } else { log_id_t log_id = android_name_to_log_id(buffer.c_str()); if (log_id >= LOG_ID_MAX) { - LogcatPanic(HELP_TRUE, "unknown buffer %s\n", buffer.c_str()); + error(EXIT_FAILURE, 0, "Unknown buffer '%s' listed for -b.", + buffer.c_str()); } if (log_id == LOG_ID_SECURITY) { security_buffer_selected = true; @@ -767,13 +741,13 @@ int Logcat::Run(int argc, char** argv) { case 'r': if (!ParseUint(optarg, &log_rotate_size_kb_) || log_rotate_size_kb_ < 1) { - LogcatPanic(HELP_TRUE, "Invalid parameter \"%s\" to -r\n", optarg); + error(EXIT_FAILURE, 0, "Invalid parameter '%s' to -r.", optarg); } break; case 'n': if (!ParseUint(optarg, &max_rotated_logs_) || max_rotated_logs_ < 1) { - LogcatPanic(HELP_TRUE, "Invalid parameter \"%s\" to -n\n", optarg); + error(EXIT_FAILURE, 0, "Invalid parameter '%s' to -n.", optarg); } break; @@ -785,7 +759,7 @@ int Logcat::Run(int argc, char** argv) { for (const auto& arg : Split(optarg, delimiters)) { int err = SetLogFormat(arg.c_str()); if (err < 0) { - LogcatPanic(HELP_FORMAT, "Invalid parameter \"%s\" to -v\n", arg.c_str()); + error(EXIT_FAILURE, 0, "Invalid parameter '%s' to -v.", arg.c_str()); } if (err) hasSetLogFormat = true; } @@ -882,20 +856,25 @@ int Logcat::Run(int argc, char** argv) { break; case ':': - LogcatPanic(HELP_TRUE, "Option -%c needs an argument\n", optopt); + error(EXIT_FAILURE, 0, "Option '%s' needs an argument.", argv[optind - 1]); + break; case 'h': show_help(); show_format_help(); return EXIT_SUCCESS; + case '?': + error(EXIT_FAILURE, 0, "Unknown option '%s'.", argv[optind - 1]); + break; + default: - LogcatPanic(HELP_TRUE, "Unrecognized Option %c\n", optopt); + error(EXIT_FAILURE, 0, "Unknown getopt_long() result '%c'.", c); } } if (max_count_ && got_t) { - LogcatPanic(HELP_TRUE, "Cannot use -m (--max-count) and -t together\n"); + error(EXIT_FAILURE, 0, "Cannot use -m (--max-count) and -t together."); } if (print_it_anyways_ && (!regex_ || !max_count_)) { // One day it would be nice if --print -v color and --regex @@ -915,12 +894,12 @@ int Logcat::Run(int argc, char** argv) { } if (log_rotate_size_kb_ != 0 && !output_file_name_) { - LogcatPanic(HELP_TRUE, "-r requires -f as well\n"); + error(EXIT_FAILURE, 0, "-r requires -f as well."); } if (setId != 0) { if (!output_file_name_) { - LogcatPanic(HELP_TRUE, "--id='%s' requires -f as well\n", setId); + error(EXIT_FAILURE, 0, "--id='%s' requires -f as well.", setId); } std::string file_name = StringPrintf("%s.id", output_file_name_); @@ -952,7 +931,7 @@ int Logcat::Run(int argc, char** argv) { if (forceFilters.size()) { int err = android_log_addFilterString(logformat_.get(), forceFilters.c_str()); if (err < 0) { - LogcatPanic(HELP_FALSE, "Invalid filter expression in logcat args\n"); + error(EXIT_FAILURE, 0, "Invalid filter expression in logcat args."); } } else if (argc == optind) { // Add from environment variable @@ -962,7 +941,7 @@ int Logcat::Run(int argc, char** argv) { int err = android_log_addFilterString(logformat_.get(), env_tags_orig); if (err < 0) { - LogcatPanic(HELP_TRUE, "Invalid filter expression in ANDROID_LOG_TAGS\n"); + error(EXIT_FAILURE, 0, "Invalid filter expression in ANDROID_LOG_TAGS."); } } } else { @@ -970,17 +949,17 @@ int Logcat::Run(int argc, char** argv) { for (int i = optind ; i < argc ; i++) { int err = android_log_addFilterString(logformat_.get(), argv[i]); if (err < 0) { - LogcatPanic(HELP_TRUE, "Invalid filter expression '%s'\n", argv[i]); + error(EXIT_FAILURE, 0, "Invalid filter expression '%s'.", argv[i]); } } } if (mode & ANDROID_LOG_PSTORE) { if (output_file_name_) { - LogcatPanic(HELP_FALSE, "-c is ambiguous with both -f and -L specified.\n"); + error(EXIT_FAILURE, 0, "-c is ambiguous with both -f and -L specified."); } if (setLogSize || getLogSize || printStatistics || getPruneList || setPruneList) { - LogcatPanic(HELP_TRUE, "-L is incompatible with -g/-G, -S, and -p/-P\n"); + error(EXIT_FAILURE, 0, "-L is incompatible with -g/-G, -S, and -p/-P."); } if (clearLog) { unlink("/sys/fs/pstore/pmsg-ramoops-0"); @@ -990,7 +969,7 @@ int Logcat::Run(int argc, char** argv) { if (output_file_name_) { if (setLogSize || getLogSize || printStatistics || getPruneList || setPruneList) { - LogcatPanic(HELP_TRUE, "-f is incompatible with -g/-G, -S, and -p/-P\n"); + error(EXIT_FAILURE, 0, "-f is incompatible with -g/-G, -S, and -p/-P."); } if (clearLog || setId) { @@ -1078,21 +1057,20 @@ int Logcat::Run(int argc, char** argv) { // report any errors in the above loop and exit if (!open_device_failures.empty()) { - LogcatPanic(HELP_FALSE, "Unable to open log device%s '%s'\n", - open_device_failures.size() > 1 ? "s" : "", - Join(open_device_failures, ",").c_str()); + error(EXIT_FAILURE, 0, "Unable to open log device%s '%s'.", + open_device_failures.size() > 1 ? "s" : "", Join(open_device_failures, ",").c_str()); } if (!clear_failures.empty()) { - LogcatPanic(HELP_FALSE, "failed to clear the '%s' log%s\n", - Join(clear_failures, ",").c_str(), clear_failures.size() > 1 ? "s" : ""); + error(EXIT_FAILURE, 0, "failed to clear the '%s' log%s.", Join(clear_failures, ",").c_str(), + clear_failures.size() > 1 ? "s" : ""); } if (!set_size_failures.empty()) { - LogcatPanic(HELP_FALSE, "failed to set the '%s' log size%s\n", - Join(set_size_failures, ",").c_str(), set_size_failures.size() > 1 ? "s" : ""); + error(EXIT_FAILURE, 0, "failed to set the '%s' log size%s.", + Join(set_size_failures, ",").c_str(), set_size_failures.size() > 1 ? "s" : ""); } if (!get_size_failures.empty()) { - LogcatPanic(HELP_FALSE, "failed to get the readable '%s' log size%s\n", - Join(get_size_failures, ",").c_str(), get_size_failures.size() > 1 ? "s" : ""); + error(EXIT_FAILURE, 0, "failed to get the readable '%s' log size%s.", + Join(get_size_failures, ",").c_str(), get_size_failures.size() > 1 ? "s" : ""); } if (setPruneList) { @@ -1103,11 +1081,11 @@ int Logcat::Run(int argc, char** argv) { if (asprintf(&buf, "%-*s", (int)(bLen - 1), setPruneList) > 0) { buf[len] = '\0'; if (android_logger_set_prune_list(logger_list.get(), buf, bLen)) { - LogcatPanic(HELP_FALSE, "failed to set the prune list"); + error(EXIT_FAILURE, 0, "Failed to set the prune list."); } free(buf); } else { - LogcatPanic(HELP_FALSE, "failed to set the prune list (alloc)"); + error(EXIT_FAILURE, 0, "Failed to set the prune list (alloc)."); } return EXIT_SUCCESS; } @@ -1138,7 +1116,7 @@ int Logcat::Run(int argc, char** argv) { } if (!buf) { - LogcatPanic(HELP_FALSE, "failed to read data"); + error(EXIT_FAILURE, 0, "Failed to read data."); } // remove trailing FF @@ -1168,30 +1146,29 @@ int Logcat::Run(int argc, char** argv) { struct log_msg log_msg; int ret = android_logger_list_read(logger_list.get(), &log_msg); if (!ret) { - LogcatPanic(HELP_FALSE, R"init(read: unexpected EOF! + error(EXIT_FAILURE, 0, R"init(Unexpected EOF! This means that either logd crashed, or more likely, this instance of logcat was unable to read log messages as quickly as they were being produced. -If you have enabled significant logging, look into using the -G option to increase log buffer sizes. -)init"); +If you have enabled significant logging, look into using the -G option to increase log buffer sizes.)init"); } if (ret < 0) { if (ret == -EAGAIN) break; if (ret == -EIO) { - LogcatPanic(HELP_FALSE, "read: unexpected EOF!\n"); + error(EXIT_FAILURE, 0, "Unexpected EOF!"); } if (ret == -EINVAL) { - LogcatPanic(HELP_FALSE, "read: unexpected length.\n"); + error(EXIT_FAILURE, 0, "Unexpected length."); } - LogcatPanic(HELP_FALSE, "logcat read failure\n"); + error(EXIT_FAILURE, errno, "Logcat read failure"); } if (log_msg.id() > LOG_ID_MAX) { - LogcatPanic(HELP_FALSE, "read: unexpected log id (%d) over LOG_ID_MAX (%d)", - log_msg.id(), LOG_ID_MAX); + error(EXIT_FAILURE, 0, "Unexpected log id (%d) over LOG_ID_MAX (%d).", log_msg.id(), + LOG_ID_MAX); } PrintDividers(log_msg.id(), printDividers);