From 59dcb9cbea8fb70ab933fd10d35582b08cd13f37 Mon Sep 17 00:00:00 2001 From: Tao Bao Date: Mon, 3 Oct 2016 18:06:46 -0700 Subject: [PATCH] edify: Move State.script and State.errmsg to std::string. This way we kill a few strdup() and free() calls. Test: 1. recovery_component_test still passes; 2. Applying an update with the new updater works; 3. The error code in a script with abort("E310: xyz") is recorded into last_install correctly. Change-Id: Ibda4da5937346e058a0d7cc81764d6f02920010a --- edify/edify_parser.cpp | 36 +++++++++++------------------ edify/expr.cpp | 39 +++++++++++--------------------- edify/expr.h | 22 +++++++++--------- tests/component/edify_test.cpp | 7 +----- updater/updater.cpp | 41 ++++++++++++++++------------------ 5 files changed, 57 insertions(+), 88 deletions(-) diff --git a/edify/edify_parser.cpp b/edify/edify_parser.cpp index 16ad2ab7..15342d3f 100644 --- a/edify/edify_parser.cpp +++ b/edify/edify_parser.cpp @@ -24,21 +24,20 @@ * makes the tool less useful. We should either extend the tool or remove it. */ +#include #include -#include -#include + +#include + +#include #include "expr.h" -#include "parser.h" -void ExprDump(int depth, Expr* n, char* script) { +static void ExprDump(int depth, const Expr* n, const std::string& script) { printf("%*s", depth*2, ""); - char temp = script[n->end]; - script[n->end] = '\0'; printf("%s %p (%d-%d) \"%s\"\n", n->name == NULL ? "(NULL)" : n->name, n->fn, n->start, n->end, - script+n->start); - script[n->end] = temp; + script.substr(n->start, n->end - n->start).c_str()); for (int i = 0; i < n->argc; ++i) { ExprDump(depth+1, n->argv[i], script); } @@ -53,34 +52,25 @@ int main(int argc, char** argv) { return 1; } - FILE* f = fopen(argv[1], "r"); - if (f == NULL) { - printf("%s: %s: No such file or directory\n", argv[0], argv[1]); + std::string buffer; + if (!android::base::ReadFileToString(argv[1], &buffer)) { + printf("%s: failed to read %s: %s\n", argv[0], argv[1], strerror(errno)); return 1; } - char buffer[8192]; - int size = fread(buffer, 1, 8191, f); - fclose(f); - buffer[size] = '\0'; Expr* root; int error_count = 0; - int error = parse_string(buffer, &root, &error_count); + int error = parse_string(buffer.data(), &root, &error_count); printf("parse returned %d; %d errors encountered\n", error, error_count); if (error == 0 || error_count > 0) { ExprDump(0, root, buffer); - State state; - state.cookie = NULL; - state.script = buffer; - state.errmsg = NULL; - + State state(buffer, nullptr); char* result = Evaluate(&state, root); if (result == NULL) { printf("result was NULL, message is: %s\n", - (state.errmsg == NULL ? "(NULL)" : state.errmsg)); - free(state.errmsg); + (state.errmsg.empty() ? "(NULL)" : state.errmsg.c_str())); } else { printf("result is [%s]\n", result); } diff --git a/edify/expr.cpp b/edify/expr.cpp index ecb1bea1..aa3a55a8 100644 --- a/edify/expr.cpp +++ b/edify/expr.cpp @@ -107,8 +107,7 @@ Value* ConcatFn(const char* name, State* state, int argc, Expr* argv[]) { Value* IfElseFn(const char* name, State* state, int argc, Expr* argv[]) { if (argc != 2 && argc != 3) { - free(state->errmsg); - state->errmsg = strdup("ifelse expects 2 or 3 arguments"); + state->errmsg = "ifelse expects 2 or 3 arguments"; return NULL; } char* cond = Evaluate(state, argv[0]); @@ -134,11 +133,10 @@ Value* AbortFn(const char* name, State* state, int argc, Expr* argv[]) { if (argc > 0) { msg = Evaluate(state, argv[0]); } - free(state->errmsg); if (msg) { state->errmsg = msg; } else { - state->errmsg = strdup("called abort()"); + state->errmsg = "called abort()"; } return NULL; } @@ -153,15 +151,8 @@ Value* AssertFn(const char* name, State* state, int argc, Expr* argv[]) { int b = BooleanString(v); free(v); if (!b) { - int prefix_len; int len = argv[i]->end - argv[i]->start; - char* err_src = reinterpret_cast(malloc(len + 20)); - strcpy(err_src, "assert failed: "); - prefix_len = strlen(err_src); - memcpy(err_src + prefix_len, state->script + argv[i]->start, len); - err_src[prefix_len + len] = '\0'; - free(state->errmsg); - state->errmsg = err_src; + state->errmsg = "assert failed: " + state->script.substr(argv[i]->start, len); return NULL; } } @@ -279,8 +270,7 @@ Value* SequenceFn(const char* name, State* state, int argc, Expr* argv[]) { Value* LessThanIntFn(const char* name, State* state, int argc, Expr* argv[]) { if (argc != 2) { - free(state->errmsg); - state->errmsg = strdup("less_than_int expects 2 arguments"); + state->errmsg = "less_than_int expects 2 arguments"; return NULL; } @@ -314,8 +304,7 @@ Value* LessThanIntFn(const char* name, State* state, int argc, Expr* argv[]) { Value* GreaterThanIntFn(const char* name, State* state, int argc, Expr* argv[]) { if (argc != 2) { - free(state->errmsg); - state->errmsg = strdup("greater_than_int expects 2 arguments"); + state->errmsg = "greater_than_int expects 2 arguments"; return NULL; } @@ -499,20 +488,12 @@ Value** ReadValueVarArgs(State* state, int argc, Expr* argv[]) { return args; } -static void ErrorAbortV(State* state, const char* format, va_list ap) { - std::string buffer; - android::base::StringAppendV(&buffer, format, ap); - free(state->errmsg); - state->errmsg = strdup(buffer.c_str()); - return; -} - // Use printf-style arguments to compose an error message to put into // *state. Returns nullptr. Value* ErrorAbort(State* state, const char* format, ...) { va_list ap; va_start(ap, format); - ErrorAbortV(state, format, ap); + android::base::StringAppendV(&state->errmsg, format, ap); va_end(ap); return nullptr; } @@ -520,8 +501,14 @@ Value* ErrorAbort(State* state, const char* format, ...) { Value* ErrorAbort(State* state, CauseCode cause_code, const char* format, ...) { va_list ap; va_start(ap, format); - ErrorAbortV(state, format, ap); + android::base::StringAppendV(&state->errmsg, format, ap); va_end(ap); state->cause_code = cause_code; return nullptr; } + +State::State(const std::string& script, void* cookie) : + script(script), + cookie(cookie) { +} + diff --git a/edify/expr.h b/edify/expr.h index 88634799..f045d938 100644 --- a/edify/expr.h +++ b/edify/expr.h @@ -18,6 +18,7 @@ #define _EXPRESSION_H #include +#include #include "error_code.h" #include "yydefs.h" @@ -26,20 +27,20 @@ typedef struct Expr Expr; -typedef struct { +struct State { + State(const std::string& script, void* cookie); + + // The source of the original script. + const std::string& script; + // Optional pointer to app-specific data; the core of edify never // uses this value. void* cookie; - // The source of the original script. Must be NULL-terminated, - // and in writable memory (Evaluate may make temporary changes to - // it but will restore it when done). - char* script; - // The error message (if any) returned if the evaluation aborts. - // Should be NULL initially, will be either NULL or a malloc'd - // pointer after Evaluate() returns. - char* errmsg; + // Should be empty initially, will be either empty or a string that + // Evaluate() returns. + std::string errmsg; // error code indicates the type of failure (e.g. failure to update system image) // during the OTA process. @@ -50,8 +51,7 @@ typedef struct { CauseCode cause_code = kNoCause; bool is_retry = false; - -} State; +}; #define VAL_STRING 1 // data will be NULL-terminated; size doesn't count null #define VAL_BLOB 2 diff --git a/tests/component/edify_test.cpp b/tests/component/edify_test.cpp index ded8540a..ede2ecb0 100644 --- a/tests/component/edify_test.cpp +++ b/tests/component/edify_test.cpp @@ -25,10 +25,7 @@ static void expect(const char* expr_str, const char* expected) { int error_count; EXPECT_EQ(parse_string(expr_str, &e, &error_count), 0); - State state; - state.cookie = nullptr; - state.errmsg = nullptr; - state.script = strdup(expr_str); + State state(expr_str, nullptr); char* result = Evaluate(&state, e); @@ -38,8 +35,6 @@ static void expect(const char* expr_str, const char* expected) { EXPECT_STREQ(result, expected); } - free(state.errmsg); - free(state.script); free(result); } diff --git a/updater/updater.cpp b/updater/updater.cpp index c222cee0..74a4048f 100644 --- a/updater/updater.cpp +++ b/updater/updater.cpp @@ -14,22 +14,24 @@ * limitations under the License. */ +#include "updater.h" + #include #include #include #include -#include "edify/expr.h" -#include "updater.h" -#include "install.h" -#include "blockimg.h" -#include "minzip/Zip.h" -#include "minzip/SysUtil.h" -#include "config.h" - +#include #include #include +#include "config.h" +#include "edify/expr.h" +#include "blockimg.h" +#include "install.h" +#include "minzip/Zip.h" +#include "minzip/SysUtil.h" + // Generated by the makefile, this function defines the // RegisterDeviceExtensions() function, which calls all the // registration functions for device-specific extensions. @@ -140,10 +142,7 @@ int main(int argc, char** argv) { updater_info.package_zip_addr = map.addr; updater_info.package_zip_len = map.length; - State state; - state.cookie = &updater_info; - state.script = script; - state.errmsg = NULL; + State state(script, &updater_info); if (argc == 5) { if (strcmp(argv[4], "retry") == 0) { @@ -160,22 +159,21 @@ int main(int argc, char** argv) { } if (result == NULL) { - if (state.errmsg == NULL) { + if (state.errmsg.empty()) { printf("script aborted (no error message)\n"); fprintf(cmd_pipe, "ui_print script aborted (no error message)\n"); } else { - printf("script aborted: %s\n", state.errmsg); - char* line = strtok(state.errmsg, "\n"); - while (line) { + printf("script aborted: %s\n", state.errmsg.c_str()); + const std::vector lines = android::base::Split(state.errmsg, "\n"); + for (const std::string& line : lines) { // Parse the error code in abort message. // Example: "E30: This package is for bullhead devices." - if (*line == 'E') { - if (sscanf(line, "E%u: ", &state.error_code) != 1) { - printf("Failed to parse error code: [%s]\n", line); + if (!line.empty() && line[0] == 'E') { + if (sscanf(line.c_str(), "E%u: ", &state.error_code) != 1) { + printf("Failed to parse error code: [%s]\n", line.c_str()); } } - fprintf(cmd_pipe, "ui_print %s\n", line); - line = strtok(NULL, "\n"); + fprintf(cmd_pipe, "ui_print %s\n", line.c_str()); } fprintf(cmd_pipe, "ui_print\n"); } @@ -189,7 +187,6 @@ int main(int argc, char** argv) { } } - free(state.errmsg); return 7; } else { fprintf(cmd_pipe, "ui_print script succeeded: result was [%s]\n", result);