Re: [Qemu-devel] [PATCH 3/6] gtk: add virtual console support

2012-02-25 Thread Stefan Weil 

Am 20.02.2012 00:45, schrieb Anthony Liguori:
This enables VteTerminal to be used to render the text consoles. 
VteTerminal is
the same widget used by gnome-terminal which means it's VT100 
emulation is as

good as they come.

It's also screen reader accessible, supports copy/paste, proper 
scrolling and

most of the other features you would expect from a terminal widget.

Signed-off-by: Anthony Liguori aligu...@us.ibm.com
---
ui/gtk.c | 138 
++

1 files changed, 138 insertions(+), 0 deletions(-)

diff --git a/ui/gtk.c b/ui/gtk.c
index 502705b..bf65a4f 100644
--- a/ui/gtk.c
+++ b/ui/gtk.c

[...]

+static int gd_vc_handler(QemuOpts *opts, CharDriverState **chrp)
+{
+ CharDriverState *chr;
+
+ chr = g_malloc0(sizeof(*chr));


Some time ago, there was a decision to prefer g_new / g_new0:

chr = g_new0(CharDriverState, 1);

In function gtk_display_init there is also a g_malloc0 which
should be replaced by g_new0.

Regards,

Stefan Weil

Re: [Qemu-devel] [PATCH 3/6] gtk: add virtual console support

2012-02-25 Thread Anthony Liguori 

On 02/25/2012 10:21 AM, Stefan Weil wrote:

Am 20.02.2012 00:45, schrieb Anthony Liguori:

This enables VteTerminal to be used to render the text consoles. VteTerminal is
the same widget used by gnome-terminal which means it's VT100 emulation is as
good as they come.

It's also screen reader accessible, supports copy/paste, proper scrolling and
most of the other features you would expect from a terminal widget.

Signed-off-by: Anthony Liguori aligu...@us.ibm.com
---
ui/gtk.c | 138 ++
1 files changed, 138 insertions(+), 0 deletions(-)

diff --git a/ui/gtk.c b/ui/gtk.c
index 502705b..bf65a4f 100644
--- a/ui/gtk.c
+++ b/ui/gtk.c

[...]

+static int gd_vc_handler(QemuOpts *opts, CharDriverState **chrp)
+{
+ CharDriverState *chr;
+
+ chr = g_malloc0(sizeof(*chr));


Some time ago, there was a decision to prefer g_new / g_new0:


I'm not sure where the book of decisions is kept, but I certainly don't agree. 
a = malloc(sizeof(*a)) is an incredibly common pattern in QEMU.


It would be silly to change this pattern without good cause.

Regards,

Anthony Liguori



chr = g_new0(CharDriverState, 1);

In function gtk_display_init there is also a g_malloc0 which
should be replaced by g_new0.

Regards,

Stefan Weil

Re: [Qemu-devel] [PATCH 3/6] gtk: add virtual console support

2012-02-25 Thread Stefan Weil 

Am 25.02.2012 20:49, schrieb Anthony Liguori:

On 02/25/2012 10:21 AM, Stefan Weil wrote:

Am 20.02.2012 00:45, schrieb Anthony Liguori:
This enables VteTerminal to be used to render the text consoles. 
VteTerminal is
the same widget used by gnome-terminal which means it's VT100 
emulation is as

good as they come.

It's also screen reader accessible, supports copy/paste, proper 
scrolling and

most of the other features you would expect from a terminal widget.

Signed-off-by: Anthony Liguori aligu...@us.ibm.com
---
ui/gtk.c | 138 
++

1 files changed, 138 insertions(+), 0 deletions(-)

diff --git a/ui/gtk.c b/ui/gtk.c
index 502705b..bf65a4f 100644
--- a/ui/gtk.c
+++ b/ui/gtk.c

[...]

+static int gd_vc_handler(QemuOpts *opts, CharDriverState **chrp)
+{
+ CharDriverState *chr;
+
+ chr = g_malloc0(sizeof(*chr));


Some time ago, there was a decision to prefer g_new / g_new0:


I'm not sure where the book of decisions is kept, but I certainly 
don't agree. a = malloc(sizeof(*a)) is an incredibly common pattern in 
QEMU.


It would be silly to change this pattern without good cause.

Regards,

Anthony Liguori


Hi Anthony,

there is no book of decisions for QEMU, but there is best practice.
As far as I remember this topic was first discussed in this
qemu-devel thread:

http://lists.nongnu.org/archive/html/qemu-devel/2011-10/msg01988.html

a = malloc(sizeof(*a)) is no longer a valid pattern for QEMU
since you introduced glib-2.0. Those calls were converted
to a = g_malloc(sizeof(*a)) which was reasonable for a simple
automated code conversion.

I also used the g_malloc pattern in my patch, but was convinced
that glib-2.0 offers a better alternative using g_new.

Kind regards,

Stefan Weil

Re: [Qemu-devel] [PATCH 3/6] gtk: add virtual console support

2012-02-25 Thread Anthony Liguori 

On 02/25/2012 02:22 PM, Stefan Weil wrote:

Am 25.02.2012 20:49, schrieb Anthony Liguori:

On 02/25/2012 10:21 AM, Stefan Weil wrote:

Am 20.02.2012 00:45, schrieb Anthony Liguori:

This enables VteTerminal to be used to render the text consoles. VteTerminal is
the same widget used by gnome-terminal which means it's VT100 emulation is as
good as they come.

It's also screen reader accessible, supports copy/paste, proper scrolling and
most of the other features you would expect from a terminal widget.

Signed-off-by: Anthony Liguori aligu...@us.ibm.com
---
ui/gtk.c | 138 ++
1 files changed, 138 insertions(+), 0 deletions(-)

diff --git a/ui/gtk.c b/ui/gtk.c
index 502705b..bf65a4f 100644
--- a/ui/gtk.c
+++ b/ui/gtk.c

[...]

+static int gd_vc_handler(QemuOpts *opts, CharDriverState **chrp)
+{
+ CharDriverState *chr;
+
+ chr = g_malloc0(sizeof(*chr));


Some time ago, there was a decision to prefer g_new / g_new0:


I'm not sure where the book of decisions is kept, but I certainly don't agree.
a = malloc(sizeof(*a)) is an incredibly common pattern in QEMU.

It would be silly to change this pattern without good cause.

Regards,

Anthony Liguori


Hi Anthony,

there is no book of decisions for QEMU, but there is best practice.
As far as I remember this topic was first discussed in this
qemu-devel thread:

http://lists.nongnu.org/archive/html/qemu-devel/2011-10/msg01988.html

a = malloc(sizeof(*a)) is no longer a valid pattern for QEMU
since you introduced glib-2.0. Those calls were converted
to a = g_malloc(sizeof(*a)) which was reasonable for a simple
automated code conversion.

I also used the g_malloc pattern in my patch, but was convinced
that glib-2.0 offers a better alternative using g_new.


I meant g_malloc of course.

If we want to have a best practice, we should document it in CODING_STYLE. 
But I wouldn't agree with such a patch to coding style anyway.


Regards,

Anthony Liguori



Kind regards,

Stefan Weil

Re: [Qemu-devel] [PATCH 3/6] gtk: add virtual console support

2012-02-20 Thread Stefan Weil 

Am 20.02.2012 00:45, schrieb Anthony Liguori:
This enables VteTerminal to be used to render the text consoles. 
VteTerminal is
the same widget used by gnome-terminal which means it's VT100 
emulation is as

good as they come.

It's also screen reader accessible, supports copy/paste, proper 
scrolling and

most of the other features you would expect from a terminal widget.

Signed-off-by: Anthony Liguori aligu...@us.ibm.com
---
ui/gtk.c | 138 
++

1 files changed, 138 insertions(+), 0 deletions(-)




The new code uses VtePty and some functions which were added to
VteTerminal in 2010, so this is a rather new interface:

   commit dd08c50c6a6dd4349d3cbce271ddf4b741db8861
   Autor: Christian Persch c...@gnome.org Do Jan 14 18:08:33 2010
   Eintragender: Christian Persch c...@gnome.org Mi Mär 17 18:22:15 2010

   Add VtePty and adapt the VteTerminal APIs to it

   Add VtePty as a GObject holding the info about the PTY. Add new API to
   VteTerminal to set a VtePty, and deprecate the old API that takes a FD
   to the PTY. Also deprecate the whole of the undocumented _vte_pty_*()
   APIs.

   Add vte_terminal_fork_command_full() variant that allow providing a
   custom child setup function and that returns a GError on failure.


= It does not work with Debian stable and other old distributions.

configure detects GTK+ and VTE, but make fails.

Regards,

Stefan Weil