Re: [PATCH] xl create: endless loop

[Stefano Stabellini <stefano.stabellini@eu.citrix.com>]

On Wed, 10 Nov 2010, Christoph Egger wrote:
> On Tuesday 09 November 2010 19:08:14 Stefano Stabellini wrote:
> > On Wed, 3 Nov 2010, Christoph Egger wrote:
> > > 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
> > >     }
> >
> > The problem you are having is that somebody in your system is setting a
> > target for dom0 without setting freemem-slack. Are you still running
> > xend at boot?
> 
> Yes, I do.

running xend alongside xl is not recommended, it could cause bugs,
especially if you don't disable autoballooning


> Xen is booted with dom0_mem.
> Dom0 has autoballooning disabled in the kernel.
> 

You still need to disable autoballooning in xl, setting autoballooning
to 0 in /etc/xen/xl.conf


> > Currently libxl__fill_dom0_memory_info assumes that both values are set
> > initially at the same time (by libxl__fill_dom0_memory_info).
> > This patch should fix the issue, I would appreciate if you could test
> > it.
> >
> > ---
> >
> >
> > libxl: do not assume target and freemem-slack are written at the same time
> >
> > Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> >
> > diff -r 7188d1e4b0e1 tools/libxl/libxl.c
> > --- a/tools/libxl/libxl.c	Tue Nov 09 12:00:05 2010 +0000
> > +++ b/tools/libxl/libxl.c	Tue Nov 09 18:05:52 2010 +0000
> > @@ -2779,18 +2779,25 @@ static int libxl__fill_dom0_memory_info(
> >      int rc;
> >      libxl_dominfo info;
> >      libxl_physinfo physinfo;
> > -    char *target = NULL, *endptr = NULL;
> > +    char *target = NULL, *staticmax = NULL, *freememslack = NULL, *endptr
> > = NULL; char *target_path = "/local/domain/0/memory/target";
> >      char *max_path = "/local/domain/0/memory/static-max";
> >      char *free_mem_slack_path = "/local/domain/0/memory/freemem-slack";
> >      xs_transaction_t t;
> >      libxl_ctx *ctx = libxl__gc_owner(gc);
> > -    uint32_t free_mem_slack = 0;
> > +    uint32_t free_mem_slack_kb = 0;
> >
> >  retry_transaction:
> >      t = xs_transaction_start(ctx->xsh);
> >
> >      target = libxl__xs_read(gc, t, target_path);
> > +    staticmax = libxl__xs_read(gc, t, target_path);
> > +    freememslack = libxl__xs_read(gc, t, target_path);
> > +    if (target && staticmax && freememslack) {
> > +        rc = 0;
> > +        goto out;
> > +    }
> 
> *target, *staticmax and *freememslack contain the value "5".
> So with this patch, rc always returns 0 from there.
> 

that is correct: the values are there, so there is no need to write them
to xenstore again, and the caller should be able to read the target
correctly