Re: [PATCH] ui/gtk: Negative Page number is not valid

2024-06-26 Thread Kim, Dongwon 

On 6/26/2024 10:06 AM, Philippe Mathieu-Daudé wrote:

Hi Dongwon,

On 26/6/24 02:08, dongwon@intel.com wrote:

From: Dongwon Kim 

Negative page number means the page with that number does not
belong to the notebook so it shouldn't be used as a valid page
number in gd_vc_find_by_page. This function should just return
null in such case.

This change, however, will cause a segfault during detaching
/untabifying process in gtk_release_modifiers because the
current VC's page number suddenly becomes '-1' as soon as
the VC is detached, which makes gd_vc_find_by_page return
null. So gtk_release_modifiers should do the null check on
VC returned from gd_vc_find_by_page.

Cc: Gerd Hoffmann 
Cc: Marc-André Lureau 
Signed-off-by: Vivek Kasireddy 
Signed-off-by: Dongwon Kim 
---
  ui/gtk.c | 4 ++--
  1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/ui/gtk.c b/ui/gtk.c
index 93b13b7a30..1f8523fd81 100644
--- a/ui/gtk.c
+++ b/ui/gtk.c
@@ -164,7 +164,7 @@ static VirtualConsole 
*gd_vc_find_by_page(GtkDisplayState *s, gint page)


The caller should check gtk_notebook_get_current_page() != -1.

We might assert(page >= 0) here.


We could do that but it means there should be more checks in
other functions where gd_vc_find_by_page is called, like
gd_vc_find_current. And we just can't do assert in\
gd_vc_find_current because detached VC has the page number of -1.




  for (i = 0; i < s->nb_vcs; i++) {
  vc = &s->vc[i];
  p = gtk_notebook_page_num(GTK_NOTEBOOK(s->notebook), 
vc->tab_item);

-    if (p == page) {
+    if (p > -1 && p == page) {


Then this is not necessary.


  return vc;
  }
  }

     return NULL;

I wonder about returning NULL, maybe just

    g_assert_not_reached();


@@ -357,7 +357,7 @@ static void gtk_release_modifiers(GtkDisplayState *s)
  {
  VirtualConsole *vc = gd_vc_find_current(s);
-    if (vc->type != GD_VC_GFX ||
+    if (!vc || vc->type != GD_VC_GFX ||


Then this is not necessary.


  !qemu_console_is_graphic(vc->gfx.dcl.con)) {
  return;
  }

Re: [PATCH] ui/gtk: Negative Page number is not valid

2024-06-26 Thread Philippe Mathieu-Daudé 

Hi Dongwon,

On 26/6/24 02:08, dongwon@intel.com wrote:

From: Dongwon Kim 

Negative page number means the page with that number does not
belong to the notebook so it shouldn't be used as a valid page
number in gd_vc_find_by_page. This function should just return
null in such case.

This change, however, will cause a segfault during detaching
/untabifying process in gtk_release_modifiers because the
current VC's page number suddenly becomes '-1' as soon as
the VC is detached, which makes gd_vc_find_by_page return
null. So gtk_release_modifiers should do the null check on
VC returned from gd_vc_find_by_page.

Cc: Gerd Hoffmann 
Cc: Marc-André Lureau 
Signed-off-by: Vivek Kasireddy 
Signed-off-by: Dongwon Kim 
---
  ui/gtk.c | 4 ++--
  1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/ui/gtk.c b/ui/gtk.c
index 93b13b7a30..1f8523fd81 100644
--- a/ui/gtk.c
+++ b/ui/gtk.c
@@ -164,7 +164,7 @@ static VirtualConsole *gd_vc_find_by_page(GtkDisplayState 
*s, gint page)


The caller should check gtk_notebook_get_current_page() != -1.

We might assert(page >= 0) here.


  for (i = 0; i < s->nb_vcs; i++) {
  vc = &s->vc[i];
  p = gtk_notebook_page_num(GTK_NOTEBOOK(s->notebook), vc->tab_item);
-if (p == page) {
+if (p > -1 && p == page) {


Then this is not necessary.


  return vc;
  }
  }

return NULL;

I wonder about returning NULL, maybe just

   g_assert_not_reached();


@@ -357,7 +357,7 @@ static void gtk_release_modifiers(GtkDisplayState *s)
  {
  VirtualConsole *vc = gd_vc_find_current(s);
  
-if (vc->type != GD_VC_GFX ||

+if (!vc || vc->type != GD_VC_GFX ||


Then this is not necessary.


  !qemu_console_is_graphic(vc->gfx.dcl.con)) {
  return;
  }

[PATCH] ui/gtk: Negative Page number is not valid

2024-06-25 Thread dongwon . kim 
From: Dongwon Kim 

Negative page number means the page with that number does not
belong to the notebook so it shouldn't be used as a valid page
number in gd_vc_find_by_page. This function should just return
null in such case.

This change, however, will cause a segfault during detaching
/untabifying process in gtk_release_modifiers because the
current VC's page number suddenly becomes '-1' as soon as
the VC is detached, which makes gd_vc_find_by_page return
null. So gtk_release_modifiers should do the null check on
VC returned from gd_vc_find_by_page.

Cc: Gerd Hoffmann 
Cc: Marc-André Lureau 
Signed-off-by: Vivek Kasireddy 
Signed-off-by: Dongwon Kim 
---
 ui/gtk.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/ui/gtk.c b/ui/gtk.c
index 93b13b7a30..1f8523fd81 100644
--- a/ui/gtk.c
+++ b/ui/gtk.c
@@ -164,7 +164,7 @@ static VirtualConsole *gd_vc_find_by_page(GtkDisplayState 
*s, gint page)
 for (i = 0; i < s->nb_vcs; i++) {
 vc = &s->vc[i];
 p = gtk_notebook_page_num(GTK_NOTEBOOK(s->notebook), vc->tab_item);
-if (p == page) {
+if (p > -1 && p == page) {
 return vc;
 }
 }
@@ -357,7 +357,7 @@ static void gtk_release_modifiers(GtkDisplayState *s)
 {
 VirtualConsole *vc = gd_vc_find_current(s);
 
-if (vc->type != GD_VC_GFX ||
+if (!vc || vc->type != GD_VC_GFX ||
 !qemu_console_is_graphic(vc->gfx.dcl.con)) {
 return;
 }
-- 
2.34.1