This reverts commit c5d3d198dcbceac928f430e2a9450ca58d6c0890. --- src/command.c | 64 +++++++------------------------------------------- src/guestfs-internal.h | 3 --- 2 files changed, 9 insertions(+), 58 deletions(-) diff --git a/src/command.c b/src/command.c index a92012d..126cc49 100644 --- a/src/command.c +++ b/src/command.c @@ -409,7 +409,7 @@ debug_command (struct command *cmd) } static int -run_command (struct command *cmd, bool get_stdout_fd, bool get_stderr_fd) +run_command (struct command *cmd) { struct sigaction sa; int i, fd, max_fd, r; @@ -421,11 +421,8 @@ run_command (struct command *cmd, bool get_stdout_fd, bool get_stderr_fd) struct rlimit rlimit; #endif - get_stdout_fd = get_stdout_fd || cmd->stdout_callback != NULL; - get_stderr_fd = get_stderr_fd || cmd->capture_errors; - /* Set up a pipe to capture command output and send it to the error log. */ - if (get_stderr_fd) { + if (cmd->capture_errors) { if (pipe2 (errorfd, O_CLOEXEC) == -1) { perrorf (cmd->g, "pipe2"); goto error; @@ -433,7 +430,7 @@ run_command (struct command *cmd, bool get_stdout_fd, bool get_stderr_fd) } /* Set up a pipe to capture stdout for the callback. */ - if (get_stdout_fd) { + if (cmd->stdout_callback) { if (pipe2 (outfd, O_CLOEXEC) == -1) { perrorf (cmd->g, "pipe2"); goto error; @@ -448,14 +445,14 @@ run_command (struct command *cmd, bool get_stdout_fd, bool get_stderr_fd) /* In parent, return to caller. */ if (cmd->pid > 0) { - if (get_stderr_fd) { + if (cmd->capture_errors) { close (errorfd[1]); errorfd[1] = -1; cmd->errorfd = errorfd[0]; errorfd[0] = -1; } - if (get_stdout_fd) { + if (cmd->stdout_callback) { close (outfd[1]); outfd[1] = -1; cmd->outfd = outfd[0]; @@ -466,15 +463,15 @@ run_command (struct command *cmd, bool get_stdout_fd, bool get_stderr_fd) } /* Child process. */ - if (get_stderr_fd) { + if (cmd->capture_errors) { close (errorfd[0]); - if (!get_stdout_fd) + if (!cmd->stdout_callback) dup2 (errorfd[1], 1); dup2 (errorfd[1], 2); close (errorfd[1]); } - if (get_stdout_fd) { + if (cmd->stdout_callback) { close (outfd[0]); dup2 (outfd[1], 1); close (outfd[1]); @@ -693,7 +690,7 @@ guestfs_int_cmd_run (struct command *cmd) if (cmd->g->verbose) debug_command (cmd); - if (run_command (cmd, false, false) == -1) + if (run_command (cmd) == -1) return -1; if (loop (cmd) == -1) @@ -702,49 +699,6 @@ guestfs_int_cmd_run (struct command *cmd) return wait_command (cmd); } -/* Fork, run the command, and returns the pid of the command, - * and its stdout and stderr file descriptors. - * - * Returns the exit status. Test it using WIF* macros. - * - * On error: Calls error(g) and returns -1. - */ -int -guestfs_int_cmd_run_async (struct command *cmd, pid_t *pid, - int *stdout_fd, int *stderr_fd) -{ - finish_command (cmd); - - if (cmd->g->verbose) - debug_command (cmd); - - if (run_command (cmd, stdout_fd != NULL, stderr_fd != NULL) == -1) - return -1; - - if (pid) - *pid = cmd->pid; - if (stdout_fd) - *stdout_fd = cmd->outfd; - if (stderr_fd) - *stderr_fd = cmd->errorfd; - - return 0; -} - -/* Wait for the command to finish. - * - * The command MUST have been started with guestfs_int_cmd_run_async. - * - * Returns the exit status. Test it using WIF* macros. - * - * On error: Calls error(g) and returns -1. - */ -int -guestfs_int_cmd_wait (struct command *cmd) -{ - return wait_command (cmd); -} - void guestfs_int_cmd_close (struct command *cmd) { diff --git a/src/guestfs-internal.h b/src/guestfs-internal.h index 665dc93..22e8e06 100644 --- a/src/guestfs-internal.h +++ b/src/guestfs-internal.h @@ -20,7 +20,6 @@ #define GUESTFS_INTERNAL_H_ #include <stdbool.h> -#include <sys/types.h> #include <libintl.h> @@ -878,8 +877,6 @@ extern void guestfs_int_cmd_clear_capture_errors (struct command *); extern void guestfs_int_cmd_clear_close_files (struct command *); extern void guestfs_int_cmd_set_child_callback (struct command *, cmd_child_callback child_callback, void *data); extern int guestfs_int_cmd_run (struct command *); -extern int guestfs_int_cmd_run_async (struct command *, pid_t *pid, int *stdout_fd, int *stderr_fd); -extern int guestfs_int_cmd_wait (struct command *); extern void guestfs_int_cmd_close (struct command *); #ifdef HAVE_ATTRIBUTE_CLEANUP -- 2.5.0

Allow this useful function to be called from elsewhere. This commit is almost, but not completely refactoring. I made a minor change to the function so that it \0-terminates the returned data, which is convenient for some callers. --- po/POTFILES | 1 + src/Makefile.am | 1 + src/guestfs-internal.h | 3 ++ src/inspect-icon.c | 72 +++------------------------------------ src/whole-file.c | 91 ++++++++++++++++++++++++++++++++++++++++++++++++++ 5 files changed, 101 insertions(+), 67 deletions(-) create mode 100644 src/whole-file.c diff --git a/po/POTFILES b/po/POTFILES index bb68183..9d92690 100644 --- a/po/POTFILES +++ b/po/POTFILES @@ -349,6 +349,7 @@ src/structs-free.c src/test-utils.c src/tmpdirs.c src/utils.c +src/whole-file.c test-tool/test-tool.c v2v/domainxml-c.c v2v/kvmuid-c.c diff --git a/src/Makefile.am b/src/Makefile.am index 38a4f50..f5ecaa6 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -126,6 +126,7 @@ libguestfs_la_SOURCES = \ structs-copy.c \ structs-free.c \ tmpdirs.c \ + whole-file.c \ libguestfs.syms libguestfs_la_CPPFLAGS = \ diff --git a/src/guestfs-internal.h b/src/guestfs-internal.h index 22e8e06..75ca71c 100644 --- a/src/guestfs-internal.h +++ b/src/guestfs-internal.h @@ -754,6 +754,9 @@ extern int guestfs_int_lazy_make_tmpdir (guestfs_h *g); extern void guestfs_int_remove_tmpdir (guestfs_h *g); extern void guestfs_int_recursive_remove_dir (guestfs_h *g, const char *dir); +/* whole-file.c */ +extern int guestfs_int_read_whole_file (guestfs_h *g, const char *filename, char **data_r, size_t *size_r); + /* drives.c */ extern size_t guestfs_int_checkpoint_drives (guestfs_h *g); extern void guestfs_int_rollback_drives (guestfs_h *g, size_t); diff --git a/src/inspect-icon.c b/src/inspect-icon.c index 33d0aa7..15c721b 100644 --- a/src/inspect-icon.c +++ b/src/inspect-icon.c @@ -25,7 +25,6 @@ #include <unistd.h> #include <fcntl.h> #include <string.h> -#include <sys/stat.h> #include <errno.h> #include <sys/wait.h> @@ -43,8 +42,6 @@ #define CAN_DO_WINDOWS 1 #endif -static int read_whole_file (guestfs_h *g, const char *filename, char **data_r, size_t *size_r); - /* All these icon_*() functions return the same way. One of: * * ret == NULL: @@ -272,7 +269,7 @@ get_png (guestfs_h *g, struct inspect_fs *fs, const char *filename, return NOT_FOUND; /* Successfully passed checks and downloaded. Read it into memory. */ - if (read_whole_file (g, local, &ret, size_r) == -1) + if (guestfs_int_read_whole_file (g, local, &ret, size_r) == -1) return NULL; return ret; @@ -419,7 +416,7 @@ icon_cirros (guestfs_h *g, struct inspect_fs *fs, size_t *size_r) return NOT_FOUND; /* Read it into memory. */ - if (read_whole_file (g, pngfile, &ret, size_r) == -1) + if (guestfs_int_read_whole_file (g, pngfile, &ret, size_r) == -1) return NULL; return ret; @@ -488,7 +485,7 @@ icon_windows_xp (guestfs_h *g, struct inspect_fs *fs, size_t *size_r) if (!WIFEXITED (r) || WEXITSTATUS (r) != 0) return NOT_FOUND; - if (read_whole_file (g, pngfile, &ret, size_r) == -1) + if (guestfs_int_read_whole_file (g, pngfile, &ret, size_r) == -1) return NULL; return ret; @@ -540,7 +537,7 @@ icon_windows_7 (guestfs_h *g, struct inspect_fs *fs, size_t *size_r) if (!WIFEXITED (r) || WEXITSTATUS (r) != 0) return NOT_FOUND; - if (read_whole_file (g, pngfile, &ret, size_r) == -1) + if (guestfs_int_read_whole_file (g, pngfile, &ret, size_r) == -1) return NULL; return ret; @@ -577,7 +574,7 @@ icon_windows_8 (guestfs_h *g, struct inspect_fs *fs, size_t *size_r) if (filename_downloaded == NULL) return NOT_FOUND; - if (read_whole_file (g, filename_downloaded, &ret, size_r) == -1) + if (guestfs_int_read_whole_file (g, filename_downloaded, &ret, size_r) == -1) return NULL; return ret; @@ -606,62 +603,3 @@ icon_windows (guestfs_h *g, struct inspect_fs *fs, size_t *size_r) } #endif /* CAN_DO_WINDOWS */ - -/* Read the whole file into a memory buffer and return it. The file - * should be a regular, local, trusted file. - */ -static int -read_whole_file (guestfs_h *g, const char *filename, - char **data_r, size_t *size_r) -{ - int fd; - char *data; - off_t size; - off_t n; - ssize_t r; - struct stat statbuf; - - fd = open (filename, O_RDONLY|O_CLOEXEC); - if (fd == -1) { - perrorf (g, "open: %s", filename); - return -1; - } - - if (fstat (fd, &statbuf) == -1) { - perrorf (g, "stat: %s", filename); - close (fd); - return -1; - } - - size = statbuf.st_size; - data = safe_malloc (g, size); - - n = 0; - while (n < size) { - r = read (fd, &data[n], size - n); - if (r == -1) { - perrorf (g, "read: %s", filename); - free (data); - close (fd); - return -1; - } - if (r == 0) { - error (g, _("read: %s: unexpected end of file"), filename); - free (data); - close (fd); - return -1; - } - n += r; - } - - if (close (fd) == -1) { - perrorf (g, "close: %s", filename); - free (data); - return -1; - } - - *data_r = data; - *size_r = size; - - return 0; -} diff --git a/src/whole-file.c b/src/whole-file.c new file mode 100644 index 0000000..38050d9 --- /dev/null +++ b/src/whole-file.c @@ -0,0 +1,91 @@ +/* libguestfs + * Copyright (C) 2011-2015 Red Hat Inc. + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library; if not, write to the Free Software + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA + */ + +#include <config.h> + +#include <stdio.h> +#include <stdlib.h> +#include <unistd.h> +#include <fcntl.h> +#include <sys/stat.h> + +#include "guestfs.h" +#include "guestfs-internal.h" + +/* Read the whole file into a memory buffer and return it. The file + * should be a regular, local, trusted file. + */ +int +guestfs_int_read_whole_file (guestfs_h *g, const char *filename, + char **data_r, size_t *size_r) +{ + int fd; + char *data; + off_t size; + off_t n; + ssize_t r; + struct stat statbuf; + + fd = open (filename, O_RDONLY|O_CLOEXEC); + if (fd == -1) { + perrorf (g, "open: %s", filename); + return -1; + } + + if (fstat (fd, &statbuf) == -1) { + perrorf (g, "stat: %s", filename); + close (fd); + return -1; + } + + size = statbuf.st_size; + data = safe_malloc (g, size + 1); + + n = 0; + while (n < size) { + r = read (fd, &data[n], size - n); + if (r == -1) { + perrorf (g, "read: %s", filename); + free (data); + close (fd); + return -1; + } + if (r == 0) { + error (g, _("read: %s: unexpected end of file"), filename); + free (data); + close (fd); + return -1; + } + n += r; + } + + if (close (fd) == -1) { + perrorf (g, "close: %s", filename); + free (data); + return -1; + } + + /* For convenience of callers, \0-terminate the data. */ + data[size] = '\0'; + + *data_r = data; + if (size_r != NULL) + *size_r = size; + + return 0; +} -- 2.5.0

Roughly equivalent to popen(...,"r"|"w") the new internal API allows you to run a subprocess and either read its stdout, or write to its stdin. --- src/command.c | 149 +++++++++++++++++++++++++++++++++++++++++++++++++ src/guestfs-internal.h | 3 + 2 files changed, 152 insertions(+) diff --git a/src/command.c b/src/command.c index 8fb0714..0547111 100644 --- a/src/command.c +++ b/src/command.c @@ -133,6 +133,9 @@ struct command bool capture_errors; int errorfd; + /* When using the pipe_* APIs, stderr is pointed to a temporary file. */ + char *error_file; + /* Close file descriptors (defaults to true). */ bool close_files; @@ -711,6 +714,147 @@ guestfs_int_cmd_run (struct command *cmd) return wait_command (cmd); } +/* Fork and run the command, but don't wait. Roughly equivalent to + * popen (..., "r"|"w"). + * + * Returns the file descriptor of the pipe, connected to stdout ("r") + * or stdin ("w") of the child process. + * + * After reading/writing to this pipe, call guestfs_int_cmd_pipe_wait + * to wait for the status of the child. + * + * Errors from the subcommand cannot be captured to the error log + * using this interface. Instead the caller should call + * guestfs_int_cmd_get_pipe_errors (after guestfs_int_cmd_pipe_wait + * returns an error). + */ +int +guestfs_int_cmd_pipe_run (struct command *cmd, const char *mode) +{ + int fd[2] = { -1, -1 }; + int errfd = -1; + int r_mode; + int ret; + + finish_command (cmd); + + /* Various options cannot be used here. */ + assert (!cmd->capture_errors); + assert (!cmd->stdout_callback); + assert (!cmd->stderr_to_stdout); + + if (STREQ (mode, "r")) r_mode = 1; + else if (STREQ (mode, "w")) r_mode = 0; + else abort (); + + if (pipe2 (fd, O_CLOEXEC) == -1) { + perrorf (cmd->g, "pipe2"); + goto error; + } + + /* We can't easily capture errors from the child process, so instead + * we write them into a temporary file and provide a separate + * function for the caller to read the error messages. + */ + if (guestfs_int_lazy_make_tmpdir (cmd->g) == -1) + goto error; + + cmd->error_file = + safe_asprintf (cmd->g, "%s/cmderr.%d", cmd->g->tmpdir, ++cmd->g->unique); + errfd = open (cmd->error_file, + O_WRONLY|O_CREAT|O_NOCTTY|O_TRUNC|O_CLOEXEC, 0600); + if (errfd == -1) { + perrorf (cmd->g, "open: %s", cmd->error_file); + goto error; + } + + cmd->pid = fork (); + if (cmd->pid == -1) { + perrorf (cmd->g, "fork"); + goto error; + } + + /* Parent. */ + if (cmd->pid > 0) { + close (errfd); + errfd = -1; + + if (r_mode) { + close (fd[1]); + ret = fd[0]; + } + else { + close (fd[0]); + ret = fd[1]; + } + + return ret; + } + + /* Child. */ + dup2 (errfd, 2); + close (errfd); + + if (r_mode) { + close (fd[0]); + dup2 (fd[1], 1); + close (fd[1]); + } + else { + close (fd[1]); + dup2 (fd[0], 0); + close (fd[0]); + } + + run_child (cmd); + /*NOTREACHED*/ + + error: + if (errfd >= 0) + close (errfd); + if (fd[0] >= 0) + close (fd[0]); + if (fd[1] >= 0) + close (fd[1]); + return -1; +} + +/* Wait for a subprocess created by guestfs_int_cmd_pipe_run to + * finish. On error (eg. failed syscall) this returns -1 and sets the + * error. If the subcommand fails, then use WIF* macros to check + * this, and call guestfs_int_cmd_get_pipe_errors to read the error + * messages printed by the child. + */ +int +guestfs_int_cmd_pipe_wait (struct command *cmd) +{ + return wait_command (cmd); +} + +/* Read the error messages printed by the child. The caller must free + * the returned buffer after use. + */ +char * +guestfs_int_cmd_get_pipe_errors (struct command *cmd) +{ + char *ret; + size_t len; + + assert (cmd->error_file != NULL); + + if (guestfs_int_read_whole_file (cmd->g, cmd->error_file, &ret, NULL) == -1) + return NULL; + + /* If the file ends with \n characters, trim them. */ + len = strlen (ret); + while (len > 0 && ret[len-1] == '\n') { + ret[len-1] = '\0'; + len--; + } + + return ret; +} + void guestfs_int_cmd_close (struct command *cmd) { @@ -733,6 +877,11 @@ guestfs_int_cmd_close (struct command *cmd) break; } + if (cmd->error_file != NULL) { + unlink (cmd->error_file); + free (cmd->error_file); + } + if (cmd->errorfd >= 0) close (cmd->errorfd); diff --git a/src/guestfs-internal.h b/src/guestfs-internal.h index 75ca71c..9c7175f 100644 --- a/src/guestfs-internal.h +++ b/src/guestfs-internal.h @@ -881,6 +881,9 @@ extern void guestfs_int_cmd_clear_close_files (struct command *); extern void guestfs_int_cmd_set_child_callback (struct command *, cmd_child_callback child_callback, void *data); extern int guestfs_int_cmd_run (struct command *); extern void guestfs_int_cmd_close (struct command *); +extern int guestfs_int_cmd_pipe_run (struct command *cmd, const char *mode); +extern int guestfs_int_cmd_pipe_wait (struct command *cmd); +extern char *guestfs_int_cmd_get_pipe_errors (struct command *cmd); #ifdef HAVE_ATTRIBUTE_CLEANUP #define CLEANUP_CMD_CLOSE __attribute__((cleanup(guestfs_int_cleanup_cmd_close))) -- 2.5.0