Hi, Attached patch makes interactive pygrub working with Linux DomUs in a NetBSD Dom0. Please apply this patch to -unstable and 3.3-testing tree. Patch is based on http://hg.opensolaris.org/sc/src/xen-gate/devel-unstable-patches/pty-fixes Solaris changes have been ok''d by Sun (Frank van der Linden). Signed-off-by: Frank van der Linden <frank.vanderlinden@sun.com> Signed-off-by: Christoph Egger <Christoph.Egger@amd.com> -- ---to satisfy European Law for business letters: Advanced Micro Devices GmbH Karl-Hammerschmidt-Str. 34, 85609 Dornach b. Muenchen Geschaeftsfuehrer: Jochen Polster, Thomas M. McCoy, Giuliano Meroni Sitz: Dornach, Gemeinde Aschheim, Landkreis Muenchen Registergericht Muenchen, HRB Nr. 43632 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Christoph Egger writes ("[Xen-devel] [PATCH] pygrub: make it
work"):> - tty.setraw(m1);
> - fcntl.fcntl(m1, fcntl.F_SETFL, os.O_NDELAY);
> - os.close(s1)
> +
> + # On Solaris, the pty master side will get cranky if we try
> + # to write to it while there is no slave. To work around this,
> + # keep the slave descriptor open until we''re done. Set it
> + # to raw terminal parameters, otherwise it will echo back
> + # characters, which will confuse the I/O loop below.
> + # Furthermore, a raw master pty device has no terminal
> + # semantics on Solaris, so don''t try to set any attributes
> + # for it.
> + if os.uname()[0] != ''SunOS'' and os.uname()[0] !=
''NetBSD'':
Surely this dependence on uname is wrong. Why would set the termios
flags on the pty master ? They should be set on the slave
unconditionally.
And keeping the slave fd around for a while is harmless too. So I
guess I''m asking why these changes need to be conditional.
> + # filedescriptors:
> + # r - input from the bootloader (bootstring output)
> + # m1 - input/output from/to xenconsole
> + # m2 - input/output from/to pty that controls the bootloader
Can you explain what the purpose of this new code is or relevantly how
it differs from the old (ie, what was wrong with the old code) ?
Ian.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel
Ian Jackson wrote:> Surely this dependence on uname is wrong. Why would set the termios > flags on the pty master ? They should be set on the slave > unconditionally.It''s been a while since I originally did this patch, so my memory is a little rusty, but as far as I remember, I simply wanted to avoid breaking Linux after an earlier version of the patch did so. I tested the final version of the patch on Linux, and it worked fine. It could be that the uname tests aren''t needed anymore.> > And keeping the slave fd around for a while is harmless too. So I > guess I''m asking why these changes need to be conditional. > >> + # filedescriptors: >> + # r - input from the bootloader (bootstring output) >> + # m1 - input/output from/to xenconsole >> + # m2 - input/output from/to pty that controls the bootloader > > Can you explain what the purpose of this new code is or relevantly how > it differs from the old (ie, what was wrong with the old code) ?The comments I added explain it: + # The filedescriptors are NDELAY, so it''s ok to try to read + # bigger chunks than may be available, to keep e.g. curses + # screen redraws in the bootloader efficient. m1 is the side that + # gets xenconsole input, which will be keystrokes, so a small number + # is sufficient. m2 is pygrub output, which will be curses screen + # updates, so a larger number (1024) is appropriate there. + # + # For writeable descriptors, only include them in the set for select + # if there is actual data to write, otherwise this would loop too fast, + # eating up CPU time. - Frank _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Frank van der Linden writes ("Re: [Xen-devel] [PATCH] pygrub: make it
work"):> It''s been a while since I originally did this patch, so my memory
is a
> little rusty, but as far as I remember, I simply wanted to avoid
> breaking Linux after an earlier version of the patch did so. I tested
> the final version of the patch on Linux, and it worked fine. It could be
> that the uname tests aren''t needed anymore.
I think it would be better to have something that is the same
everywhere so I''d encourage you to resubmit without the special
casing (and I''ll help fix it if it goes wrong).
Although Keir may disagree :-).
> Ian Jackson wrote:
> > Can you explain what the purpose of this new code is or relevantly how
> > it differs from the old (ie, what was wrong with the old code) ?
>
> The comments I added explain it:
>
> + # The filedescriptors are NDELAY, so it''s ok to try to read
Yes, that''s a good explanation of the new code. What seems to be
lacking is an explanation of what was wrong with the old code - ie,
what should go in the changeset comment.
Ian.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel
Ian Jackson wrote:> > I think it would be better to have something that is the same > everywhere so I''d encourage you to resubmit without the special > casing (and I''ll help fix it if it goes wrong).I don''t have much time right now to test on Linux, but I''ll see if I can. Maybe Christoph can too.> Yes, that''s a good explanation of the new code. What seems to be > lacking is an explanation of what was wrong with the old code - ie, > what should go in the changeset comment. >The comments do explain what was wrong with the old code.. The old code had the following problems: 1) It was reading and writing data one byte at the time, leading to overhead. However, it''s safe to try to read/write bigger chunks, since the filedescriptors are NDELAY. 2) Instead of checking len(outbuf) / len(inbuf) for writes, simply do not include those filedescriptors in the select if there is nothing to write. Otherwise, the loop would run around in circles: the select would find that the output filedescriptors were writeable, but then the loop code would find that it had nothing to write to them. In the next iteration, the write filedescs would still be writeable, so select returns immediately, but there would still be no data to write. Etc, etc. It was essentially a while (1) until there was actual data to write. A waste of CPU cycles. - Frank _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Frank van der Linden writes ("Re: [Xen-devel] [PATCH] pygrub: make it
work"):> Ian Jackson wrote:
> > I think it would be better to have something that is the same
> > everywhere so I''d encourage you to resubmit without the
special
> > casing (and I''ll help fix it if it goes wrong).
>
> I don''t have much time right now to test on Linux, but
I''ll see if I
> can. Maybe Christoph can too.
No, I mean, please resubmit the patch without test for Linux. We
should apply it. The changes you quote look sensible to me. If they
break for someone I will help them debug it or do it myself.
> The comments do explain what was wrong with the old code..
> The old code had the following problems:
The explanation you just gave was what I was looking for. In general
I think there is a difference between explaining the code as it
currently is and explaining what was wrong with the old code.
The former should be done in comments (if and only if it is not
sufficiently clear from the code itself), and the latter should be
done in checkin comments.
Thanks,
Ian.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel