On Tue, 2016-09-27 at 14:52 +1000, David Gibson wrote: > On Thu, Sep 22, 2016 at 09:33:05PM -0600, Kevin Locke wrote: >> - Create fread_noeintr to avoid EINTR in fread without reading any data. >> - Handle short reads from fread. This can happen with non-conformant >> libc or if EINTR occurs after reading some data. > > Hrm. Have you actually confirmed this problem with EINTR? My > previous objections to not handling short read were based entirely on > not realizing that fread() wasn't supposed to do that, rather than > thinking of some edge case you hadn't. > > Unless we have a confirmed case where fread() can give a short read > that won't have feof() or ferror() set afterwards, I'm inclined to go > back to your initial implementation.
Nope, I was just being overly cautious. Sounds good to me. Below is an updated patch which is the same as v1 but with the addition of the #define for _POSIX_C_SOURCE to get pclose, popen, and strdup. -8<-------------------------------------------------------------------- Rather than using fork+pipe+system+waitpid, most of which are only available on POSIX-like systems, use popen which is also available on Windows (under the name _popen). Changes since v1: - Create fread_noeintr to avoid EINTR in fread without reading any data. - Handle short reads from fread. This can happen with non-conformant libc or if EINTR occurs after reading some data. - Define _POSIX_C_SOURCE for popen/pclose with strict implementations which require it (e.g. gcc with -std=c11). Changes since v2: - Revert fread_noeintr and short read changes in v1 as unnecessary. Signed-off-by: Kevin Locke <ke...@kevinlocke.name> --- tools/configurator/configurator.c | 79 +++++++++++++++------------------------ 1 file changed, 31 insertions(+), 48 deletions(-) diff --git a/tools/configurator/configurator.c b/tools/configurator/configurator.c index c7577c0..2ad3fff 100644 --- a/tools/configurator/configurator.c +++ b/tools/configurator/configurator.c @@ -21,17 +21,19 @@ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN * THE SOFTWARE. */ +#define _POSIX_C_SOURCE 200809L /* For pclose, popen, strdup */ + #include <stdio.h> #include <stdbool.h> #include <stdlib.h> -#include <unistd.h> #include <err.h> -#include <sys/types.h> -#include <sys/wait.h> -#include <sys/stat.h> -#include <fcntl.h> #include <string.h> +#ifdef _MSC_VER +#define popen _popen +#define pclose _pclose +#endif + #define DEFAULT_COMPILER "cc" #define DEFAULT_FLAGS "-g3 -ggdb -Wall -Wundef -Wmissing-prototypes -Wmissing-declarations -Wstrict-prototypes -Wold-style-definition" @@ -375,20 +377,19 @@ static struct test tests[] = { }, }; -static char *grab_fd(int fd) +static char *grab_stream(FILE *file) { - int ret; - size_t max, size = 0; + size_t max, ret, size = 0; char *buffer; - max = 16384; - buffer = malloc(max+1); - while ((ret = read(fd, buffer + size, max - size)) > 0) { + max = BUFSIZ; + buffer = malloc(max); + while ((ret = fread(buffer+size, 1, max - size, file)) == max - size) { size += ret; - if (size == max) - buffer = realloc(buffer, max *= 2); + buffer = realloc(buffer, max *= 2); } - if (ret < 0) + size += ret; + if (ferror(file)) err(1, "reading from command"); buffer[size] = '\0'; return buffer; @@ -396,43 +397,25 @@ static char *grab_fd(int fd) static char *run(const char *cmd, int *exitstatus) { - pid_t pid; - int p[2]; + static const char redir[] = " 2>&1"; + size_t cmdlen; + char *cmdredir; + FILE *cmdout; char *ret; - int status; - if (pipe(p) != 0) - err(1, "creating pipe"); - - pid = fork(); - if (pid == -1) - err(1, "forking"); - - if (pid == 0) { - if (dup2(p[1], STDOUT_FILENO) != STDOUT_FILENO - || dup2(p[1], STDERR_FILENO) != STDERR_FILENO - || close(p[0]) != 0 - || close(STDIN_FILENO) != 0 - || open("/dev/null", O_RDONLY) != STDIN_FILENO) - exit(128); - - status = system(cmd); - if (WIFEXITED(status)) - exit(WEXITSTATUS(status)); - /* Here's a hint... */ - exit(128 + WTERMSIG(status)); - } + cmdlen = strlen(cmd); + cmdredir = malloc(cmdlen + sizeof(redir)); + memcpy(cmdredir, cmd, cmdlen); + memcpy(cmdredir + cmdlen, redir, sizeof(redir)); + + cmdout = popen(cmdredir, "r"); + if (!cmdout) + err(1, "popen \"%s\"", cmdredir); + + free(cmdredir); - close(p[1]); - ret = grab_fd(p[0]); - /* This shouldn't fail... */ - if (waitpid(pid, &status, 0) != pid) - err(1, "Failed to wait for child"); - close(p[0]); - if (WIFEXITED(status)) - *exitstatus = WEXITSTATUS(status); - else - *exitstatus = -WTERMSIG(status); + ret = grab_stream(cmdout); + *exitstatus = pclose(cmdout); return ret; } -- 2.9.3 _______________________________________________ ccan mailing list ccan@lists.ozlabs.org https://lists.ozlabs.org/listinfo/ccan