5:42 a.m.
On principle, we shouldn't ignore write failure:
From 56317a61bc22e935dc750cf669a164bacc12cf12 Mon Sep 17 00:00:00 2001
From: Jim Meyering <meyering(a)redhat.com>
Date: Thu, 20 Aug 2009 12:29:46 +0200
Subject: [PATCH libguestfs] daemon: diagnose socket write failure
* daemon/proto.c (send_chunk): Don't ignore socket-write error.
---
daemon/proto.c | 8 +++++---
1 files changed, 5 insertions(+), 3 deletions(-)
diff --git a/daemon/proto.c b/daemon/proto.c
index 9b33902..c2dd50c 100644
--- a/daemon/proto.c
+++ b/daemon/proto.c
@@ -495,8 +495,10 @@ send_chunk (const guestfs_chunk *chunk)
xdr_uint32_t (&xdr, &len);
xdr_destroy (&xdr);
- (void) xwrite (sock, lenbuf, 4);
- (void) xwrite (sock, buf, len);
+ int err = (xwrite (sock, lenbuf, 4) == 0
+ && xwrite (sock, buf, len) == 0 ? 0 : -1);
+ if (err)
+ fprintf (stderr, "send_chunk: write failed\n");
- return 0;
+ return err;
}
--
1.6.4.378.g88f2f
5:49 a.m.
On Thu, Aug 20, 2009 at 12:42:47PM +0200, Jim Meyering wrote:
On principle, we shouldn't ignore write failure:
[...]
- (void) xwrite (sock, lenbuf, 4);
- (void) xwrite (sock, buf, len);
What's happening in the original code is that if the daemon can't
write its reply message / reply chunk back over the guestfwd socket to
the host (library), then it just ignores the error.
The code at the moment is wrong - I can't think of any reason why we
should ignore this error.
If the daemon cannot contact the host, then things have gone badly
wrong, so the daemon should exit, which causes an appliance kernel
panic, which causes qemu to exit, and the library notices this and
informs the caller through either an error result or a callback.
So, the code below is better because it checks the return value, but:
+ int err = (xwrite (sock, lenbuf, 4) == 0
+ && xwrite (sock, buf, len) == 0 ? 0 : -1);
+ if (err)
+ fprintf (stderr, "send_chunk: write failed\n");
I think in this case it should exit(1).
If you look at the earlier calls to xwrite in daemon/proto.c:reply()
you'll see that those exit. It's the same situation here.
Rich.
--
Richard Jones, Emerging Technologies, Red Hat http://et.redhat.com/~rjones
New in Fedora 11: Fedora Windows cross-compiler. Compile Windows
programs, test, and build Windows installers. Over 70 libraries supprt'd
http://fedoraproject.org/wiki/MinGW http://www.annexia.org/fedora_mingw
5:59 a.m.
Richard W.M. Jones wrote:
On Thu, Aug 20, 2009 at 12:42:47PM +0200, Jim Meyering wrote:
> On principle, we shouldn't ignore write failure:
[...]
> - (void) xwrite (sock, lenbuf, 4);
> - (void) xwrite (sock, buf, len);
What's happening in the original code is that if the daemon can't
write its reply message / reply chunk back over the guestfwd socket to
the host (library), then it just ignores the error.
The code at the moment is wrong - I can't think of any reason why we
should ignore this error.
If the daemon cannot contact the host, then things have gone badly
wrong, so the daemon should exit, which causes an appliance kernel
panic, which causes qemu to exit, and the library notices this and
informs the caller through either an error result or a callback.
So, the code below is better because it checks the return value, but:
> + int err = (xwrite (sock, lenbuf, 4) == 0
> + && xwrite (sock, buf, len) == 0 ? 0 : -1);
> + if (err)
> + fprintf (stderr, "send_chunk: write failed\n");
I think in this case it should exit(1).
If you look at the earlier calls to xwrite in daemon/proto.c:reply()
you'll see that those exit. It's the same situation here.
Ok, then by that argument, the preceding code that checks for encoding
failure should also exit:
static int
send_chunk (const guestfs_chunk *chunk)
{
char buf[GUESTFS_MAX_CHUNK_SIZE + 48];
char lenbuf[4];
XDR xdr;
uint32_t len;
xdrmem_create (&xdr, buf, sizeof buf, XDR_ENCODE);
if (!xdr_guestfs_chunk (&xdr, (guestfs_chunk *) chunk)) {
fprintf (stderr, "send_chunk: failed to encode chunk\n");
xdr_destroy (&xdr);
return -1;
}
However, if we do that, we might as well make the function void,
since it will never return an error status.
But then, look at its two callers.
send_file_write currently tests for send_chunk failure
send_file_end does not
Easy to adapt. Just remove the test in send_file_end.
Ok with all that?
6:32 a.m.
On Thu, Aug 20, 2009 at 12:59:54PM +0200, Jim Meyering wrote:
Ok, then by that argument, the preceding code that checks for
encoding
failure should also exit:
static int
send_chunk (const guestfs_chunk *chunk)
{
char buf[GUESTFS_MAX_CHUNK_SIZE + 48];
char lenbuf[4];
XDR xdr;
uint32_t len;
xdrmem_create (&xdr, buf, sizeof buf, XDR_ENCODE);
if (!xdr_guestfs_chunk (&xdr, (guestfs_chunk *) chunk)) {
fprintf (stderr, "send_chunk: failed to encode chunk\n");
xdr_destroy (&xdr);
return -1;
}
However, if we do that, we might as well make the function void,
since it will never return an error status.
Encoding failures can be non-fatal (arguably not in this case). One
case where they can legitimately fail is if the data to be encoded in
the chunk exceeds the size of the fixed buffer (buf).
But then, look at its two callers.
send_file_write currently tests for send_chunk failure
send_file_end does not
Easy to adapt. Just remove the test in send_file_end.
Ok with all that?
Both should check the return value. Whether they can do anything
useful about it is questionable though.
Rich.
--
Richard Jones, Emerging Technologies, Red Hat http://et.redhat.com/~rjones
New in Fedora 11: Fedora Windows cross-compiler. Compile Windows
programs, test, and build Windows installers. Over 70 libraries supprt'd
http://fedoraproject.org/wiki/MinGW http://www.annexia.org/fedora_mingw
6:59 a.m.
Richard W.M. Jones wrote:
...
> Ok with all that?
Both should check the return value. Whether they can do anything
useful about it is questionable though.
Ok. Per discussion on IRC, here's an update:
From f3542cad8d9a092099967a3983e922003a57c211 Mon Sep 17 00:00:00 2001
From: Jim Meyering <meyering(a)redhat.com>
Date: Thu, 20 Aug 2009 12:29:46 +0200
Subject: [PATCH libguestfs] daemon: diagnose socket write failure
* daemon/proto.c (send_chunk): Don't ignore socket-write error.
* daemon/proto.c (send_file_end): Return "int", not void,
so we can propagate send_chunk failure to caller.
* daemon/daemon.h (send_file_end): Update prototype.
* daemon/tar.c (do_tar_out, do_tgz_out): Update uses of send_file_end.
* daemon/upload.c (do_download): Likewise.
---
daemon/daemon.h | 2 +-
daemon/proto.c | 14 +++++++++-----
daemon/tar.c | 8 ++++++--
daemon/upload.c | 4 +++-
4 files changed, 19 insertions(+), 9 deletions(-)
diff --git a/daemon/daemon.h b/daemon/daemon.h
index 0ce56e1..83c9672 100644
--- a/daemon/daemon.h
+++ b/daemon/daemon.h
@@ -116,7 +116,7 @@ extern void cancel_receive (void);
* Note max write size if GUESTFS_MAX_CHUNK_SIZE.
*/
extern int send_file_write (const void *buf, int len);
-extern void send_file_end (int cancel);
+extern int send_file_end (int cancel);
/* only call this if there is a FileOut parameter */
extern void reply (xdrproc_t xdrp, char *ret);
diff --git a/daemon/proto.c b/daemon/proto.c
index 9b33902..d935ded 100644
--- a/daemon/proto.c
+++ b/daemon/proto.c
@@ -462,7 +462,7 @@ check_for_library_cancellation (void)
return 1;
}
-void
+int
send_file_end (int cancel)
{
guestfs_chunk chunk;
@@ -470,7 +470,7 @@ send_file_end (int cancel)
chunk.cancel = cancel;
chunk.data.data_len = 0;
chunk.data.data_val = NULL;
- send_chunk (&chunk);
+ return send_chunk (&chunk);
}
static int
@@ -495,8 +495,12 @@ send_chunk (const guestfs_chunk *chunk)
xdr_uint32_t (&xdr, &len);
xdr_destroy (&xdr);
- (void) xwrite (sock, lenbuf, 4);
- (void) xwrite (sock, buf, len);
+ int err = (xwrite (sock, lenbuf, 4) == 0
+ && xwrite (sock, buf, len) == 0 ? 0 : -1);
+ if (err) {
+ fprintf (stderr, "send_chunk: write failed\n");
+ exit (1);
+ }
- return 0;
+ return err;
}
diff --git a/daemon/tar.c b/daemon/tar.c
index 03dc512..c3bdcf7 100644
--- a/daemon/tar.c
+++ b/daemon/tar.c
@@ -149,7 +149,9 @@ do_tar_out (const char *dir)
return -1;
}
- send_file_end (0); /* Normal end of file. */
+ if (send_file_end (0)) /* Normal end of file. */
+ return -1;
+
return 0;
}
@@ -268,6 +270,8 @@ do_tgz_out (const char *dir)
return -1;
}
- send_file_end (0); /* Normal end of file. */
+ if (send_file_end (0)) /* Normal end of file. */
+ return -1;
+
return 0;
}
diff --git a/daemon/upload.c b/daemon/upload.c
index 143fa82..da86bd6 100644
--- a/daemon/upload.c
+++ b/daemon/upload.c
@@ -129,6 +129,8 @@ do_download (const char *filename)
return -1;
}
- send_file_end (0); /* Normal end of file. */
+ if (send_file_end (0)) /* Normal end of file. */
+ return -1;
+
return 0;
}
--
1.6.4.378.g88f2f
8:03 a.m.
On Thu, Aug 20, 2009 at 01:59:09PM +0200, Jim Meyering wrote:
Richard W.M. Jones wrote:
...
>> Ok with all that?
>
> Both should check the return value. Whether they can do anything
> useful about it is questionable though.
Ok. Per discussion on IRC, here's an update:
Yes this looks good, ACK.
Rich.
--
Richard Jones, Emerging Technologies, Red Hat http://et.redhat.com/~rjones
Read my programming blog: http://rwmj.wordpress.com
Fedora now supports 75 OCaml packages (the OPEN alternative to F#)
http://cocan.org/getting_started_with_ocaml_on_red_hat_and_fedora
9 a.m.
Richard W.M. Jones wrote:
On Thu, Aug 20, 2009 at 01:59:09PM +0200, Jim Meyering wrote:
> Richard W.M. Jones wrote:
> ...
> >> Ok with all that?
> >
> > Both should check the return value. Whether they can do anything
> > useful about it is questionable though.
>
> Ok. Per discussion on IRC, here's an update:
Yes this looks good, ACK.
pushed
5497
days inactive
5497
days old
6 comments
2 participants
participants (2)
-
Jim Meyering -
Richard W.M. Jones