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

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Wed, 2012-03-21 at 12:17 +0000, Ian Jackson wrote:
> 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.

That's very reasonable...

FWIW most of this code was copied from the xend equivalent (actually a C
python binding) which I think was actually cobbled together from actual
users of the platforms in question, not that this makes it correct
necessarily.

> This is now done with libxl_fd_set_nonblock immediately after the call
> to carefd.
[...]
> This code has not actually been removed by my patch.  It's still there
> at the end of bootloader_gotptys.

Sorry, I spotted both of these later and neglected to come back and
remove these comments.

> > > +    } 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.

Right, this is obvious when you put it that way. In my defence it had
been a long review session...

Ian.


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