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?