[Xen-devel] Re: [PATCH,RFC]: Introduce libxl_domain_create()

[Gianni Tedesco <gianni.tedesco@xxxxxxxxxx>]

On Tue, 2010-12-14 at 19:37 +0000, Ian Jackson wrote:
> Gianni Tedesco writes ("[PATCH,RFC]: Introduce libxl_domain_create()"):
> > Any thoughts or suggestions?
> 
> Thanks.  It seems to be roughly along the right lines.
> 
> I'll put my comments inline ...
> 
> >  /* domain related functions */
> > +#define LIBXL_CREATE_RESTORE            (1<<0)
> > +#define LIBXL_CREATE_RUN_BOOTLOADER     (1<<1)
> > +#define LIBXL_CREATE_CONSOLE_CONNECT    (1<<2)
> 
> If we don't say CREATE_RUN_BOOTLOADER, where or when does the
> bootloader run ?
> 
> I'm not sure how a daemon caller of the library supposed to deal with
> this.  Supppose the library caller is virt-manager, which is a
> daemon.  Is the bootloader going to run with no console connected, or
> what ?

AFAICT this is just so that we don't run the bootloader during
resume/migrate-receive. I've been a bit of a scaredy cat and left
seperate flags for each bit of logic that was in the original function.

To be honest, I'm not sure I want to stick with the flags based API.

> > +#define MUST( call ) ({                                                 \
> > +        int must_rc = (call);                                           \
> > +        if (must_rc < 0) {                                                 
> >  \
> > +            fprintf(stderr,"xl: fatal error: %s:%d, rc=%d: %s\n",       \
> > +                    __FILE__,__LINE__, must_rc, #call);                 \
> > +            exit(-must_rc);                                             \
> > +        }                                                               \
> > +    })
> 
> As a library function, the error handling needs to be changed.  It
> needs to not cause your whole process to exit.  Instead, it should
> return an error to the caller.  And error messages should go to the
> log.

Well spotted

> > +    console->output = strdup("pty");
> 
> I think this should be some kind of checked strdup, surely ?

Again, yes.

> > +#if 0
> >  waitpid_out:
> >      if (child_console_pid > 0 &&
> >              waitpid(child_console_pid, &status, 0) < 0 && errno == EINTR)
> >          goto waitpid_out;
> > +#endif
> 
> Uh ?

Heh, actualyl I wasn't sure if this was needed or not but I think it is
if we had run the daemon. Need to fix that.

Gianni


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