Re: [PATCH] xl create: endless loop

[Christoph Egger <Christoph.Egger@amd.com>]

[-- Attachment #1: Type: text/plain, Size: 4044 bytes --]

On Wednesday 03 November 2010 17:12:47 Christoph Egger wrote:
> On Thursday 21 October 2010 16:11:55 Stefano Stabellini wrote:
> > On Tue, 19 Oct 2010, Christoph Egger wrote:
> > > On Monday 18 October 2010 14:50:35 Christoph Egger wrote:
> > > > Hi!
> > > >
> > > > I cannot start a guest with 'xl create' due to an endless loop in
> > > > libxl.c, function libxl__get_free_memory_slack():
> > > >
> > > > There is this code snippet:
> > > >
> > > > retry:
> > > >     free_mem_slack_s = libxl__xs_read(gc, XBT_NULL,
> > > > free_mem_slack_path); if (!free_mem_slack_s) {
> > > >         rc = libxl__fill_dom0_memory_info(gc, &target_memkb);
> > > >         if (rc < 0)
> > > >             return rc;
> > > >         goto retry;
> > > >     } else {
> > > >
> > > >
> > > > libxl__xs_read() returns 0 and libxl__fill_dom0_memory_info() also
> > > > returns 0. So there's a loop of retries.
> > >
> > > Attached patch fixes the endless loop.
> > >
> > > Signed-off-by: Christoph Egger <Christoph.Egger@amd.com>
> >
> > thanks for the patch but this cannot possibly be the right fix:
> >
> >
> > diff -r 9d8d6b93114e tools/libxl/libxl.c
> > --- a/tools/libxl/libxl.c	Mon Oct 18 15:24:39 2010 +0200
> > +++ b/tools/libxl/libxl.c	Tue Oct 19 10:29:00 2010 +0200
> > @@ -2836,7 +2836,7 @@ retry:
> >      free_mem_slack_s = libxl__xs_read(gc, XBT_NULL,
> > free_mem_slack_path); if (!free_mem_slack_s) {
> >          rc = libxl__fill_dom0_memory_info(gc, &target_memkb);
> > -        if (rc < 0)
> > +        if (rc <= 0)
> >              return rc;
> >          goto retry;
> >      } else {
> >
> > the idea is that libxl__fill_dom0_memory_info should fill the missing
> > informations in xenstore so that we can go ahead and try to read them
> > again and the second time should be successful.
> > Libxl__fill_dom0_memory_info returns 0 on success, so it is correct to
> > goto retry in that case.
> >
> > The bug must be in libxl__fill_dom0_memory_info that doesn't return
> > error when it should.
> > Does the appended patch works for you?
> >
> > ---
> >
> > diff -r 00b92112b055 tools/libxl/libxl.c
> > --- a/tools/libxl/libxl.c	Wed Oct 20 17:26:51 2010 +0100
> > +++ b/tools/libxl/libxl.c	Thu Oct 21 15:08:29 2010 +0100
> > @@ -2793,11 +2793,11 @@ retry_transaction:
> >
> >      rc = libxl_domain_info(ctx, &info, 0);
> >      if (rc < 0)
> > -        return rc;
> > +        goto out;
> >
> >      rc = libxl_get_physinfo(ctx, &physinfo);
> >      if (rc < 0)
> > -        return rc;
> > +        goto out;
> >
> >      libxl__xs_write(gc, t, target_path, "%"PRIu32,
> >              (uint32_t) info.current_memkb);
> > @@ -2816,9 +2816,12 @@ retry_transaction:
> >      rc = 0;
> >
> >  out:
> > -    if (!xs_transaction_end(ctx->xsh, t, 0))
> > +    if (!xs_transaction_end(ctx->xsh, t, 0)) {
> >          if (errno == EAGAIN)
> >              goto retry_transaction;
> > +        else
> > +            rc = ERROR_FAIL;
> > +    }
> >
> >
> >      return rc;
>
> No, this patch has no effect for me.
> In libxl__fill_dom0_memory_info(), the code path goes that way:
>
>     t = xs_transaction_start(ctx->xsh);
>
>     target = libxl__xs_read(gc, t, target_path);
>     if (target) {    <-- target contains "5"
>         *target_memkb = strtoul(target, &endptr, 10);
>         if (*endptr != '\0') {     <-- *endptr contains '\0'
>             LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR,
>                     "invalid memory target %s from %s\n", target,
> target_path);
>             rc = ERROR_FAIL;
>             goto out;
>         }
>         rc = 0;
>         goto out;    <-- take this jump with rc being 0
>     }
>

A slightly modified version works.

Signed-off-by: Mark Langsdorf <Mark.Langsdorf@amd.com>
Signed-off-by: Christoph Egger <Christoph.Egger@amd.com>




-- 
---to satisfy European Law for business letters:
Advanced Micro Devices GmbH
Einsteinring 24, 85609 Dornach b. Muenchen
Geschaeftsfuehrer: Alberto Bozzo, Andrew Bowd
Sitz: Dornach, Gemeinde Aschheim, Landkreis Muenchen
Registergericht Muenchen, HRB Nr. 43632

[-- Attachment #2: xen_xl.diff --]
[-- Type: text/x-diff, Size: 909 bytes --]

diff -r 8f66a16efee0 tools/libxl/libxl.c
--- a/tools/libxl/libxl.c	Tue Nov 02 16:08:56 2010 +0100
+++ b/tools/libxl/libxl.c	Thu Nov 04 16:30:27 2010 +0100
@@ -2787,17 +2787,15 @@ retry_transaction:
             rc = ERROR_FAIL;
             goto out;
         }
-        rc = 0;
-        goto out;
     }
 
     rc = libxl_domain_info(ctx, &info, 0);
     if (rc < 0)
-        return rc;
+        goto out;
 
     rc = libxl_get_physinfo(ctx, &physinfo);
     if (rc < 0)
-        return rc;
+        goto out;
 
     libxl__xs_write(gc, t, target_path, "%"PRIu32,
             (uint32_t) info.current_memkb);
@@ -2816,10 +2814,12 @@ retry_transaction:
     rc = 0;
 
 out:
-    if (!xs_transaction_end(ctx->xsh, t, 0))
+    if (!xs_transaction_end(ctx->xsh, t, 0)) {
         if (errno == EAGAIN)
             goto retry_transaction;
-
+        else
+            rc = ERROR_FAIL;
+    }
 
     return rc;
 }

[-- Attachment #3: Type: text/plain, Size: 138 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel