From 6c45b57fa8eef0f570abb1ac216ac43e0acf314a Mon Sep 17 00:00:00 2001 From: David Steele Date: Sat, 24 Feb 2024 11:22:48 +1300 Subject: [PATCH] Add execOne() to simplify exec for build, documentation, and testing. The core Exec object is efficient but geared toward the specific needs of core and not ease-of-use as required for build, documentation, and testing. execOne() works similarly to system() except that it automatically redirects stderr to stdout and captures the output. --- src/build/common/exec.c | 87 +++++++++++++++++++++++++ src/build/common/exec.h | 27 ++++++++ src/common/exec.c | 50 ++++++++++---- src/common/exec.h | 8 +++ test/define.yaml | 8 +-- test/src/command/test/test.c | 51 +++------------ test/src/meson.build | 2 + test/src/module/common/execTest.c | 38 +++++++++++ test/src/module/protocol/protocolTest.c | 2 +- test/src/module/test/testTest.c | 22 ------- 10 files changed, 213 insertions(+), 82 deletions(-) create mode 100644 src/build/common/exec.c create mode 100644 src/build/common/exec.h diff --git a/src/build/common/exec.c b/src/build/common/exec.c new file mode 100644 index 000000000..ebe27d195 --- /dev/null +++ b/src/build/common/exec.c @@ -0,0 +1,87 @@ +/*********************************************************************************************************************************** +Execute Process Extensions +***********************************************************************************************************************************/ +// Include core module +#include "common/exec.c" + +#include "build/common/exec.h" +#include "common/io/fd.h" + +/**********************************************************************************************************************************/ +static String * +execProcess(Exec *const this) +{ + FUNCTION_LOG_BEGIN(logLevelDebug); + FUNCTION_LOG_PARAM(EXEC, this); + FUNCTION_LOG_END(); + + String *const result = strNew(); + + MEM_CONTEXT_TEMP_BEGIN() + { + int processStatus; + int processResult; + + ioReadOpen(this->ioReadFd); + strCat(result, strNewBuf(ioReadBuf(this->ioReadFd))); + + THROW_ON_SYS_ERROR( + (processResult = waitpid(this->processId, &processStatus, 0)) == -1, ExecuteError, + "unable to wait on child process"); + + // Clear the process id so we don't try to wait for this process on free + this->processId = 0; + + // If the process exited normally but without a success status + if (WIFEXITED(processStatus)) + { + if (WEXITSTATUS(processStatus) != 0) + execCheckStatusError(this, processStatus, strTrim(result)); + } + // Else if the process did not exit normally then it must have been a signal + else + execCheckSignalError(this, processStatus); + } + MEM_CONTEXT_TEMP_END(); + + FUNCTION_LOG_RETURN(STRING, result); +} + +/**********************************************************************************************************************************/ +FN_EXTERN String * +execOne(const String *const command, const ExecOneParam param) +{ + FUNCTION_LOG_BEGIN(logLevelDebug); + FUNCTION_LOG_PARAM(STRING, command); + (void)param; + FUNCTION_LOG_END(); + + String *result = NULL; + + MEM_CONTEXT_TEMP_BEGIN() + { + const StringList *const shellList = strLstNewSplitZ(param.shell != NULL ? param.shell : STRDEF("sh -c"), " "); + StringList *const param = strLstNew(); + + ASSERT(strLstSize(shellList) != 0); + + for (unsigned int shellIdx = 1; shellIdx < strLstSize(shellList); shellIdx++) + strLstAdd(param, strLstGet(shellList, shellIdx)); + + strLstAddFmt(param, "%s 2>&1", strZ(command)); + strLstAddZ(param, "2>&1"); + + Exec *const exec = execNew(strLstGet(shellList, 0), param, command, ioTimeoutMs()); + + execOpen(exec); + + MEM_CONTEXT_PRIOR_BEGIN() + { + result = execProcess(exec); + } + MEM_CONTEXT_PRIOR_END(); + } + MEM_CONTEXT_TEMP_END(); + + FUNCTION_LOG_RETURN(STRING, result); +} diff --git a/src/build/common/exec.h b/src/build/common/exec.h new file mode 100644 index 000000000..cf53a4377 --- /dev/null +++ b/src/build/common/exec.h @@ -0,0 +1,27 @@ +/*********************************************************************************************************************************** +Execute Process Extensions + +Functions intended to simplify exec'ing and getting output. The core Exec object is efficient but it does not work well for the +requirements of the build, test, and doc which prefer ease of use. +***********************************************************************************************************************************/ +#ifndef BUILD_COMMON_EXEC_H +#define BUILD_COMMON_EXEC_H + +#include "common/exec.h" + +/*********************************************************************************************************************************** +Functions +***********************************************************************************************************************************/ +// Execute a command similar to system() while also capturing output. Note that stderr is redirected to stdout. +typedef struct ExecOneParam +{ + VAR_PARAM_HEADER; + const String *shell; // Shell command to use for exec (default is sh -c) +} ExecOneParam; + +#define execOneP(command, ...) \ + execOne(command, (ExecOneParam){VAR_PARAM_INIT, __VA_ARGS__}) + +FN_EXTERN String *execOne(const String *command, ExecOneParam param); + +#endif diff --git a/src/common/exec.c b/src/common/exec.c index dde92c3ea..97ad86faf 100644 --- a/src/common/exec.c +++ b/src/common/exec.c @@ -27,7 +27,6 @@ Object type struct Exec { ExecPub pub; // Publicly accessible variables - String *command; // Command to execute StringList *param; // List of parameters to pass to command const String *name; // Name to display in log/error messages TimeMSec timeout; // Timeout for any i/o operation (read, write, etc.) @@ -129,7 +128,10 @@ execNew(const String *command, const StringList *param, const String *name, Time { *this = (Exec) { - .command = strDup(command), + .pub = + { + .command = strDup(command), + }, .name = strDup(name), .timeout = timeout, @@ -138,7 +140,7 @@ execNew(const String *command, const StringList *param, const String *name, Time }; // The first parameter must be the command - strLstInsert(this->param, 0, this->command); + strLstInsert(this->param, 0, execCommand(this)); } OBJ_NEW_END(); @@ -151,6 +153,31 @@ Check if the process is still running This should be called when anything unexpected happens while reading or writing, including errors and eof. If this function returns then the original error should be rethrown. ***********************************************************************************************************************************/ +// Helper to throw status error +static void +execCheckStatusError(Exec *const this, const int status, const String *const output) +{ + FUNCTION_TEST_BEGIN(); + FUNCTION_LOG_PARAM(EXEC, this); + FUNCTION_TEST_PARAM(INT, status); + FUNCTION_TEST_PARAM(STRING, output); + FUNCTION_TEST_END(); + + // Throw the error with as much information as is available + THROWP_FMT( + errorTypeFromCode(WEXITSTATUS(status)), "%s terminated unexpectedly [%d]%s%s", strZ(this->name), + WEXITSTATUS(status), strSize(output) > 0 ? ": " : "", strSize(output) > 0 ? strZ(output) : ""); + + FUNCTION_TEST_RETURN_VOID(); +} + +// Helper to throw signal error +static void +execCheckSignalError(Exec *const this, const int status) +{ + THROW_FMT(ExecuteError, "%s terminated unexpectedly on signal %d", strZ(this->name), WTERMSIG(status)); +} + static void execCheck(Exec *this) { @@ -174,18 +201,13 @@ execCheck(Exec *this) // If the process exited normally if (WIFEXITED(processStatus)) { - // Get data from stderr to help diagnose the problem - String *errorStr = strTrim( - strNewBuf(ioReadBuf(ioFdReadNewOpen(strNewFmt("%s error", strZ(this->name)), this->fdError, 0)))); - - // Throw the error with as much information as is available - THROWP_FMT( - errorTypeFromCode(WEXITSTATUS(processStatus)), "%s terminated unexpectedly [%d]%s%s", strZ(this->name), - WEXITSTATUS(processStatus), strSize(errorStr) > 0 ? ": " : "", strSize(errorStr) > 0 ? strZ(errorStr) : ""); + execCheckStatusError( + this, processStatus, + strTrim(strNewBuf(ioReadBuf(ioFdReadNewOpen(strNewFmt("%s error", strZ(this->name)), this->fdError, 0))))); } // If the process did not exit normally then it must have been a signal - THROW_FMT(ExecuteError, "%s terminated unexpectedly on signal %d", strZ(this->name), WTERMSIG(processStatus)); + execCheckSignalError(this, processStatus); } FUNCTION_LOG_RETURN_VOID(); @@ -332,11 +354,11 @@ execOpen(Exec *this) PIPE_DUP2(pipeError, 1, STDERR_FILENO); // Execute the binary. This statement will not return if it is successful - execvp(strZ(this->command), (char **const)strLstPtr(this->param)); + execvp(strZ(execCommand(this)), (char **const)strLstPtr(this->param)); // If we got here then there was an error. We can't use a throw as we normally would because we have already shutdown // logging and we don't want to execute exit paths that might free parent resources which we still have references to. - fprintf(stderr, "unable to execute '%s': [%d] %s\n", strZ(this->command), errno, strerror(errno)); + fprintf(stderr, "unable to execute '%s': [%d] %s\n", strZ(execCommand(this)), errno, strerror(errno)); exit(errorTypeCode(&ExecuteError)); } diff --git a/src/common/exec.h b/src/common/exec.h index f21808d56..43fb7bea8 100644 --- a/src/common/exec.h +++ b/src/common/exec.h @@ -29,10 +29,18 @@ Getters/Setters ***********************************************************************************************************************************/ typedef struct ExecPub { + String *command; // Command to execute IoRead *ioReadExec; // Wrapper for file descriptor read interface IoWrite *ioWriteExec; // Wrapper for file descriptor write interface } ExecPub; +// Exec command +FN_INLINE_ALWAYS const String * +execCommand(const Exec *const this) +{ + return THIS_PUB(Exec)->command; +} + // Read interface FN_INLINE_ALWAYS IoRead * execIoRead(Exec *const this) diff --git a/test/define.yaml b/test/define.yaml index e47f1650f..324648ebb 100644 --- a/test/define.yaml +++ b/test/define.yaml @@ -406,10 +406,10 @@ unit: # ---------------------------------------------------------------------------------------------------------------------------- - name: exec - total: 1 + total: 2 coverage: - - common/exec + - build/common/exec # ---------------------------------------------------------------------------------------------------------------------------- - name: ini @@ -483,7 +483,7 @@ unit: - protocol/server include: - - common/exec + - build/common/exec # ******************************************************************************************************************************** - name: config @@ -700,7 +700,7 @@ unit: test: # ---------------------------------------------------------------------------------------------------------------------------- - name: test - total: 3 + total: 2 coverage: - test/command/test/build diff --git a/test/src/command/test/test.c b/test/src/command/test/test.c index 3fd8c8636..b6c677eb0 100644 --- a/test/src/command/test/test.c +++ b/test/src/command/test/test.c @@ -5,43 +5,13 @@ Test Command #include +#include "build/common/exec.h" #include "command/test/build.h" #include "common/debug.h" #include "common/log.h" #include "config/config.h" #include "storage/posix/storage.h" -/**********************************************************************************************************************************/ -static const String *cmdTestExecLog; - -static void -cmdTestExec(const String *const command) -{ - FUNCTION_LOG_BEGIN(logLevelDebug); - FUNCTION_LOG_PARAM(STRING, command); - FUNCTION_LOG_END(); - - ASSERT(cmdTestExecLog != NULL); - ASSERT(command != NULL); - - MEM_CONTEXT_TEMP_BEGIN() - { - LOG_DETAIL_FMT("exec: %s", strZ(command)); - - if (system(zNewFmt("%s > %s 2>&1", strZ(command), strZ(cmdTestExecLog))) != 0) - { - const Buffer *const buffer = storageGetP(storageNewReadP(storagePosixNewP(FSLASH_STR), cmdTestExecLog)); - - THROW_FMT( - ExecuteError, "unable to execute: %s > %s 2>&1:\n%s", strZ(command), strZ(cmdTestExecLog), - zNewFmt("\n%s", strZ(strTrim(strNewBuf(buffer))))); - } - } - MEM_CONTEXT_TEMP_END(); - - FUNCTION_LOG_RETURN_VOID(); -} - /**********************************************************************************************************************************/ static void cmdTestPathCreate(const Storage *const storage, const String *const path) @@ -55,17 +25,19 @@ cmdTestPathCreate(const Storage *const storage, const String *const path) ASSERT(storage != NULL); + const String *const rmCommand = strNewFmt("rm -rf '%s'/*", strZ(storagePathP(storage, path))); + TRY_BEGIN() { - storagePathRemoveP(storage, path, .recurse = true); + execOneP(rmCommand); } CATCH_ANY() { // Reset permissions - cmdTestExec(strNewFmt("chmod -R 777 %s", strZ(storagePathP(storage, path)))); + execOneP(strNewFmt("chmod -R 777 '%s'", strZ(storagePathP(storage, path)))); // Try to remove again - storagePathRemoveP(storage, path, .recurse = true); + execOneP(rmCommand); } TRY_END(); @@ -100,9 +72,6 @@ cmdTest( MEM_CONTEXT_TEMP_BEGIN() { - // Log file name - cmdTestExecLog = strNewFmt("%s/exec-%u.log", strZ(pathTest), vmId); - // Find test ASSERT(moduleName != NULL); @@ -144,9 +113,9 @@ cmdTest( { LOG_DETAIL("meson setup"); - cmdTestExec( + execOneP( strNewFmt( - "meson setup -Dwerror=true -Dfatal-errors=true %s %s %s", strZ(mesonSetup), strZ(pathUnitBuild), + "meson setup -Dwerror=true -Dfatal-errors=true %s '%s' '%s'", strZ(mesonSetup), strZ(pathUnitBuild), strZ(pathUnit))); } // Else reconfigure @@ -154,7 +123,7 @@ cmdTest( { LOG_DETAIL("meson configure"); - cmdTestExec(strNewFmt("meson configure %s %s", strZ(mesonSetup), strZ(pathUnitBuild))); + execOneP(strNewFmt("meson configure %s '%s'", strZ(mesonSetup), strZ(pathUnitBuild))); } // Remove old coverage data. Note that coverage can be in different paths depending on the meson version. @@ -175,7 +144,7 @@ cmdTest( storageRemoveP(storageUnitBuild, STRDEF("gmon.out")); // Ninja build - cmdTestExec(strNewFmt("ninja -C %s", strZ(pathUnitBuild))); + execOneP(strNewFmt("ninja -C '%s'", strZ(pathUnitBuild))); buildRetry = false; } CATCH_ANY() diff --git a/test/src/meson.build b/test/src/meson.build index 66a27ecb1..256ced772 100644 --- a/test/src/meson.build +++ b/test/src/meson.build @@ -26,6 +26,7 @@ subdir('command/help') # test target #################################################################################################################################### src_test = [ + '../../src/build/common/exec.c', '../../src/build/common/render.c', '../../src/build/common/string.c', '../../src/build/common/yaml.c', @@ -34,6 +35,7 @@ src_test = [ '../../src/command/help/help.c', '../../src/common/compress/bz2/common.c', '../../src/common/compress/bz2/decompress.c', + '../../src/common/fork.c', '../../src/common/ini.c', '../../src/common/io/fd.c', '../../src/common/io/fdRead.c', diff --git a/test/src/module/common/execTest.c b/test/src/module/common/execTest.c index c5c830c1f..b56dd283d 100644 --- a/test/src/module/common/execTest.c +++ b/test/src/module/common/execTest.c @@ -108,5 +108,43 @@ testRun(void) TEST_RESULT_VOID(execFree(exec), "sleep exited as expected"); } + // ***************************************************************************************************************************** + if (testBegin("execOne()")) + { + Exec *exec = NULL; + + // ------------------------------------------------------------------------------------------------------------------------- + TEST_TITLE("exec without output"); + + TEST_RESULT_STR_Z(execOneP(STRDEF("ls " TEST_PATH)), "", "exec ls"); + + // ------------------------------------------------------------------------------------------------------------------------- + TEST_TITLE("touch file"); + + TEST_RESULT_STR_Z(execOneP(STRDEF("touch " TEST_PATH "/file")), "", "exec touch"); + + // ------------------------------------------------------------------------------------------------------------------------- + TEST_TITLE("exec with custom shell and output"); + + TEST_RESULT_STR_Z(execOneP(STRDEF("ls " TEST_PATH), .shell = STRDEF("sh -c")), "file\n", "exec ls"); + + // ------------------------------------------------------------------------------------------------------------------------- + TEST_TITLE("exec exits from signal"); + + TEST_ASSIGN(exec, execNew(STRDEF("cat"), NULL, STRDEF("cat"), 1000), "new cat exec"); + TEST_RESULT_VOID(execOpen(exec), "open cat exec"); + kill(exec->processId, SIGKILL); + + TEST_ERROR(execProcess(exec), ExecuteError, "cat terminated unexpectedly on signal 9"); + TEST_RESULT_VOID(execFree(exec), "free exec"); + + // ------------------------------------------------------------------------------------------------------------------------- + TEST_TITLE("exec exits with error"); + + TEST_ERROR( + execOneP(STRDEF("cat missing.txt")), UnknownError, + "cat missing.txt terminated unexpectedly [1]: cat: missing.txt: No such file or directory"); + } + FUNCTION_HARNESS_RETURN_VOID(); } diff --git a/test/src/module/protocol/protocolTest.c b/test/src/module/protocol/protocolTest.c index a7f180204..255bb9a1f 100644 --- a/test/src/module/protocol/protocolTest.c +++ b/test/src/module/protocol/protocolTest.c @@ -299,7 +299,7 @@ testRun(void) OBJ_NEW_BASE_BEGIN(Exec, .childQty = MEM_CONTEXT_QTY_MAX, .callbackQty = 1) { protocolHelperClient.exec = OBJ_NEW_ALLOC(); - *protocolHelperClient.exec = (Exec){.name = strNewZ("test"), .command = strNewZ("test"), .processId = INT_MAX}; + *protocolHelperClient.exec = (Exec){.pub = {.command = strNewZ("test")}, .name = strNewZ("test"), .processId = INT_MAX}; memContextCallbackSet(memContextCurrent(), execFreeResource, protocolHelperClient.exec); } OBJ_NEW_END(); diff --git a/test/src/module/test/testTest.c b/test/src/module/test/testTest.c index fa758c87a..00336511c 100644 --- a/test/src/module/test/testTest.c +++ b/test/src/module/test/testTest.c @@ -66,28 +66,6 @@ testRun(void) TEST_RESULT_STR_Z(cmdBldPathRelative(STRDEF("/tmp"), STRDEF("/tmp/sub")), "sub", "base is sub of compare"); } - // ***************************************************************************************************************************** - if (testBegin("cmdTestExec()")) - { - cmdTestExecLog = STRDEF(TEST_PATH "/error.log"); - bool error = false; - - // The error will vary by OS so just make sure an error was thrown - TRY_BEGIN() - { - cmdTestExec(STRDEF("/bogus/bogus")); - } - CATCH(ExecuteError) - { - error = true; - } - TRY_END(); - - TEST_RESULT_BOOL(error, true, "an error should be thrown"); - - cmdTestExecLog = NULL; - } - // ***************************************************************************************************************************** if (testBegin("TestDef and TestBuild")) {