[Qemu-devel] [PATCH] pty: unbreak libvirt

[Qemu-devel] [PATCH] pty: unbreak libvirt

[Gerd Hoffmann]

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(-)

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;
-- 
1.7.1

Re: [Qemu-devel] [PATCH] pty: unbreak libvirt

[Daniel P. Berrange]

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
-- 
|: 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 :|

Re: [Qemu-devel] [PATCH] pty: unbreak libvirt

[Anthony Liguori]

"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 :|

Re: [Qemu-devel] [PATCH] pty: unbreak libvirt

[Anthony Liguori]

Thanks, applied.

Regards,

Anthony Liguori

Re: [Qemu-devel] [PATCH] pty: unbreak libvirt

[Peter Maydell]

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

Re: [Qemu-devel] [PATCH] pty: unbreak libvirt

[Anthony Liguori]

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