Re: [Xen-devel] [PATCH 20/20] libxl: ao: Convert libxl_run_bootloader

[Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx>]

Ian Campbell writes ("Re: [Xen-devel] [PATCH 20/20] libxl: ao: Convert 
libxl_run_bootloader"):
> On Fri, 2012-03-16 at 16:26 +0000, Ian Jackson wrote:
> > +int libxl__openptys(libxl__gc *gc, libxl__openpty_state *op,
> > +                    const struct termios *termp,
> > +                    const struct winsize *winp) {
> 
> "{" on next line please, (here and elsewhere)

Oops.

> libxl__openptys might be deserving of its own home outside
> libxl_bootloader.c?

I considered this but it's not that long and there are currently no
other callers.

> > +        if (rc) { LOGE(ERROR,"sendmsg to parent failed"); _exit(-1); }
> > +        _exit(0);
> > +    }
> > +
> > +    libxl__carefd_close(for_child);
> 
> this is sockets[1], which we then read below (via recvmsg_fds)? I
> suspect one or the other is wrong (I think the read).

Yes, the read is wrong.

> > + out:
> > +    if (sockets[0] >= 0) close(sockets[0]);
> > +    if (sockets[1] >= 0) close(sockets[1]);
> > +    libxl__carefd_close(for_child);
> 
> for_child == sockets[1], so now we've closed it twice.

Well spotted.

> > +static void bootloader_arg(libxl__bootloader_state *bl, const char *arg) {
> > +    assert(bl->nargs < bl->argsspace);
> > +    bl->args[bl->nargs++] = arg;
> > +}
> 
> I'm not a huge fan of the flexarray stuff but is reinventing them again
> useful?

The flexarray stuff doesn't use the gc.  And we don't need a
variable-sized buffer.  So I don't think it buys us much here.

> (comes back later.. why not use XEN_RUN_DIR? Also you seem to have
> created bl->outputdir for this purpose anyway?)

The bug is that I wasn't using bl->outputdir.  It needs to have the
domid in it for obvious reasons.

> >      /* Sentinal for execv */
> 
> Pre-existing:
>           Sentinel

I'll fix that.

> > -    flexarray_set(args, nr++, NULL);
> > +    ARG(NULL);
> >
> > -    return (char **) flexarray_contents(args); /* Frees args */
> 
> Hopefully I'll find blargs getting free'd later on...

bl->args is from the gc.

More later...

Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel