ACK: [PATCH] lib + tests: modify fwts_pipe_exec to return the child exit status
Alex Hung
alex.hung at canonical.com
Wed Dec 12 07:44:09 UTC 2012
On 12/07/2012 10:08 PM, Colin King wrote:
> From: Colin Ian King <colin.king at canonical.com>
>
> Fix bug that has been in the fwts s3, s4 and s3power tests. Originally the
> semantics to fwts_pipe_exec() was that was meant to return the child exit status
> but this got changed to return FWTS_OK if successful and FWTS_EXEC_FAILED if
> the child exec failed. Thus we no longer passed back the child exit status which
> the s3, s4 and s4power tests relied on to check the suspend or hibernate pm script
> exit status.
>
> This patch adds an extra child exit status parameter to fwts_pipe_exec() which can
> be checked if required. However, for most use cases, one can ignore this and just
> check for FWTS_OK or FWTS_EXEC_FAILED if you don't care about the child exit status.
>
> The patch fixes up all the calls to fwts_pipe_exec and also changes the way errors
> are detected assuming anything that is not a FWTS_OK return is a failure.
>
> Signed-off-by: Colin Ian King <colin.king at canonical.com>
> ---
> src/acpi/mcfg/mcfg.c | 3 ++-
> src/acpi/s3/s3.c | 2 +-
> src/acpi/s3power/s3power.c | 3 +--
> src/acpi/s4/s4.c | 2 +-
> src/bios/mtrr/mtrr.c | 3 ++-
> src/hotkey/hotkey/hotkey.c | 3 ++-
> src/lib/include/fwts_pipeio.h | 2 +-
> src/lib/src/fwts_efi_module.c | 8 ++++++--
> src/lib/src/fwts_hwinfo.c | 20 +++++++++++---------
> src/lib/src/fwts_pipeio.c | 12 ++++++------
> src/pci/aspm/aspm.c | 6 ++++--
> src/pci/maxreadreq/maxreadreq.c | 4 +++-
> 12 files changed, 40 insertions(+), 28 deletions(-)
>
> diff --git a/src/acpi/mcfg/mcfg.c b/src/acpi/mcfg/mcfg.c
> index 331c24f..39ae697 100644
> --- a/src/acpi/mcfg/mcfg.c
> +++ b/src/acpi/mcfg/mcfg.c
> @@ -40,6 +40,7 @@ static void compare_config_space(
> {
> fwts_list *lspci_output;
> fwts_list_link *item;
> + int status;
>
> char command[PATH_MAX];
> char compare_line[50];
> @@ -54,7 +55,7 @@ static void compare_config_space(
> snprintf(command, sizeof(command), "%s -vxxx -s %" PRIu16 ":%" PRIu32,
> fw->lspci, segment, device);
>
> - if (fwts_pipe_exec(command, &lspci_output) == FWTS_EXEC_ERROR) {
> + if (fwts_pipe_exec(command, &lspci_output, &status) != FWTS_OK) {
> fwts_log_warning(fw, "Could not execute %s", command);
> return;
> }
> diff --git a/src/acpi/s3/s3.c b/src/acpi/s3/s3.c
> index a1b7848..182db43 100644
> --- a/src/acpi/s3/s3.c
> +++ b/src/acpi/s3/s3.c
> @@ -106,7 +106,7 @@ static int s3_do_suspend_resume(fwts_framework *fw,
> /* Do S3 here */
> fwts_progress_message(fw, percent, "(Suspending)");
> time(&t_start);
> - status = fwts_pipe_exec(command, &output);
> + (void)fwts_pipe_exec(command, &output, &status);
> time(&t_end);
> fwts_progress_message(fw, percent, "(Resumed)");
> fwts_text_list_free(output);
> diff --git a/src/acpi/s3power/s3power.c b/src/acpi/s3power/s3power.c
> index a8a7c25..0b98df1 100644
> --- a/src/acpi/s3power/s3power.c
> +++ b/src/acpi/s3power/s3power.c
> @@ -191,8 +191,7 @@ static int s3power_test(fwts_framework *fw)
> /* Do S3 here */
> fwts_progress_message(fw, 100, "(Suspending)");
> time(&t_start);
> - status = 0;
> - status = fwts_pipe_exec(PM_SUSPEND, &output);
> + (void)fwts_pipe_exec(PM_SUSPEND, &output, &status);
> time(&t_end);
> fwts_progress_message(fw, 100, "(Resumed)");
> fwts_text_list_free(output);
> diff --git a/src/acpi/s4/s4.c b/src/acpi/s4/s4.c
> index 728062d..bea7fce 100644
> --- a/src/acpi/s4/s4.c
> +++ b/src/acpi/s4/s4.c
> @@ -137,7 +137,7 @@ static int s4_hibernate(fwts_framework *fw,
>
> /* Do s4 here */
> fwts_progress_message(fw, percent, "(Hibernating)");
> - status = fwts_pipe_exec(command, &output);
> + (void)fwts_pipe_exec(command, &output, &status);
> fwts_progress_message(fw, percent, "(Resumed)");
> fwts_text_list_free(output);
> free(command);
> diff --git a/src/bios/mtrr/mtrr.c b/src/bios/mtrr/mtrr.c
> index 6dbc8e2..62a91c7 100644
> --- a/src/bios/mtrr/mtrr.c
> +++ b/src/bios/mtrr/mtrr.c
> @@ -199,11 +199,12 @@ static bool is_prefetchable(fwts_framework *fw, char *device, uint64_t address)
> char line[4096];
> fwts_list *lspci_output;
> fwts_list_link *item;
> + int status;
>
> memset(line,0,4096);
>
> snprintf(line, sizeof(line), "%s -v -s %s", fw->lspci, device);
> - fwts_pipe_exec(line, &lspci_output);
> + fwts_pipe_exec(line, &lspci_output, &status);
> if (lspci_output == NULL)
> return pref;
>
> diff --git a/src/hotkey/hotkey/hotkey.c b/src/hotkey/hotkey/hotkey.c
> index 267b69e..59a6829 100644
> --- a/src/hotkey/hotkey/hotkey.c
> +++ b/src/hotkey/hotkey/hotkey.c
> @@ -182,9 +182,10 @@ static char *hotkey_find_keymap(char *device)
>
> char buffer[1024];
> char *keymap = NULL;
> + int status;
>
> snprintf(buffer, sizeof(buffer), "udevadm test /class/%s 2>&1", device);
> - if (fwts_pipe_exec(buffer, &output) != FWTS_OK)
> + if (fwts_pipe_exec(buffer, &output, &status) != FWTS_OK)
> return NULL;
>
> snprintf(buffer, sizeof(buffer), "keymap %s", device);
> diff --git a/src/lib/include/fwts_pipeio.h b/src/lib/include/fwts_pipeio.h
> index f1875da..04f4dea 100644
> --- a/src/lib/include/fwts_pipeio.h
> +++ b/src/lib/include/fwts_pipeio.h
> @@ -31,6 +31,6 @@
> int fwts_pipe_open(const char *command, pid_t *childpid);
> char *fwts_pipe_read(const int fd, ssize_t *length);
> int fwts_pipe_close(const int fd, const pid_t pid);
> -int fwts_pipe_exec(const char *command, fwts_list **list);
> +int fwts_pipe_exec(const char *command, fwts_list **list, int *status);
>
> #endif
> diff --git a/src/lib/src/fwts_efi_module.c b/src/lib/src/fwts_efi_module.c
> index 26e5740..c92a091 100644
> --- a/src/lib/src/fwts_efi_module.c
> +++ b/src/lib/src/fwts_efi_module.c
> @@ -57,7 +57,9 @@ int fwts_lib_efi_runtime_load_module(fwts_framework *fw)
> }
>
> if (!module_already_loaded) {
> - if (fwts_pipe_exec("modprobe efi_runtime", &output) != FWTS_OK) {
> + int status;
> +
> + if (fwts_pipe_exec("modprobe efi_runtime", &output, &status) != FWTS_OK) {
> fwts_log_error(fw, "Load efi_runtime module error.");
> return FWTS_ERROR;
> } else {
> @@ -94,7 +96,9 @@ int fwts_lib_efi_runtime_unload_module(fwts_framework *fw)
> return FWTS_ERROR;
> }
> if (module_already_loaded) {
> - if (fwts_pipe_exec("modprobe -r efi_runtime", &output) != FWTS_OK) {
> + int status;
> +
> + if (fwts_pipe_exec("modprobe -r efi_runtime", &output, &status) != FWTS_OK) {
> fwts_log_error(fw, "Unload efi_runtime module error.");
> return FWTS_ERROR;
> } else {
> diff --git a/src/lib/src/fwts_hwinfo.c b/src/lib/src/fwts_hwinfo.c
> index 4365c64..3fcac80 100644
> --- a/src/lib/src/fwts_hwinfo.c
> +++ b/src/lib/src/fwts_hwinfo.c
> @@ -33,15 +33,17 @@ int fwts_hwinfo_get(fwts_framework *fw, fwts_hwinfo *hwinfo)
> {
> FWTS_UNUSED(fw);
>
> - fwts_pipe_exec("lspci | grep Network", &hwinfo->network);
> - fwts_pipe_exec("lspci | grep Ethernet", &hwinfo->ethernet);
> - fwts_pipe_exec("ifconfig -a | grep -A1 '^\\w'", &hwinfo->ifconfig);
> - fwts_pipe_exec("iwconfig | grep -A1 '^\\w'", &hwinfo->iwconfig);
> - fwts_pipe_exec("hciconfig -a | grep -A2 '^\\w", &hwinfo->hciconfig);
> - fwts_pipe_exec("lspci | grep VGA", &hwinfo->videocard);
> - fwts_pipe_exec("xinput list", &hwinfo->xinput);
> - fwts_pipe_exec("pactl list | grep Sink | grep -v Latency", &hwinfo->pactl);
> - fwts_pipe_exec("pactl list | grep Source | grep -v Latency", &hwinfo->pactl);
> + int status;
> +
> + fwts_pipe_exec("lspci | grep Network", &hwinfo->network, &status);
> + fwts_pipe_exec("lspci | grep Ethernet", &hwinfo->ethernet, &status);
> + fwts_pipe_exec("ifconfig -a | grep -A1 '^\\w'", &hwinfo->ifconfig, &status);
> + fwts_pipe_exec("iwconfig | grep -A1 '^\\w'", &hwinfo->iwconfig, &status);
> + fwts_pipe_exec("hciconfig -a | grep -A2 '^\\w", &hwinfo->hciconfig, &status);
> + fwts_pipe_exec("lspci | grep VGA", &hwinfo->videocard, &status);
> + fwts_pipe_exec("xinput list", &hwinfo->xinput, &status);
> + fwts_pipe_exec("pactl list | grep Sink | grep -v Latency", &hwinfo->pactl, &status);
> + fwts_pipe_exec("pactl list | grep Source | grep -v Latency", &hwinfo->pactl, &status);
>
> return FWTS_OK;
> }
> diff --git a/src/lib/src/fwts_pipeio.c b/src/lib/src/fwts_pipeio.c
> index 68abee9..5af817b 100644
> --- a/src/lib/src/fwts_pipeio.c
> +++ b/src/lib/src/fwts_pipeio.c
> @@ -133,14 +133,15 @@ int fwts_pipe_close(const int fd, const pid_t pid)
> * fwts_pipe_exec()
> * execute a command, return a list containing lines
> * of the stdout output from the command.
> + * Return FWTS_OK if the exec worked, FWTS_EXEC_ERROR if
> + * it failed. status contains the child exit status.
> */
> -int fwts_pipe_exec(const char *command, fwts_list **list)
> +int fwts_pipe_exec(const char *command, fwts_list **list, int *status)
> {
> pid_t pid;
> int fd;
> ssize_t len;
> char *text;
> - int ret;
>
> if ((fd = fwts_pipe_open(command, &pid)) < 0)
> return FWTS_ERROR;
> @@ -149,12 +150,11 @@ int fwts_pipe_exec(const char *command, fwts_list **list)
> *list = fwts_list_from_text(text);
> free(text);
>
> - ret = fwts_pipe_close(fd, pid);
> -
> - if (ret == FWTS_EXEC_ERROR) {
> + *status = fwts_pipe_close(fd, pid);
> + if (*status) {
> fwts_list_free(*list, free);
> *list = NULL;
> - return FWTS_ERROR;
> + return FWTS_EXEC_ERROR;
> }
> return FWTS_OK;
> }
> diff --git a/src/pci/aspm/aspm.c b/src/pci/aspm/aspm.c
> index 1a6cc27..a2cda8c 100644
> --- a/src/pci/aspm/aspm.c
> +++ b/src/pci/aspm/aspm.c
> @@ -229,10 +229,11 @@ static int pcie_check_aspm_registers(fwts_framework *fw)
> struct pci_device *head = NULL, *cur = NULL, *device = NULL;
> char command[PATH_MAX];
> int ret = FWTS_OK;
> + int status;
>
> snprintf(command, sizeof(command), "%s", fw->lspci);
>
> - if (fwts_pipe_exec(command, &lspci_output) == FWTS_EXEC_ERROR) {
> + if (fwts_pipe_exec(command, &lspci_output, &status) != FWTS_OK) {
> fwts_log_warning(fw, "Could not execute %s", command);
> return FWTS_ERROR;
> }
> @@ -267,10 +268,11 @@ static int pcie_check_aspm_registers(fwts_framework *fw)
> for (cur = head; cur; cur = cur->next) {
> int reg_loc = 0, reg_val = 0;
> int i;
> + int status;
>
> snprintf(command, sizeof(command), "%s -s %02X:%02X.%02X -xxx",
> fw->lspci, cur->bus, cur->dev, cur->func);
> - if (fwts_pipe_exec(command, &lspci_output) == FWTS_EXEC_ERROR) {
> + if (fwts_pipe_exec(command, &lspci_output, &status) != FWTS_OK) {
> fwts_log_warning(fw, "Could not execute %s", command);
> pci_device_list_free(head);
> return FWTS_ERROR;
> diff --git a/src/pci/maxreadreq/maxreadreq.c b/src/pci/maxreadreq/maxreadreq.c
> index e5ace46..92af83f 100644
> --- a/src/pci/maxreadreq/maxreadreq.c
> +++ b/src/pci/maxreadreq/maxreadreq.c
> @@ -37,10 +37,12 @@ static fwts_list *lspci_text;
>
> static int maxreadreq_init(fwts_framework *fw)
> {
> + int status;
> +
> if (fwts_check_executable(fw, fw->lspci, "lspci"))
> return FWTS_ERROR;
>
> - if (fwts_pipe_exec("lspci -vvv", &lspci_text)) {
> + if (fwts_pipe_exec("lspci -vvv", &lspci_text, &status) != FWTS_OK) {
> fwts_log_error(fw, "Failed to execute lspci -vvv.");
> return FWTS_ERROR;
> }
>
Acked-by: Alex Hung <alex.hung at canonical.com>
More information about the fwts-devel
mailing list