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:
> > @@ -92,309 +241,360 @@ static int open_xenconsoled_pty(int *master, int 
> > *slave, char *slave_path, size_
> >       * for it.
> >       */
> >  #if !defined(__sun__) && !defined(__NetBSD__)
> > +    tcgetattr(master, &termattr);
> >      cfmakeraw(&termattr);
> > +    tcsetattr(master, TCSANOW, &termattr);
> >
> > +    close(slave);
> > +    slave = -1;
> >  #else
> > +    tcgetattr(slave, &termattr);
> >      cfmakeraw(&termattr);
> > +    tcsetattr(slave, TCSANOW, &termattr);
> >  #endif
> 
> Could this stuff be usefully pushed into libxl__openpty?

Hmm.  This is a bit of a can of worms I think.  When I wrote my patch
I left it well alone.  TBH I think the current code is arguably wrong.
I can see no reason why we would try to do termios operations on the
pty master at all.  I think these functions should be performed on the
slave on all platforms.

But I don't want to introduce a perhaps-not-wonderfully-portable
change like that in this series.

> > -    fcntl(*master, F_SETFL, O_NDELAY);
> 
> Why not?

This is now done with libxl_fd_set_nonblock immediately after the call
to carefd.

> > -    /*
> > -     * On Solaris, the master pty side does not have terminal semantics,
> > -     * so don't try to set any attributes, as it will fail.
> > -     */
> > -#if !defined(__sun__)
> > -    tcgetattr(*master, &termattr);
> > -    cfmakeraw(&termattr);
> > -    tcsetattr(*master, TCSANOW, &termattr);
> > -#endif
> 
> I think we are right to give up any pretence of supporting Solaris now.

This code has not actually been removed by my patch.  It's still there
at the end of bootloader_gotptys.

> > +    } else if (!libxl__ev_fd_isregistered(&dc->toread)) {
> > +        /* we have had eof */
> > +        libxl__datacopier_kill(gc, dc);
> > +        dc->callback(egc, dc, 0, 0);
> > +        return;
> > +    } else {
> > +        /* nothing buffered, but still reading */
> > +        libxl__ev_fd_deregister(gc, &dc->towrite);
> 
> is it worth the effort to handle only registering for write events when
> something is buffered? It would be simpler to just set it up at start of
> day and leave it until done?

If you register for write events that means the event loop will spin
continually offering you the chance to write something.  Or to put it
another way, the event callback function must either make the original
condition it was triggering on not true, or it must disable the event.

> > +    assert(revents & POLLIN);
> > +    for (;;) {
> > +        while (dc->used >= dc->maxsz) {
> 
> This is the overflow discard loop, correct? (took me a while to figure
> that out, in the meantime I was v. confused ;-))

Yes.  We discard a whole buffer at a time.

> > +void libxl__bootloader_init(libxl__bootloader_state *bl)
> >  {
> [...]
> > +    bl->diskpath = NULL;
> > +    bl->ptys[0].master = bl->ptys[0].slave = 0;
> > +    bl->ptys[1].master = bl->ptys[1].slave = 0;
> 
> Not -1? A more typical "invalid" fd value.

These are libxl__carefd*.

> > +    for (i=0; i<2; i++) {
> > +        libxl__carefd_close(bl->ptys[0].master);
> > +        libxl__carefd_close(bl->ptys[0].slave);
> 
> Don't you mean [i] rather than [0] both times here?

Yes, oops.

> [...]
> > +    if (!pid) {
> > +        /* child */
> > +        r = login_tty(libxl__carefd_fd(bl->ptys[0].slave));
> 
> login_tty is a new one to me. When I lookup up the manpage I happened to
> notice:
>        Link with -lutil.

Hmm.  The BSD manpage is more forthcoming and suggests we need
something different there.  (This isn't a particularly portable
function.)

I think we need some more autoconfery.

> > +/* onwrite==1 means failure happend when writing, logged, errno is valid
> > + * onwrite==0 means failure happend when reading
> 
>                               happened (twice)

Fixed.

Ian.

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