* openfile handle to qemu-resume file
@ 2014-10-09 16:05 web1
2014-10-15 16:16 ` Ian Jackson
0 siblings, 1 reply; 9+ messages in thread
From: web1 @ 2014-10-09 16:05 UTC (permalink / raw)
To: xen-devel lists.xen.org
Hello everybody,
i do some vm-restore tests with xen-4.4.1 , an i observe , that after i restore a vm via "xl restore", that their is an openfile handle to a file called "/var/lib/xen/qemu-resume.<vm-id>" that is marked as "deleted", the associated qemu-process show the same.
if i look in the source code, i found in libxl_dm.c in the function libxl__build_device_model_args_new that this "resume-file" is opened, but i found no call to close this file . Later in libxl__spawn_local_dm (which called libxl__build_device_model_args) i found a call to device_model_spawn_outcome, their in the state-file is deleted (if it exists).
i don´t really understand while the state-file is open and not close before delete. Can anyone that explain?
xen-4.4.1 was complied from sources without any special options
for one restore of a vm it is possilbe that this fact is not really importent, but if you use libvirt in combination with xen/libxl it cause to an increase of openfile handles , if you restore often vm´s for test´s
thanks and all the best
max
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: openfile handle to qemu-resume file
2014-10-09 16:05 openfile handle to qemu-resume file web1
@ 2014-10-15 16:16 ` Ian Jackson
2014-10-16 17:44 ` [PATCH 1/2] libxl: Fix a couple of log messages to print correct errnos Ian Jackson
0 siblings, 1 reply; 9+ messages in thread
From: Ian Jackson @ 2014-10-15 16:16 UTC (permalink / raw)
To: web1; +Cc: ian.campbell, xen-devel lists.xen.org
web1 writes ("[Xen-devel] openfile handle to qemu-resume file"):
...
> if i look in the source code, i found in libxl_dm.c in the function libxl__build_device_model_args_new that this "resume-file" is opened, but i found no call to close this file . Later in libxl__spawn_local_dm (which called libxl__build_device_model_args) i found a call to device_model_spawn_outcome, their in the state-file is deleted (if it exists).
Thanks. Yes, I think you are right.
> i don´t really understand while the state-file is open and not close before delete. Can anyone that explain?
It looks like a bug to me. I think the code in libxl here is quite
wrong. I am preparing a patch to fix it. (Sadly this is not entirely
trivial.)
> for one restore of a vm it is possilbe that this fact is not really importent, but if you use libvirt in combination with xen/libxl it cause to an increase of openfile handles , if you restore often vm´s for test´s
Indeed. Thanks for the report and the analysis.
Ian.
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH 1/2] libxl: Fix a couple of log messages to print correct errnos
2014-10-15 16:16 ` Ian Jackson
@ 2014-10-16 17:44 ` Ian Jackson
2014-10-16 17:44 ` [PATCH 2/2] libxl: Avoid fd leak of qemu state fd during migration Ian Jackson
` (2 more replies)
0 siblings, 3 replies; 9+ messages in thread
From: Ian Jackson @ 2014-10-16 17:44 UTC (permalink / raw)
To: xen-devel; +Cc: ian.campbell, Ian Jackson, Wei Liu
xc_domain_create and xc_cpupool_movedomain do not return errno values;
they return -1 and set errno. Fix the logging accordingly.
Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
---
tools/libxl/libxl_create.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
index 8b82584..8ae9701 100644
--- a/tools/libxl/libxl_create.c
+++ b/tools/libxl/libxl_create.c
@@ -515,14 +515,14 @@ int libxl__domain_make(libxl__gc *gc, libxl_domain_create_info *info,
ret = xc_domain_create(ctx->xch, info->ssidref, handle, flags, domid);
if (ret < 0) {
- LIBXL__LOG_ERRNOVAL(ctx, LIBXL__LOG_ERROR, ret, "domain creation fail");
+ LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "domain creation fail");
rc = ERROR_FAIL;
goto out;
}
ret = xc_cpupool_movedomain(ctx->xch, info->poolid, *domid);
if (ret < 0) {
- LIBXL__LOG_ERRNOVAL(ctx, LIBXL__LOG_ERROR, ret, "domain move fail");
+ LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "domain move fail");
rc = ERROR_FAIL;
goto out;
}
--
1.7.10.4
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH 2/2] libxl: Avoid fd leak of qemu state fd during migration
2014-10-16 17:44 ` [PATCH 1/2] libxl: Fix a couple of log messages to print correct errnos Ian Jackson
@ 2014-10-16 17:44 ` Ian Jackson
2014-10-16 17:45 ` Ian Jackson
2014-10-17 12:38 ` Wei Liu
2014-10-17 12:24 ` [PATCH 1/2] libxl: Fix a couple of log messages to print correct errnos Don Slutz
2014-10-17 12:26 ` Wei Liu
2 siblings, 2 replies; 9+ messages in thread
From: Ian Jackson @ 2014-10-16 17:44 UTC (permalink / raw)
To: xen-devel; +Cc: ian.campbell, Ian Jackson, Wei Liu
In a long-running process (such as virt-manager) this might eventually
run the process out of fds.
That qemu argument construction might generate an fd that needs to be
fed to qemu is a bit odd, but we just run with it and provide a
parameter to the qemu argument construction code for this purpose.
There is no need to use the carefd machinery, because leaking the odd
copy of this descriptor into a child unexpectedly forked out of
another thread, is fine. We just don't want to leak it back to the
main process.
Reported-by: ustermann.max@web.de
Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
---
tools/libxl/libxl_dm.c | 23 ++++++++++++++++-------
1 file changed, 16 insertions(+), 7 deletions(-)
diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
index d8992bb..b7c8a99 100644
--- a/tools/libxl/libxl_dm.c
+++ b/tools/libxl/libxl_dm.c
@@ -403,7 +403,8 @@ static char *dm_spice_options(libxl__gc *gc,
static char ** libxl__build_device_model_args_new(libxl__gc *gc,
const char *dm, int guest_domid,
const libxl_domain_config *guest_config,
- const libxl__domain_build_state *state)
+ const libxl__domain_build_state *state,
+ int *dm_state_fd)
{
libxl_ctx *ctx = libxl__gc_owner(gc);
const libxl_domain_create_info *c_info = &guest_config->c_info;
@@ -677,9 +678,9 @@ static char ** libxl__build_device_model_args_new(libxl__gc *gc,
if (state->saved_state) {
/* This file descriptor is meant to be used by QEMU */
- int migration_fd = open(state->saved_state, O_RDONLY);
+ *dm_state_fd = open(state->saved_state, O_RDONLY);
flexarray_append(dm_args, "-incoming");
- flexarray_append(dm_args, libxl__sprintf(gc, "fd:%d", migration_fd));
+ flexarray_append(dm_args, GCSPRINTF("fd:%d",*dm_state_fd));
}
for (i = 0; b_info->extra && b_info->extra[i] != NULL; i++)
flexarray_append(dm_args, b_info->extra[i]);
@@ -792,7 +793,10 @@ static char ** libxl__build_device_model_args_new(libxl__gc *gc,
static char ** libxl__build_device_model_args(libxl__gc *gc,
const char *dm, int guest_domid,
const libxl_domain_config *guest_config,
- const libxl__domain_build_state *state)
+ const libxl__domain_build_state *state,
+ int *dm_state_fd)
+/* dm_state_fd may be NULL iff caller knows we are using old stubdom
+ * and therefore will be passing a filename rather than a fd. */
{
libxl_ctx *ctx = libxl__gc_owner(gc);
@@ -802,9 +806,11 @@ static char ** libxl__build_device_model_args(libxl__gc *gc,
guest_domid, guest_config,
state);
case LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN:
+ assert(dm_state_fd != NULL);
+ assert(*dm_state_fd < 0);
return libxl__build_device_model_args_new(gc, dm,
guest_domid, guest_config,
- state);
+ state, dm_state_fd);
default:
LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "unknown device model version %d",
guest_config->b_info.device_model_version);
@@ -1016,7 +1022,7 @@ void libxl__spawn_stub_dm(libxl__egc *egc, libxl__stub_dm_spawn_state *sdss)
goto out;
args = libxl__build_device_model_args(gc, "stubdom-dm", guest_domid,
- guest_config, d_state);
+ guest_config, d_state, NULL);
if (!args) {
ret = ERROR_FAIL;
goto out;
@@ -1268,6 +1274,7 @@ void libxl__spawn_local_dm(libxl__egc *egc, libxl__dm_spawn_state *dmss)
char *vm_path;
char **pass_stuff;
const char *dm;
+ int dm_state_fd = -1;
if (libxl_defbool_val(b_info->device_model_stubdomain)) {
abort();
@@ -1284,7 +1291,8 @@ void libxl__spawn_local_dm(libxl__egc *egc, libxl__dm_spawn_state *dmss)
rc = ERROR_FAIL;
goto out;
}
- args = libxl__build_device_model_args(gc, dm, domid, guest_config, state);
+ args = libxl__build_device_model_args(gc, dm, domid, guest_config, state,
+ &dm_state_fd);
if (!args) {
rc = ERROR_FAIL;
goto out;
@@ -1377,6 +1385,7 @@ out_close:
if (null >= 0) close(null);
if (logfile_w >= 0) close(logfile_w);
out:
+ if (dm_state_fd >= 0) close(dm_state_fd);
if (rc)
device_model_spawn_outcome(egc, dmss, rc);
}
--
1.7.10.4
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2] libxl: Avoid fd leak of qemu state fd during migration
2014-10-16 17:44 ` [PATCH 2/2] libxl: Avoid fd leak of qemu state fd during migration Ian Jackson
@ 2014-10-16 17:45 ` Ian Jackson
2014-10-17 12:38 ` Wei Liu
1 sibling, 0 replies; 9+ messages in thread
From: Ian Jackson @ 2014-10-16 17:45 UTC (permalink / raw)
To: web1; +Cc: ian.campbell, xen-devel, Wei Liu
Ian Jackson writes ("[PATCH 2/2] libxl: Avoid fd leak of qemu state fd during migration"):
> In a long-running process (such as virt-manager) this might eventually
> run the process out of fds.
>
> That qemu argument construction might generate an fd that needs to be
> fed to qemu is a bit odd, but we just run with it and provide a
> parameter to the qemu argument construction code for this purpose.
>
> There is no need to use the carefd machinery, because leaking the odd
> copy of this descriptor into a child unexpectedly forked out of
> another thread, is fine. We just don't want to leak it back to the
> main process.
It seems I didn't CC this to you, which I should have, sorry. Will
send the patch in a separate email.
Ian.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/2] libxl: Fix a couple of log messages to print correct errnos
2014-10-16 17:44 ` [PATCH 1/2] libxl: Fix a couple of log messages to print correct errnos Ian Jackson
2014-10-16 17:44 ` [PATCH 2/2] libxl: Avoid fd leak of qemu state fd during migration Ian Jackson
@ 2014-10-17 12:24 ` Don Slutz
2014-10-17 12:26 ` Wei Liu
2 siblings, 0 replies; 9+ messages in thread
From: Don Slutz @ 2014-10-17 12:24 UTC (permalink / raw)
To: Ian Jackson, xen-devel; +Cc: ian.campbell, Wei Liu
On 10/16/14 13:44, Ian Jackson wrote:
> xc_domain_create and xc_cpupool_movedomain do not return errno values;
> they return -1 and set errno. Fix the logging accordingly.
>
> Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
> ---
> tools/libxl/libxl_create.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
> index 8b82584..8ae9701 100644
> --- a/tools/libxl/libxl_create.c
> +++ b/tools/libxl/libxl_create.c
> @@ -515,14 +515,14 @@ int libxl__domain_make(libxl__gc *gc, libxl_domain_create_info *info,
>
> ret = xc_domain_create(ctx->xch, info->ssidref, handle, flags, domid);
> if (ret < 0) {
> - LIBXL__LOG_ERRNOVAL(ctx, LIBXL__LOG_ERROR, ret, "domain creation fail");
> + LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "domain creation fail");
> rc = ERROR_FAIL;
> goto out;
> }
>
> ret = xc_cpupool_movedomain(ctx->xch, info->poolid, *domid);
> if (ret < 0) {
> - LIBXL__LOG_ERRNOVAL(ctx, LIBXL__LOG_ERROR, ret, "domain move fail");
> + LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "domain move fail");
> rc = ERROR_FAIL;
> goto out;
> }
Looks good.
Reviewed-by: Don Slutz <dslutz@verizon.com>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/2] libxl: Fix a couple of log messages to print correct errnos
2014-10-16 17:44 ` [PATCH 1/2] libxl: Fix a couple of log messages to print correct errnos Ian Jackson
2014-10-16 17:44 ` [PATCH 2/2] libxl: Avoid fd leak of qemu state fd during migration Ian Jackson
2014-10-17 12:24 ` [PATCH 1/2] libxl: Fix a couple of log messages to print correct errnos Don Slutz
@ 2014-10-17 12:26 ` Wei Liu
2 siblings, 0 replies; 9+ messages in thread
From: Wei Liu @ 2014-10-17 12:26 UTC (permalink / raw)
To: Ian Jackson; +Cc: ian.campbell, xen-devel, Wei Liu
On Thu, Oct 16, 2014 at 06:44:12PM +0100, Ian Jackson wrote:
> xc_domain_create and xc_cpupool_movedomain do not return errno values;
> they return -1 and set errno. Fix the logging accordingly.
>
> Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
Acked-by: Wei Liu <wei.liu2@citrix.com>
> ---
> tools/libxl/libxl_create.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
> index 8b82584..8ae9701 100644
> --- a/tools/libxl/libxl_create.c
> +++ b/tools/libxl/libxl_create.c
> @@ -515,14 +515,14 @@ int libxl__domain_make(libxl__gc *gc, libxl_domain_create_info *info,
>
> ret = xc_domain_create(ctx->xch, info->ssidref, handle, flags, domid);
> if (ret < 0) {
> - LIBXL__LOG_ERRNOVAL(ctx, LIBXL__LOG_ERROR, ret, "domain creation fail");
> + LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "domain creation fail");
> rc = ERROR_FAIL;
> goto out;
> }
>
> ret = xc_cpupool_movedomain(ctx->xch, info->poolid, *domid);
> if (ret < 0) {
> - LIBXL__LOG_ERRNOVAL(ctx, LIBXL__LOG_ERROR, ret, "domain move fail");
> + LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "domain move fail");
> rc = ERROR_FAIL;
> goto out;
> }
> --
> 1.7.10.4
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2] libxl: Avoid fd leak of qemu state fd during migration
2014-10-16 17:44 ` [PATCH 2/2] libxl: Avoid fd leak of qemu state fd during migration Ian Jackson
2014-10-16 17:45 ` Ian Jackson
@ 2014-10-17 12:38 ` Wei Liu
2014-10-20 13:21 ` Ian Campbell
1 sibling, 1 reply; 9+ messages in thread
From: Wei Liu @ 2014-10-17 12:38 UTC (permalink / raw)
To: Ian Jackson; +Cc: ian.campbell, xen-devel, Wei Liu
On Thu, Oct 16, 2014 at 06:44:13PM +0100, Ian Jackson wrote:
> In a long-running process (such as virt-manager) this might eventually
> run the process out of fds.
>
> That qemu argument construction might generate an fd that needs to be
> fed to qemu is a bit odd, but we just run with it and provide a
> parameter to the qemu argument construction code for this purpose.
>
> There is no need to use the carefd machinery, because leaking the odd
> copy of this descriptor into a child unexpectedly forked out of
> another thread, is fine. We just don't want to leak it back to the
> main process.
>
> Reported-by: ustermann.max@web.de
> Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
The reasoning is sensible and the code looks correct.
Reviewed-by: Wei Liu <wei.liu2@citrix.com>
Wei.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2] libxl: Avoid fd leak of qemu state fd during migration
2014-10-17 12:38 ` Wei Liu
@ 2014-10-20 13:21 ` Ian Campbell
0 siblings, 0 replies; 9+ messages in thread
From: Ian Campbell @ 2014-10-20 13:21 UTC (permalink / raw)
To: Wei Liu; +Cc: xen-devel, Ian Jackson
On Fri, 2014-10-17 at 13:38 +0100, Wei Liu wrote:
> On Thu, Oct 16, 2014 at 06:44:13PM +0100, Ian Jackson wrote:
> > In a long-running process (such as virt-manager) this might eventually
> > run the process out of fds.
> >
> > That qemu argument construction might generate an fd that needs to be
> > fed to qemu is a bit odd, but we just run with it and provide a
> > parameter to the qemu argument construction code for this purpose.
> >
> > There is no need to use the carefd machinery, because leaking the odd
> > copy of this descriptor into a child unexpectedly forked out of
> > another thread, is fine. We just don't want to leak it back to the
> > main process.
> >
> > Reported-by: ustermann.max@web.de
> > Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
>
> The reasoning is sensible and the code looks correct.
>
> Reviewed-by: Wei Liu <wei.liu2@citrix.com>
Also acked. This is a pretty clear bug fix and therefore I have applied
both without waiting for a release exception.
Ian.
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2014-10-20 13:21 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-10-09 16:05 openfile handle to qemu-resume file web1
2014-10-15 16:16 ` Ian Jackson
2014-10-16 17:44 ` [PATCH 1/2] libxl: Fix a couple of log messages to print correct errnos Ian Jackson
2014-10-16 17:44 ` [PATCH 2/2] libxl: Avoid fd leak of qemu state fd during migration Ian Jackson
2014-10-16 17:45 ` Ian Jackson
2014-10-17 12:38 ` Wei Liu
2014-10-20 13:21 ` Ian Campbell
2014-10-17 12:24 ` [PATCH 1/2] libxl: Fix a couple of log messages to print correct errnos Don Slutz
2014-10-17 12:26 ` Wei Liu
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.