Ian Campbell
2011-Dec-13 17:09 UTC
[PATCH] libxl: improve error handling when saving device model state
# HG changeset patch
# User Ian Campbell <ian.campbell@citrix.com>
# Date 1323795995 0
# Node ID 64e48ae2ef6760db221fe1896f7571260b0fd6ba
# Parent 738c34b1b0450724c4b91b739d1e9cae09f26035
libxl: improve error handling when saving device model state.
Do not leak a file descriptor (fd2 when used with upstream qemu) or a file (the
save file which is leaked on failure).
Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
diff -r 738c34b1b045 -r 64e48ae2ef67 tools/libxl/libxl_dom.c
--- a/tools/libxl/libxl_dom.c Tue Dec 13 17:06:35 2011 +0000
+++ b/tools/libxl/libxl_dom.c Tue Dec 13 17:06:35 2011 +0000
@@ -605,7 +605,7 @@ out:
int libxl__domain_save_device_model(libxl__gc *gc, uint32_t domid, int fd)
{
libxl_ctx *ctx = libxl__gc_owner(gc);
- int fd2, c;
+ int ret, fd2, c;
char buf[1024];
const char *filename = libxl__device_model_savefile(gc, domid);
struct stat st;
@@ -630,8 +630,10 @@ int libxl__domain_save_device_model(libx
return ERROR_FAIL;
}
/* Save DM state into fd2 */
- if (libxl__qmp_migrate(gc, domid, fd2))
- return ERROR_FAIL;
+ ret = libxl__qmp_migrate(gc, domid, fd2);
+ close(fd2);
+ if (ret)
+ goto out_unlink;
break;
default:
return ERROR_INVAL;
@@ -646,31 +648,35 @@ int libxl__domain_save_device_model(libx
qemu_state_len = st.st_size;
LIBXL__LOG(ctx, LIBXL__LOG_DEBUG, "Qemu state is %d bytes\n",
qemu_state_len);
- c = libxl_write_exactly(ctx, fd, QEMU_SIGNATURE, strlen(QEMU_SIGNATURE),
- "saved-state file", "qemu
signature");
- if (c)
- return c;
+ ret = libxl_write_exactly(ctx, fd, QEMU_SIGNATURE, strlen(QEMU_SIGNATURE),
+ "saved-state file", "qemu
signature");
+ if (ret)
+ goto out_unlink;
- c = libxl_write_exactly(ctx, fd, &qemu_state_len,
sizeof(qemu_state_len),
+ ret = libxl_write_exactly(ctx, fd, &qemu_state_len,
sizeof(qemu_state_len),
"saved-state file", "saved-state
length");
- if (c)
- return c;
+ if (ret)
+ goto out_unlink;
fd2 = open(filename, O_RDONLY);
while ((c = read(fd2, buf, sizeof(buf))) != 0) {
if (c < 0) {
if (errno == EINTR)
continue;
- return errno;
+ ret = errno;
+ goto out_close_fd2;
}
- c = libxl_write_exactly(
+ ret = libxl_write_exactly(
ctx, fd, buf, c, "saved-state file", "qemu
state");
- if (c)
- return c;
+ if (ret)
+ goto out_close_fd2;
}
+ ret = 0;
+out_close_fd2:
close(fd2);
+out_unlink:
unlink(filename);
- return 0;
+ return ret;
}
char *libxl__uuid2string(libxl__gc *gc, const libxl_uuid uuid)
Ian Jackson
2011-Dec-15 17:00 UTC
Re: [PATCH] libxl: improve error handling when saving device model state
Ian Campbell writes ("[Xen-devel] [PATCH] libxl: improve error handling
when saving device model state"):> libxl: improve error handling when saving device model state.
...> +out_close_fd2:
> close(fd2);
> +out_unlink:
> unlink(filename);
This style of error handling is very prone to errors.
How about:
int fd2 = -1;
blah blah maybe goto out blah blah
if (fd2 >= 0) close(fd2);
And always unlinking the filename is fine, surely ?
Ian.
Ian Campbell
2011-Dec-16 10:37 UTC
Re: [PATCH] libxl: improve error handling when saving device model state
On Thu, 2011-12-15 at 17:00 +0000, Ian Jackson wrote:> Ian Campbell writes ("[Xen-devel] [PATCH] libxl: improve error handling when saving device model state"): > > libxl: improve error handling when saving device model state. > ... > > +out_close_fd2: > > close(fd2); > > +out_unlink: > > unlink(filename); > > This style of error handling is very prone to errors. > > How about: > > int fd2 = -1; > > blah blah maybe goto out blah blah > > if (fd2 >= 0) close(fd2); > > And always unlinking the filename is fine, surely ?Yes. Patch is below. In principal it ought to be possible to save upstream qemu state direct to the fd and avoid the file altogether, Anthony what do you think? Ian. diff -r 5924514e0523 tools/libxl/libxl_dom.c --- a/tools/libxl/libxl_dom.c Fri Dec 16 09:48:06 2011 +0000 +++ b/tools/libxl/libxl_dom.c Fri Dec 16 10:36:34 2011 +0000 @@ -605,7 +605,7 @@ out: int libxl__domain_save_device_model(libxl__gc *gc, uint32_t domid, int fd) { libxl_ctx *ctx = libxl__gc_owner(gc); - int fd2, c; + int ret, fd2 = -1, c; char buf[1024]; const char *filename = libxl__device_model_savefile(gc, domid); struct stat st; @@ -630,8 +630,11 @@ int libxl__domain_save_device_model(libx return ERROR_FAIL; } /* Save DM state into fd2 */ - if (libxl__qmp_migrate(gc, domid, fd2)) - return ERROR_FAIL; + ret = libxl__qmp_migrate(gc, domid, fd2); + if (ret) + goto out; + close(fd2); + fd2 = -1; break; default: return ERROR_INVAL; @@ -640,37 +643,45 @@ int libxl__domain_save_device_model(libx if (stat(filename, &st) < 0) { LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "Unable to stat qemu save file\n"); - return ERROR_FAIL; + ret = ERROR_FAIL; + goto out; } qemu_state_len = st.st_size; LIBXL__LOG(ctx, LIBXL__LOG_DEBUG, "Qemu state is %d bytes\n", qemu_state_len); - c = libxl_write_exactly(ctx, fd, QEMU_SIGNATURE, strlen(QEMU_SIGNATURE), - "saved-state file", "qemu signature"); - if (c) - return c; + ret = libxl_write_exactly(ctx, fd, QEMU_SIGNATURE, strlen(QEMU_SIGNATURE), + "saved-state file", "qemu signature"); + if (ret) + goto out; - c = libxl_write_exactly(ctx, fd, &qemu_state_len, sizeof(qemu_state_len), + ret = libxl_write_exactly(ctx, fd, &qemu_state_len, sizeof(qemu_state_len), "saved-state file", "saved-state length"); - if (c) - return c; + if (ret) + goto out; fd2 = open(filename, O_RDONLY); + if (fd2 < 0) { + LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "Unable to open qemu save file\n"); + goto out; + } while ((c = read(fd2, buf, sizeof(buf))) != 0) { if (c < 0) { if (errno == EINTR) continue; - return errno; + ret = errno; + goto out; } - c = libxl_write_exactly( + ret = libxl_write_exactly( ctx, fd, buf, c, "saved-state file", "qemu state"); - if (c) - return c; + if (ret) + goto out; } - close(fd2); + ret = 0; +out: + if (fd2 >= 0) close(fd2); unlink(filename); - return 0; + return ret; } char *libxl__uuid2string(libxl__gc *gc, const libxl_uuid uuid)
Anthony PERARD
2011-Dec-20 15:54 UTC
Re: [PATCH] libxl: improve error handling when saving device model state
On Fri, 16 Dec 2011, Ian Campbell wrote:> On Thu, 2011-12-15 at 17:00 +0000, Ian Jackson wrote: > > Ian Campbell writes ("[Xen-devel] [PATCH] libxl: improve error handling when saving device model state"): > > > libxl: improve error handling when saving device model state. > > ... > > > +out_close_fd2: > > > close(fd2); > > > +out_unlink: > > > unlink(filename); > > > > This style of error handling is very prone to errors. > > > > How about: > > > > int fd2 = -1; > > > > blah blah maybe goto out blah blah > > > > if (fd2 >= 0) close(fd2); > > > > And always unlinking the filename is fine, surely ? > > Yes. Patch is below. > > In principal it ought to be possible to save upstream qemu state direct > to the fd and avoid the file altogether, Anthony what do you think?Upstream QEMU only use the fd, so only xl now about the filename. I''ve just create the file because it was more convenient to use in libxl than changing more code. So yes, we can avoid the file. And the patch looks fine to me. Regards, -- Anthony PERARD
Ian Jackson
2011-Dec-20 18:15 UTC
Re: [PATCH] libxl: improve error handling when saving device model state [and 1 more messages]
Ian Campbell writes ("Re: [Xen-devel] [PATCH] libxl: improve error handling
when saving device model state"):> On Thu, 2011-12-15 at 17:00 +0000, Ian Jackson wrote:
> > Ian Campbell writes ("[Xen-devel] [PATCH] libxl: improve error
handling when saving device model state"):
> > And always unlinking the filename is fine, surely ?
>
> Yes. Patch is below.
>
> In principal it ought to be possible to save upstream qemu state direct
> to the fd and avoid the file altogether, Anthony what do you think?
Applied, thanks to both of you.
Ian.