openfile handle to qemu-resume file

openfile handle to qemu-resume file

[web1]

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

Re: openfile handle to qemu-resume file

[Ian Jackson]

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.

[PATCH 1/2] libxl: Fix a couple of log messages to print correct errnos

[Ian Jackson]

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

[PATCH 2/2] libxl: Avoid fd leak of qemu state fd during migration

[Ian Jackson]

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

Re: [PATCH 2/2] libxl: Avoid fd leak of qemu state fd during migration

[Ian Jackson]

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.

Re: [PATCH 2/2] libxl: Avoid fd leak of qemu state fd during migration

[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.

Re: [PATCH 2/2] libxl: Avoid fd leak of qemu state fd during migration

[Ian Campbell]

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.

Re: [PATCH 1/2] libxl: Fix a couple of log messages to print correct errnos

[Don Slutz]

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>

Re: [PATCH 1/2] libxl: Fix a couple of log messages to print correct errnos

[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