| Message ID | 1357219383-30748-1-git-send-email-kraxel@redhat.com |
|---|---|
| State | New |
| Headers | show |
On Thu, Jan 03, 2013 at 02:23:03PM +0100, Gerd Hoffmann wrote: > Commit 586502189edf9fd0f89a83de96717a2ea826fdb0 breaks libvirt pty > support because it tried to figure the pts name from stderr output. > > Fix this by moving the label to the end of the line, this way the > libvirt parser does still recognise the message. libvirt looks > for "char device redirected to ${ptsname}<whitespace>". FWIW, libvirt was not supposed to be parsing this data still. We rely on query-chardev to get the PTYs, but we were accidentally still invoking the stdio parsing code even though we didn't use the result :-( This flaw is fixed in latest libvirt GIT. > > Signed-off-by: Gerd Hoffmann <kraxel@redhat.com> > --- > qemu-char.c | 9 +++++---- > 1 files changed, 5 insertions(+), 4 deletions(-) > > diff --git a/qemu-char.c b/qemu-char.c > index 331ad5c..f41788c 100644 > --- a/qemu-char.c > +++ b/qemu-char.c > @@ -1012,10 +1012,11 @@ static CharDriverState *qemu_chr_open_pty(QemuOpts *opts) > qemu_opt_set(opts, "path", q_ptsname(master_fd)); > > label = qemu_opts_id(opts); > - fprintf(stderr, "char device%s%s redirected to %s\n", > - label ? " " : "", > - label ?: "", > - q_ptsname(master_fd)); > + fprintf(stderr, "char device redirected to %s%s%s%s\n", > + q_ptsname(master_fd), > + label ? " (label " : "", > + label ? label : "", > + label ? ")" : ""); > > s = g_malloc0(sizeof(PtyCharDriver)); > chr->opaque = s; Daniel
"Daniel P. Berrange" <berrange@redhat.com> writes: > On Thu, Jan 03, 2013 at 02:23:03PM +0100, Gerd Hoffmann wrote: >> Commit 586502189edf9fd0f89a83de96717a2ea826fdb0 breaks libvirt pty >> support because it tried to figure the pts name from stderr output. >> >> Fix this by moving the label to the end of the line, this way the >> libvirt parser does still recognise the message. libvirt looks >> for "char device redirected to ${ptsname}<whitespace>". > > FWIW, libvirt was not supposed to be parsing this data still. > We rely on query-chardev to get the PTYs, but we were accidentally > still invoking the stdio parsing code even though we didn't use > the result :-( Thanks for the explanation. I thought about libvirt before applying this but had figured it was using query-chardev. I still think this is a reasonable change to make though even if the latest libvirt doesn't need it so I'll apply it. Regards, Anthony Liguori > > This flaw is fixed in latest libvirt GIT. > >> >> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com> >> --- >> qemu-char.c | 9 +++++---- >> 1 files changed, 5 insertions(+), 4 deletions(-) >> >> diff --git a/qemu-char.c b/qemu-char.c >> index 331ad5c..f41788c 100644 >> --- a/qemu-char.c >> +++ b/qemu-char.c >> @@ -1012,10 +1012,11 @@ static CharDriverState *qemu_chr_open_pty(QemuOpts *opts) >> qemu_opt_set(opts, "path", q_ptsname(master_fd)); >> >> label = qemu_opts_id(opts); >> - fprintf(stderr, "char device%s%s redirected to %s\n", >> - label ? " " : "", >> - label ?: "", >> - q_ptsname(master_fd)); >> + fprintf(stderr, "char device redirected to %s%s%s%s\n", >> + q_ptsname(master_fd), >> + label ? " (label " : "", >> + label ? label : "", >> + label ? ")" : ""); >> >> s = g_malloc0(sizeof(PtyCharDriver)); >> chr->opaque = s; > > > Daniel > -- > |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| > |: http://libvirt.org -o- http://virt-manager.org :| > |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| > |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
Thanks, applied. Regards, Anthony Liguori
On 3 January 2013 19:00, Anthony Liguori <aliguori@us.ibm.com> wrote:
> Thanks, applied.
So we now say "char device redirected to /dev/pts/5 (compat_monitor0)"
rather than "char device compat_monitor0 redirected to /dev/pts/5" ?
I think that's a reduction in clarity and it's sad that we have to do it.
I also think that everywhere we have something with a specific
format which we're retaining for the benefit of libvirt we should
have big warning comments saying "Do not change this because
libvirt versions older than X.Y depend upon the exact text".
Otherwise we'll just trip over the same bugs again later.
-- PMM
Peter Maydell <peter.maydell@linaro.org> writes: > On 3 January 2013 19:00, Anthony Liguori <aliguori@us.ibm.com> wrote: >> Thanks, applied. > > So we now say "char device redirected to /dev/pts/5 (compat_monitor0)" > rather than "char device compat_monitor0 redirected to /dev/pts/5" ? > I think that's a reduction in clarity and it's sad that we have to do it. > > I also think that everywhere we have something with a specific > format which we're retaining for the benefit of libvirt we should > have big warning comments saying "Do not change this because > libvirt versions older than X.Y depend upon the exact text". > Otherwise we'll just trip over the same bugs again later. I don't intend that we keep this forever. But it's hardly compelling to do it one way vs. the other so compatibility wins. Regards, Anthony Liguori > > -- PMM
diff --git a/qemu-char.c b/qemu-char.c index 331ad5c..f41788c 100644 --- a/qemu-char.c +++ b/qemu-char.c @@ -1012,10 +1012,11 @@ static CharDriverState *qemu_chr_open_pty(QemuOpts *opts) qemu_opt_set(opts, "path", q_ptsname(master_fd)); label = qemu_opts_id(opts); - fprintf(stderr, "char device%s%s redirected to %s\n", - label ? " " : "", - label ?: "", - q_ptsname(master_fd)); + fprintf(stderr, "char device redirected to %s%s%s%s\n", + q_ptsname(master_fd), + label ? " (label " : "", + label ? label : "", + label ? ")" : ""); s = g_malloc0(sizeof(PtyCharDriver)); chr->opaque = s;
Commit 586502189edf9fd0f89a83de96717a2ea826fdb0 breaks libvirt pty support because it tried to figure the pts name from stderr output. Fix this by moving the label to the end of the line, this way the libvirt parser does still recognise the message. libvirt looks for "char device redirected to ${ptsname}<whitespace>". Signed-off-by: Gerd Hoffmann <kraxel@redhat.com> --- qemu-char.c | 9 +++++---- 1 files changed, 5 insertions(+), 4 deletions(-)