Patchwork [1/9] chardev: add greeting

login
register
mail settings
Submitter Gerd Hoffmann
Date Nov. 17, 2009, 9:38 a.m.
Message ID <1258450699-24445-2-git-send-email-kraxel@redhat.com>
Download mbox | patch
Permalink /patch/38599/
State New
Headers show

Comments

Gerd Hoffmann - Nov. 17, 2009, 9:38 a.m.
Add a greeting string to CharDriverState which is printed after
initialization.  Used to have the qemu vc consoles labeled.  This
way we can avoid walking all the chardevs a second time after
initialization just to print the greeting.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 qemu-char.c |    2 ++
 qemu-char.h |    1 +
 vl.c        |   36 ++++++++++++------------------------
 3 files changed, 15 insertions(+), 24 deletions(-)
Markus Armbruster - Nov. 20, 2009, 5 p.m.
Gerd Hoffmann <kraxel@redhat.com> writes:

> Add a greeting string to CharDriverState which is printed after
> initialization.  Used to have the qemu vc consoles labeled.  This
> way we can avoid walking all the chardevs a second time after
> initialization just to print the greeting.

I doubt this would be worthwhile on its own, but it enables getting rid
of serial_devices[] and parallel_devices[] later in the series, which I
like.
Paul Brook - Nov. 20, 2009, 5:41 p.m.
On Tuesday 17 November 2009, Gerd Hoffmann wrote:
> Add a greeting string to CharDriverState which is printed after
> initialization.  Used to have the qemu vc consoles labeled.  This
> way we can avoid walking all the chardevs a second time after
> initialization just to print the greeting.

I think "greeting" is propagating a bad idea into new code. Much better would 
be some form of ID and/or human readable description that can also be used 
elsewhere.

Paul
Gerd Hoffmann - Nov. 23, 2009, 8:22 a.m.
On 11/20/09 18:41, Paul Brook wrote:
> On Tuesday 17 November 2009, Gerd Hoffmann wrote:
>> Add a greeting string to CharDriverState which is printed after
>> initialization.  Used to have the qemu vc consoles labeled.  This
>> way we can avoid walking all the chardevs a second time after
>> initialization just to print the greeting.
>
> I think "greeting" is propagating a bad idea into new code. Much better would
> be some form of ID and/or human readable description that can also be used
> elsewhere.

The naming is only one part of the problem.  The second part is that the 
greeting is printed only for the 'vc' backend (where you really need it 
because there is no other way to figure what chardev you are looking at 
when switching screens via Ctrl-Alt-<nr>).

There already is a 'label' field.  So we could add a flag instead of a 
string, then do:

    if (chr->want_greeting)
        qemu_chr_printf(chr, "%s console\n", chr->label);

How about this?

cheers,
   Gerd
Paul Brook - Nov. 23, 2009, 1:26 p.m.
On Monday 23 November 2009, Gerd Hoffmann wrote:
> On 11/20/09 18:41, Paul Brook wrote:
> > On Tuesday 17 November 2009, Gerd Hoffmann wrote:
> >> Add a greeting string to CharDriverState which is printed after
> >> initialization.  Used to have the qemu vc consoles labeled.  This
> >> way we can avoid walking all the chardevs a second time after
> >> initialization just to print the greeting.
> >
> > I think "greeting" is propagating a bad idea into new code. Much better
> > would be some form of ID and/or human readable description that can also
> > be used elsewhere.
> 
> The naming is only one part of the problem.  The second part is that the
> greeting is printed only for the 'vc' backend (where you really need it
> because there is no other way to figure what chardev you are looking at
> when switching screens via Ctrl-Alt-<nr>).
> 
> There already is a 'label' field.  So we could add a flag instead of a
> string, then do:
> 
>     if (chr->want_greeting)
>         qemu_chr_printf(chr, "%s console\n", chr->label);
> 
> How about this?

I thinking more that this should be done by the character backend itself.  For 
example, the "graphical" consoles should probably be putting this as part of 
the window title rather than having the interface layer randomly send extra 
characters in connect.

Paul
Gerd Hoffmann - Nov. 23, 2009, 3:10 p.m.
On 11/23/09 14:26, Paul Brook wrote:
> I thinking more that this should be done by the character backend itself.  For
> example, the "graphical" consoles should probably be putting this as part of
> the window title

Doesn't work with vnc.

cheers,
   Gerd
Paul Brook - Nov. 23, 2009, 3:18 p.m.
On Monday 23 November 2009, Gerd Hoffmann wrote:
> On 11/23/09 14:26, Paul Brook wrote:
> > I thinking more that this should be done by the character backend itself.
> >  For example, the "graphical" consoles should probably be putting this as
> > part of the window title
> 
> Doesn't work with vnc.

That's why it needs to be a backend decision. The VNC devices should display 
his information however is most appropriate.

Paul
Daniel P. Berrange - Nov. 23, 2009, 3:20 p.m.
On Mon, Nov 23, 2009 at 04:10:48PM +0100, Gerd Hoffmann wrote:
> On 11/23/09 14:26, Paul Brook wrote:
> >I thinking more that this should be done by the character backend itself.  
> >For
> >example, the "graphical" consoles should probably be putting this as part 
> >of
> >the window title
> 
> Doesn't work with vnc.

It could be made to actually, there's a VNC extension for sending updates
to the desktop name

  http://www.tigervnc.org/cgi-bin/rfbproto#desktopname-pseudo-encoding

I'd happily implement that in GTK-VNC if QEMU had the server side

