From 1b644ad64eb9b9c1d6e97953b98c8c00559bf37d Mon Sep 17 00:00:00 2001 From: "Richard W.M. Jones" Date: Wed, 13 Mar 2013 16:59:23 +0000 Subject: [PATCH] utils: Fix error messages for external commands that fail (RHBZ#921040). This adds a common utility function (guestfs___exit_status_to_string) and a common error function (guestfs___external_command_failed), and uses them all over the library and tools when converting exit status in error messages etc. --- fish/man.c | 7 +++- fuse/test-guestunmount-fd.c | 37 +++++++------------ src/appliance.c | 2 +- src/command.c | 15 +++----- src/dbdump.c | 2 +- src/errors.c | 33 +++++++++++++++++ src/filearch.c | 6 ++- src/guestfs-internal-frontend.h | 1 + src/guestfs-internal.h | 1 + src/info.c | 4 +- src/launch-appliance.c | 21 ++++------- src/launch-libvirt.c | 2 +- src/lpj.c | 11 +++++- src/utils.c | 35 ++++++++++++++++++ tests/mount-local/test-parallel-mount-local.c | 20 ++++------ 15 files changed, 126 insertions(+), 71 deletions(-) diff --git a/fish/man.c b/fish/man.c index a7106079a..71f7486ef 100644 --- a/fish/man.c +++ b/fish/man.c @@ -52,7 +52,12 @@ run_man (const char *cmd, size_t argc, char *argv[]) if (r != 0) return -1; if (!WIFEXITED (r) || WEXITSTATUS (r) != 0) { - fprintf (stderr, _("the external 'man' program failed\n")); + char status_string[80]; + + fprintf (stderr, "%s\n", + guestfs___exit_status_to_string (r, "man", + status_string, + sizeof status_string)); return -1; } diff --git a/fuse/test-guestunmount-fd.c b/fuse/test-guestunmount-fd.c index d607b6619..9594ff5a1 100644 --- a/fuse/test-guestunmount-fd.c +++ b/fuse/test-guestunmount-fd.c @@ -35,8 +35,6 @@ #include "guestfs.h" #include "guestfs-internal-frontend.h" -static void display_exit_status (int status, FILE *fp); - int main (int argc, char *argv[]) { @@ -82,9 +80,12 @@ main (int argc, char *argv[]) exit (EXIT_FAILURE); } if (r != 0) { - fprintf (stderr, "%s: test failed: guestunmount unexpectedly ", argv[0]); - display_exit_status (status, stderr); - fputc ('\n', stderr); + char status_string[80]; + + fprintf (stderr, "%s: test failed: %s\n", argv[0], + guestfs___exit_status_to_string (r, "guestunmount", + status_string, + sizeof status_string)); exit (EXIT_FAILURE); } @@ -100,27 +101,15 @@ main (int argc, char *argv[]) exit (EXIT_FAILURE); } if (!WIFEXITED (status) || WEXITSTATUS (status) != 2) { - fprintf (stderr, "%s: test failed: guestunmount didn't return status code 2; instead it ", argv[0]); - display_exit_status (status, stderr); - fputc ('\n', stderr); + char status_string[80]; + + fprintf (stderr, "%s: test failed: guestunmount didn't return status code 2; %s\n", + argv[0], + guestfs___exit_status_to_string (status, "guestunmount", + status_string, + sizeof status_string)); exit (EXIT_FAILURE); } exit (EXIT_SUCCESS); } - -static void -display_exit_status (int status, FILE *fp) -{ - if (WIFEXITED (status)) - fprintf (fp, "exited with status code %d", WEXITSTATUS (status)); - else if (WIFSIGNALED (status)) { - fprintf (fp, "exited on signal %d", WTERMSIG (status)); - if (WCOREDUMP (status)) - fprintf (fp, " and dumped core"); - } - else if (WIFSTOPPED (status)) - fprintf (fp, "stopped on signal %d", WSTOPSIG (status)); - else - fprintf (fp, "<< unknown status %d >>", status); -} diff --git a/src/appliance.c b/src/appliance.c index 37e2a5546..f7976a8d4 100644 --- a/src/appliance.c +++ b/src/appliance.c @@ -689,7 +689,7 @@ run_supermin_helper (guestfs_h *g, const char *supermin_path, if (r == -1) return -1; if (!WIFEXITED (r) || WEXITSTATUS (r) != 0) { - error (g, _("external command failed, see earlier error messages")); + guestfs___external_command_failed (g, r, SUPERMIN_HELPER, NULL); return -1; } diff --git a/src/command.c b/src/command.c index 2c9f8e868..03abc8b16 100644 --- a/src/command.c +++ b/src/command.c @@ -366,6 +366,7 @@ run_command (struct command *cmd) int i, fd, max_fd, r; int errorfd[2] = { -1, -1 }; int outfd[2] = { -1, -1 }; + char status_string[80]; /* Set up a pipe to capture command output and send it to the error log. */ if (cmd->capture_errors) { @@ -470,16 +471,10 @@ run_command (struct command *cmd) } if (WIFEXITED (r)) _exit (WEXITSTATUS (r)); - if (WIFSIGNALED (r)) { - fprintf (stderr, "%s: received signal %d\n", cmd->string.str, - WTERMSIG (r)); - _exit (EXIT_FAILURE); - } - if (WIFSTOPPED (r)) { - fprintf (stderr, "%s: stopped by signal %d\n", cmd->string.str, - WSTOPSIG (r)); - _exit (EXIT_FAILURE); - } + fprintf (stderr, "%s\n", + guestfs___exit_status_to_string (r, cmd->string.str, + status_string, + sizeof status_string)); _exit (EXIT_FAILURE); case COMMAND_STYLE_NOT_SELECTED: diff --git a/src/dbdump.c b/src/dbdump.c index 75fd25ec9..e147a111e 100644 --- a/src/dbdump.c +++ b/src/dbdump.c @@ -82,7 +82,7 @@ guestfs___read_db_dump (guestfs_h *g, if (r == -1) return -1; if (!WIFEXITED (r) || WEXITSTATUS (r) != 0) { - error (g, _("%s: command failed"), DB_DUMP); + guestfs___external_command_failed (g, r, DB_DUMP, NULL); return -1; } if (data.state != reading_finished) { diff --git a/src/errors.c b/src/errors.c index 8af272c22..114806e78 100644 --- a/src/errors.c +++ b/src/errors.c @@ -304,3 +304,36 @@ guestfs___unexpected_close_error (guestfs_h *g) "See http://libguestfs.org/guestfs-faq.1.html#debugging-libguestfs\n" "for information about how to debug libguestfs and report bugs.")); } + +/* External command failed. */ +void +guestfs___external_command_failed (guestfs_h *g, int status, + const char *cmd_name, const char *extra) +{ + size_t len = 80 + strlen (cmd_name); + char status_string[len]; + + guestfs___exit_status_to_string (status, cmd_name, status_string, len); + + if (g->verbose) { + if (!extra) + error (g, _("%s, see debug messages above"), status_string); + else + error (g, _("%s: %s: %s, see debug messages above"), + cmd_name, extra, status_string); + } + else { + if (!extra) + error (g, _( +"%s.\n" +"To see full error messages you may need to enable debugging.\n" +"See http://libguestfs.org/guestfs-faq.1.html#debugging-libguestfs"), + status_string); + else + error (g, _( +"%s: %s: %s.\n" +"To see full error messages you may need to enable debugging.\n" +"See http://libguestfs.org/guestfs-faq.1.html#debugging-libguestfs"), + cmd_name, extra, status_string); + } +} diff --git a/src/filearch.c b/src/filearch.c index a0be19d81..90d41dddb 100644 --- a/src/filearch.c +++ b/src/filearch.c @@ -181,8 +181,10 @@ cpio_arch (guestfs_h *g, const char *file, const char *path) } r = guestfs___cmd_run (cmd); - if (r == -1 || !WIFEXITED (r) || WEXITSTATUS (r) != 0) { - error (g, _("cpio command failed (status 0x%x)"), r); + if (r == -1) + goto out; + if (!WIFEXITED (r) || WEXITSTATUS (r) != 0) { + guestfs___external_command_failed (g, r, "cpio", path); goto out; } diff --git a/src/guestfs-internal-frontend.h b/src/guestfs-internal-frontend.h index 34b046d84..ac29f280f 100644 --- a/src/guestfs-internal-frontend.h +++ b/src/guestfs-internal-frontend.h @@ -91,6 +91,7 @@ extern char *guestfs___safe_asprintf (guestfs_h *g, const char *fs, ...) /* utils.c */ extern void guestfs___free_string_list (char **); extern size_t guestfs___count_strings (char *const *); +extern char *guestfs___exit_status_to_string (int status, const char *cmd_name, char *buffer, size_t buflen); /* These functions are used internally by the CLEANUP_* macros. * Don't call them directly. diff --git a/src/guestfs-internal.h b/src/guestfs-internal.h index fd6be0df2..a5ac40373 100644 --- a/src/guestfs-internal.h +++ b/src/guestfs-internal.h @@ -508,6 +508,7 @@ extern void guestfs___print_BufferOut (FILE *out, const char *buf, size_t buf_si extern void guestfs___launch_failed_error (guestfs_h *g); extern void guestfs___unexpected_close_error (guestfs_h *g); +extern void guestfs___external_command_failed (guestfs_h *g, int status, const char *cmd_name, const char *extra); /* actions-support.c */ struct trace_buffer { diff --git a/src/info.c b/src/info.c index 1560c5d5a..577bb90c8 100644 --- a/src/info.c +++ b/src/info.c @@ -246,7 +246,7 @@ get_json_output (guestfs_h *g, const char *filename) if (r == -1) return NULL; if (!WIFEXITED (r) || WEXITSTATUS (r) != 0) { - error (g, _("qemu-img info: %s: child process failed"), filename); + guestfs___external_command_failed (g, r, "qemu-img info", filename); return NULL; } @@ -508,7 +508,7 @@ old_parser_run_qemu_img_info (guestfs_h *g, const char *filename, if (r == -1) return -1; if (!WIFEXITED (r) || WEXITSTATUS (r) != 0) { - error (g, _("qemu-img: %s: child process failed"), filename); + guestfs___external_command_failed (g, r, "qemu-img info", filename); return -1; } diff --git a/src/launch-appliance.c b/src/launch-appliance.c index adaf498f3..122f14369 100644 --- a/src/launch-appliance.c +++ b/src/launch-appliance.c @@ -785,7 +785,10 @@ test_qemu (guestfs_h *g) return 0; error: - error (g, _("qemu command failed\nIf qemu is located on a non-standard path, try setting the LIBGUESTFS_QEMU\nenvironment variable. There may also be errors printed above.")); + if (r == -1) + return -1; + + guestfs___external_command_failed (g, r, g->qemu, NULL); return -1; } @@ -995,7 +998,7 @@ static int shutdown_appliance (guestfs_h *g, int check_for_errors) { int ret = 0; - int status, sig; + int status; /* Signal qemu to shutdown cleanly, and kill the recovery process. */ if (g->app.pid > 0) { @@ -1010,18 +1013,8 @@ shutdown_appliance (guestfs_h *g, int check_for_errors) perrorf (g, "waitpid (qemu)"); ret = -1; } - else if (WIFEXITED (status) && WEXITSTATUS (status) != 0) { - error (g, "qemu failed (status %d)", WEXITSTATUS (status)); - ret = -1; - } - else if (WIFSIGNALED (status)) { - sig = WTERMSIG (status); - error (g, "qemu terminated by signal %d (%s)", sig, strsignal (sig)); - ret = -1; - } - else if (WIFSTOPPED (status)) { - sig = WSTOPSIG (status); - error (g, "qemu stopped by signal %d (%s)", sig, strsignal (sig)); + else if (!WIFEXITED (status) || WEXITSTATUS (status) != 0) { + guestfs___external_command_failed (g, status, g->qemu, NULL); ret = -1; } } diff --git a/src/launch-libvirt.c b/src/launch-libvirt.c index 5cdf2ddbb..58ccb9e40 100644 --- a/src/launch-libvirt.c +++ b/src/launch-libvirt.c @@ -1427,7 +1427,7 @@ make_qcow2_overlay (guestfs_h *g, const char *path, const char *format, if (r == -1) goto error; if (!WIFEXITED (r) || WEXITSTATUS (r) != 0) { - error (g, _("qemu-img create: could not create snapshot over %s"), path); + guestfs___external_command_failed (g, r, "qemu-img create", path); goto error; } diff --git a/src/lpj.c b/src/lpj.c index b029d9845..6bbee80e7 100644 --- a/src/lpj.c +++ b/src/lpj.c @@ -142,8 +142,15 @@ read_lpj_common (guestfs_h *g, const char *func, struct command *cmd) guestfs___cmd_set_stdout_callback (cmd, read_all, &buf, CMD_STDOUT_FLAG_WHOLE_BUFFER); r = guestfs___cmd_run (cmd); - if (r == -1 || !WIFEXITED (r) || WEXITSTATUS (r) != 0) { - debug (g, "%s: external command failed with code %d", func, r); + if (r == -1) + return -1; + if (!WIFEXITED (r) || WEXITSTATUS (r) != 0) { + char status_string[80]; + + debug (g, "%s: %s", func, + guestfs___exit_status_to_string (r, "external command", + status_string, + sizeof status_string)); return -1; } diff --git a/src/utils.c b/src/utils.c index 1bc404280..7391455be 100644 --- a/src/utils.c +++ b/src/utils.c @@ -20,8 +20,12 @@ #include #include +#include #include #include +#include +#include +#include #include "guestfs.h" #include "guestfs-internal-frontend.h" @@ -49,3 +53,34 @@ guestfs___count_strings (char *const *argv) return r; } + +/* Translate a wait/system exit status into a printable string. The + * string must be freed by the caller. + */ +char * +guestfs___exit_status_to_string (int status, const char *cmd_name, + char *buffer, size_t buflen) +{ + if (WIFEXITED (status)) { + if (WEXITSTATUS (status) == 0) + snprintf (buffer, buflen, _("%s exited successfully"), + cmd_name); + else + snprintf (buffer, buflen, _("%s exited with error status %d"), + cmd_name, WEXITSTATUS (status)); + } + else if (WIFSIGNALED (status)) { + snprintf (buffer, buflen, _("%s killed by signal %d (%s)"), + cmd_name, WTERMSIG (status), strsignal (WTERMSIG (status))); + } + else if (WIFSTOPPED (status)) { + snprintf (buffer, buflen, _("%s stopped by signal %d (%s)"), + cmd_name, WSTOPSIG (status), strsignal (WSTOPSIG (status))); + } + else { + snprintf (buffer, buflen, _("%s exited for an unknown reason (status %d)"), + cmd_name, status); + } + + return buffer; +} diff --git a/tests/mount-local/test-parallel-mount-local.c b/tests/mount-local/test-parallel-mount-local.c index 74b04ac7e..bfa3e7651 100644 --- a/tests/mount-local/test-parallel-mount-local.c +++ b/tests/mount-local/test-parallel-mount-local.c @@ -258,20 +258,14 @@ start_thread (void *statevp) perror ("waitpid"); goto error; } - if (WIFEXITED (status)) { - if (WEXITSTATUS (status) != 0) { - fprintf (stderr, "%s: test exited with non-zero status %d\n", - state->mp, WEXITSTATUS (status)); + if (!WIFEXITED (status) || WEXITSTATUS (status) != 0) { + char status_string[80]; + + fprintf (stderr, "%s: %s\n", state->mp, + guestfs___exit_status_to_string (status, "test", + status_string, + sizeof status_string)); goto error; - } - } else if (WIFSIGNALED (status)) { - fprintf (stderr, "%s: subprocess killed by signal %d\n", - state->mp, WTERMSIG (status)); - goto error; - } else if (WIFSTOPPED (status)) { - fprintf (stderr, "%s: subprocess stopped by signal %d\n", - state->mp, WSTOPSIG (status)); - goto error; } if (r == -1) /* guestfs_mount_local_run above failed */