[PATCH libguestfs] guestfish: detect a few more failed syscalls
There were a few unchecked syscalls in fish.c
From ba8b8b0684a03b6e6fbb939ed7e1cbf5e1000092 Mon Sep 17 00:00:00 2001 From: Jim Meyering <meyering@redhat.com> Date: Wed, 19 Aug 2009 10:01:07 +0200 Subject: [PATCH libguestfs] guestfish: detect a few more failed syscalls
* fish/fish.c (issue_command): Detect/diagnose more failed syscalls. --- fish/fish.c | 26 +++++++++++++++++++++----- 1 files changed, 21 insertions(+), 5 deletions(-) diff --git a/fish/fish.c b/fish/fish.c index 830617b..e6cd270 100644 --- a/fish/fish.c +++ b/fish/fish.c @@ -750,8 +750,14 @@ issue_command (const char *cmd, char *argv[], const char *pipecmd) if (pipecmd) { int fd[2]; - fflush (stdout); - pipe (fd); + if (fflush (stdout)); { + perror ("failed to flush standard output"); + return -1; + } + if (pipe (fd)) { + perror ("pipe failed"); + return -1; + } pid = fork (); if (pid == -1) { perror ("fork"); @@ -760,7 +766,10 @@ issue_command (const char *cmd, char *argv[], const char *pipecmd) if (pid == 0) { /* Child process. */ close (fd[1]); - dup2 (fd[0], 0); + if (dup2 (fd[0], 0) < 0) { + perror ("dup2 of stdin failed"); + _exit (1); + } r = system (pipecmd); if (r == -1) { @@ -770,9 +779,16 @@ issue_command (const char *cmd, char *argv[], const char *pipecmd) _exit (WEXITSTATUS (r)); } - stdout_saved_fd = dup (1); + if ((stdout_saved_fd = dup (1)) < 0) { + perror ("failed to dup stdout"); + return -1; + } close (fd[0]); - dup2 (fd[1], 1); + if (dup2 (fd[1], 1)) { + perror ("failed to dup stdout"); + close (stdout_saved_fd); + return -1; + } close (fd[1]); } -- 1.6.4.378.g88f2f
Jim Meyering wrote:
There were a few unchecked syscalls in fish.c
From ba8b8b0684a03b6e6fbb939ed7e1cbf5e1000092 Mon Sep 17 00:00:00 2001 From: Jim Meyering <meyering@redhat.com> Date: Wed, 19 Aug 2009 10:01:07 +0200 Subject: [PATCH libguestfs] guestfish: detect a few more failed syscalls
* fish/fish.c (issue_command): Detect/diagnose more failed syscalls. ... - dup2 (fd[1], 1); + if (dup2 (fd[1], 1)) {
Rich noticed that this test is wrong (should test < 0). Thanks! Here's a fixed patch (also made the pipe test for < 0, though there it's not important):
From afee87d3b908bd1b6970e5b141452841d8dd5da8 Mon Sep 17 00:00:00 2001 From: Jim Meyering <meyering@redhat.com> Date: Wed, 19 Aug 2009 10:01:07 +0200 Subject: [PATCH libguestfs] guestfish: detect a few more failed syscalls
* fish/fish.c (issue_command): Detect/diagnose more failed syscalls. --- fish/fish.c | 26 +++++++++++++++++++++----- 1 files changed, 21 insertions(+), 5 deletions(-) diff --git a/fish/fish.c b/fish/fish.c index 830617b..5b2cf45 100644 --- a/fish/fish.c +++ b/fish/fish.c @@ -750,8 +750,14 @@ issue_command (const char *cmd, char *argv[], const char *pipecmd) if (pipecmd) { int fd[2]; - fflush (stdout); - pipe (fd); + if (fflush (stdout)); { + perror ("failed to flush standard output"); + return -1; + } + if (pipe (fd) < 0) { + perror ("pipe failed"); + return -1; + } pid = fork (); if (pid == -1) { perror ("fork"); @@ -760,7 +766,10 @@ issue_command (const char *cmd, char *argv[], const char *pipecmd) if (pid == 0) { /* Child process. */ close (fd[1]); - dup2 (fd[0], 0); + if (dup2 (fd[0], 0) < 0) { + perror ("dup2 of stdin failed"); + _exit (1); + } r = system (pipecmd); if (r == -1) { @@ -770,9 +779,16 @@ issue_command (const char *cmd, char *argv[], const char *pipecmd) _exit (WEXITSTATUS (r)); } - stdout_saved_fd = dup (1); + if ((stdout_saved_fd = dup (1)) < 0) { + perror ("failed to dup stdout"); + return -1; + } close (fd[0]); - dup2 (fd[1], 1); + if (dup2 (fd[1], 1) < 0) { + perror ("failed to dup stdout"); + close (stdout_saved_fd); + return -1; + } close (fd[1]); } -- 1.6.4.378.g88f2f
On 19/08/09 09:34, Jim Meyering wrote:
There were a few unchecked syscalls in fish.c
From ba8b8b0684a03b6e6fbb939ed7e1cbf5e1000092 Mon Sep 17 00:00:00 2001 From: Jim Meyering<meyering@redhat.com> Date: Wed, 19 Aug 2009 10:01:07 +0200 Subject: [PATCH libguestfs] guestfish: detect a few more failed syscalls
* fish/fish.c (issue_command): Detect/diagnose more failed syscalls. --- fish/fish.c | 26 +++++++++++++++++++++----- 1 files changed, 21 insertions(+), 5 deletions(-)
diff --git a/fish/fish.c b/fish/fish.c index 830617b..e6cd270 100644 --- a/fish/fish.c +++ b/fish/fish.c @@ -750,8 +750,14 @@ issue_command (const char *cmd, char *argv[], const char *pipecmd) if (pipecmd) { int fd[2];
- fflush (stdout); - pipe (fd); + if (fflush (stdout)); {
Looks like a stray semicolon in there. Also, wouldn't it be better form to test for test for a return value of EOF?
+ perror ("failed to flush standard output"); + return -1; + } + if (pipe (fd)) { + perror ("pipe failed"); + return -1; + }
Again, I'd explicitly test for a return value of -1 for consistency.
pid = fork (); if (pid == -1) { perror ("fork"); @@ -760,7 +766,10 @@ issue_command (const char *cmd, char *argv[], const char *pipecmd)
if (pid == 0) { /* Child process. */ close (fd[1]); - dup2 (fd[0], 0); + if (dup2 (fd[0], 0)< 0) { + perror ("dup2 of stdin failed"); + _exit (1); + }
fd[1] ought to be closed after this call. Not directly relevant to this patch, but '0' is a magic number. Use STDIN_FILENO from unistd.h.
r = system (pipecmd); if (r == -1) { @@ -770,9 +779,16 @@ issue_command (const char *cmd, char *argv[], const char *pipecmd) _exit (WEXITSTATUS (r)); }
- stdout_saved_fd = dup (1); + if ((stdout_saved_fd = dup (1))< 0) { + perror ("failed to dup stdout"); + return -1; + }
'1' is a magic number. Use STDOUT_FILENO from unistd.h.
close (fd[0]); - dup2 (fd[1], 1); + if (dup2 (fd[1], 1)) { + perror ("failed to dup stdout"); + close (stdout_saved_fd); + return -1; + }
Test for return value of -1. '1' is a magic number.
close (fd[1]); }
-- 1.6.4.378.g88f2f
Matt -- Matthew Booth, RHCA, RHCSS Red Hat Engineering, Virtualisation Team M: +44 (0)7977 267231 GPG ID: D33C3490 GPG FPR: 3733 612D 2D05 5458 8A8A 1600 3441 EA19 D33C 3490
Matthew Booth wrote:
On 19/08/09 09:34, Jim Meyering wrote:
There were a few unchecked syscalls in fish.c
From ba8b8b0684a03b6e6fbb939ed7e1cbf5e1000092 Mon Sep 17 00:00:00 2001 From: Jim Meyering<meyering@redhat.com> Date: Wed, 19 Aug 2009 10:01:07 +0200 Subject: [PATCH libguestfs] guestfish: detect a few more failed syscalls
* fish/fish.c (issue_command): Detect/diagnose more failed syscalls. --- fish/fish.c | 26 +++++++++++++++++++++----- 1 files changed, 21 insertions(+), 5 deletions(-)
diff --git a/fish/fish.c b/fish/fish.c index 830617b..e6cd270 100644 --- a/fish/fish.c +++ b/fish/fish.c @@ -750,8 +750,14 @@ issue_command (const char *cmd, char *argv[], const char *pipecmd) if (pipecmd) { int fd[2];
- fflush (stdout); - pipe (fd); + if (fflush (stdout)); {
Looks like a stray semicolon in there. Also, wouldn't it be better form to test for test for a return value of EOF?
Argh. Thanks. And yes, testing for EOF does seem better. I also noticed that there are other unchecked dup2 and fflush calls. Another round coming right up.
Jim Meyering wrote:
Matthew Booth wrote:
On 19/08/09 09:34, Jim Meyering wrote:
There were a few unchecked syscalls in fish.c
From ba8b8b0684a03b6e6fbb939ed7e1cbf5e1000092 Mon Sep 17 00:00:00 2001 From: Jim Meyering<meyering@redhat.com> Date: Wed, 19 Aug 2009 10:01:07 +0200 Subject: [PATCH libguestfs] guestfish: detect a few more failed syscalls
* fish/fish.c (issue_command): Detect/diagnose more failed syscalls. --- fish/fish.c | 26 +++++++++++++++++++++----- 1 files changed, 21 insertions(+), 5 deletions(-)
diff --git a/fish/fish.c b/fish/fish.c index 830617b..e6cd270 100644 --- a/fish/fish.c +++ b/fish/fish.c @@ -750,8 +750,14 @@ issue_command (const char *cmd, char *argv[], const char *pipecmd) if (pipecmd) { int fd[2];
- fflush (stdout); - pipe (fd); + if (fflush (stdout)); {
Looks like a stray semicolon in there. Also, wouldn't it be better form to test for test for a return value of EOF?
Argh. Thanks. And yes, testing for EOF does seem better.
I also noticed that there are other unchecked dup2 and fflush calls. Another round coming right up.
And waitpid. This compiles. "make check" in progress.
From f0e6f60d95296024de57a0970f151023f9196df3 Mon Sep 17 00:00:00 2001 From: Jim Meyering <meyering@redhat.com> Date: Wed, 19 Aug 2009 10:01:07 +0200 Subject: [PATCH libguestfs] guestfish: detect a more failed syscalls
* fish/fish.c (issue_command): Detect/diagnose more failed syscalls. --- fish/fish.c | 41 +++++++++++++++++++++++++++++++++-------- 1 files changed, 33 insertions(+), 8 deletions(-) diff --git a/fish/fish.c b/fish/fish.c index 830617b..987df59 100644 --- a/fish/fish.c +++ b/fish/fish.c @@ -750,8 +750,14 @@ issue_command (const char *cmd, char *argv[], const char *pipecmd) if (pipecmd) { int fd[2]; - fflush (stdout); - pipe (fd); + if (fflush (stdout) == EOF) { + perror ("failed to flush standard output"); + return -1; + } + if (pipe (fd) < 0) { + perror ("pipe failed"); + return -1; + } pid = fork (); if (pid == -1) { perror ("fork"); @@ -760,7 +766,10 @@ issue_command (const char *cmd, char *argv[], const char *pipecmd) if (pid == 0) { /* Child process. */ close (fd[1]); - dup2 (fd[0], 0); + if (dup2 (fd[0], 0) < 0) { + perror ("dup2 of stdin failed"); + _exit (1); + } r = system (pipecmd); if (r == -1) { @@ -770,9 +779,16 @@ issue_command (const char *cmd, char *argv[], const char *pipecmd) _exit (WEXITSTATUS (r)); } - stdout_saved_fd = dup (1); + if ((stdout_saved_fd = dup (1)) < 0) { + perror ("failed to dup stdout"); + return -1; + } close (fd[0]); - dup2 (fd[1], 1); + if (dup2 (fd[1], 1) < 0) { + perror ("failed to dup stdout"); + close (stdout_saved_fd); + return -1; + } close (fd[1]); } @@ -823,13 +839,22 @@ issue_command (const char *cmd, char *argv[], const char *pipecmd) /* Always flush stdout after every command, so that messages, results * etc appear immediately. */ - fflush (stdout); + if (fflush (stdout) == EOF) { + perror ("failed to flush standard output"); + return -1; + } if (pipecmd) { close (1); - dup2 (stdout_saved_fd, 1); + if (dup2 (stdout_saved_fd, 1) < 0) { + perror ("failed to dup2 standard output"); + r = -1; + } close (stdout_saved_fd); - waitpid (pid, NULL, 0); + if (waitpid (pid, NULL, 0) < 0) { + perror ("waiting for command to complete"); + r = -1; + } } return r; -- 1.6.4.378.g88f2f
On Wed, Aug 19, 2009 at 12:03:59PM +0200, Jim Meyering wrote: [...] OK, this one looks good. Rich. -- Richard Jones, Emerging Technologies, Red Hat http://et.redhat.com/~rjones virt-p2v converts physical machines to virtual machines. Boot with a live CD or over the network (PXE) and turn machines into Xen guests. http://et.redhat.com/~rjones/virt-p2v
participants (3)
-
Jim Meyering -
Matthew Booth -
Richard W.M. Jones