Regards,
Daniel
Anthony Liguori - Nov. 23, 2009, 3:20 p.m.
Gerd Hoffmann wrote:
> On 11/23/09 14:26, Paul Brook wrote:
>> I thinking more that this should be done by the character backend 
>> itself.  For
>> example, the "graphical" consoles should probably be putting this as 
>> part of
>> the window title
>
> Doesn't work with vnc.

A vc is what renders the for VNC so if the "vc" did this, it would just 
work for VNC and SDL.

>
> cheers,
>   Gerd
>
>
Gerd Hoffmann - Nov. 23, 2009, 4:13 p.m.
On 11/23/09 16:20, Anthony Liguori wrote:
> Gerd Hoffmann wrote:
>> On 11/23/09 14:26, Paul Brook wrote:
>>> I thinking more that this should be done by the character backend
>>> itself. For
>>> example, the "graphical" consoles should probably be putting this as
>>> part of
>>> the window title
>>
>> Doesn't work with vnc.
>
> A vc is what renders the for VNC so if the "vc" did this, it would just
> work for VNC and SDL.

Yea, right, as vc is the only user we can easily make this local to 
console.c.  Good idea.  We can even colorize it then ;)

What I was referring to is that updating the window title of the vnc 
client isn't going to work.  Well, at least not that easy.  As Daniel 
points out a extension for that exists, but I'd tend to not depend on a 
optional vnc client feature for this.

cheers,
   Gerd
Anthony Liguori - Dec. 2, 2009, 2:55 p.m.
Gerd Hoffmann wrote:
> On 11/23/09 16:20, Anthony Liguori wrote:
>> Gerd Hoffmann wrote:
>>> On 11/23/09 14:26, Paul Brook wrote:
>>>> I thinking more that this should be done by the character backend
>>>> itself. For
>>>> example, the "graphical" consoles should probably be putting this as
>>>> part of
>>>> the window title
>>>
>>> Doesn't work with vnc.
>>
>> A vc is what renders the for VNC so if the "vc" did this, it would just
>> work for VNC and SDL.
>
> Yea, right, as vc is the only user we can easily make this local to 
> console.c.  Good idea.  We can even colorize it then ;)
>
> What I was referring to is that updating the window title of the vnc 
> client isn't going to work.  Well, at least not that easy.  As Daniel 
> points out a extension for that exists, but I'd tend to not depend on 
> a optional vnc client feature for this.

It will at least be updated on reconnect.

Regards,

Anthony Liguori

Patch

diff --git a/qemu-char.c b/qemu-char.c
index 40bd7e8..19be58f 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -132,6 +132,8 @@  void qemu_chr_initial_reset(void)
 
     QTAILQ_FOREACH(chr, &chardevs, next) {
         qemu_chr_reset(chr);
+        if (chr->greeting)
+            qemu_chr_printf(chr, "%s", chr->greeting);
     }
 }
 
diff --git a/qemu-char.h b/qemu-char.h
index 05fe15d..e50a4f3 100644
--- a/qemu-char.h
+++ b/qemu-char.h
@@ -66,6 +66,7 @@  struct CharDriverState {
     QEMUBH *bh;
     char *label;
     char *filename;
+    char *greeting;
     QTAILQ_ENTRY(CharDriverState) next;
 };
 
diff --git a/vl.c b/vl.c
index fff8e8d..095aff6 100644
--- a/vl.c
+++ b/vl.c
@@ -5657,6 +5657,10 @@  int main(int argc, char **argv, char **envp)
                         devname, strerror(errno));
                 exit(1);
             }
+            if (strstart(devname, "vc", 0)) {
+                snprintf(label, sizeof(label), "serial%d console\r\n", i);
+                serial_hds[i]->greeting = qemu_strdup(label);
+            }
         }
     }
 
@@ -5671,6 +5675,10 @@  int main(int argc, char **argv, char **envp)
                         devname, strerror(errno));
                 exit(1);
             }
+            if (strstart(devname, "vc", 0)) {
+                snprintf(label, sizeof(label), "parallel%d console\r\n", i);
+                parallel_hds[i]->greeting = qemu_strdup(label);
+            }
         }
     }
 
@@ -5685,6 +5693,10 @@  int main(int argc, char **argv, char **envp)
                         devname, strerror(errno));
                 exit(1);
             }
+            if (strstart(devname, "vc", 0)) {
+                snprintf(label, sizeof(label), "virtio console%d\r\n", i);
+                virtcon_hds[i]->greeting = qemu_strdup(label);
+            }
         }
     }
 
@@ -5800,30 +5812,6 @@  int main(int argc, char **argv, char **envp)
         }
     }
 
-    for(i = 0; i < MAX_SERIAL_PORTS; i++) {
-        const char *devname = serial_devices[i];
-        if (devname && strcmp(devname, "none")) {
-            if (strstart(devname, "vc", 0))
-                qemu_chr_printf(serial_hds[i], "serial%d console\r\n", i);
-        }
-    }
-
-    for(i = 0; i < MAX_PARALLEL_PORTS; i++) {
-        const char *devname = parallel_devices[i];
-        if (devname && strcmp(devname, "none")) {
-            if (strstart(devname, "vc", 0))
-                qemu_chr_printf(parallel_hds[i], "parallel%d console\r\n", i);
-        }
-    }
-
-    for(i = 0; i < MAX_VIRTIO_CONSOLES; i++) {
-        const char *devname = virtio_consoles[i];
-        if (virtcon_hds[i] && devname) {
-            if (strstart(devname, "vc", 0))
-                qemu_chr_printf(virtcon_hds[i], "virtio console%d\r\n", i);
-        }
-    }
-
     if (gdbstub_dev && gdbserver_start(gdbstub_dev) < 0) {
         fprintf(stderr, "qemu: could not open gdbserver on device '%s'\n",
                 gdbstub_dev);