[PATCH v2 0/4] ui/console: Remove console_select()

Akihiko Odaki posted 4 patches 1 month, 1 week ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20240319-console-v2-0-3fd6feef321a@daynix.com
Maintainers: Gerd Hoffmann <kraxel@redhat.com>, "Marc-André Lureau" <marcandre.lureau@redhat.com>, Peter Maydell <peter.maydell@linaro.org>, "Philippe Mathieu-Daudé" <philmd@linaro.org>, Akihiko Odaki <akihiko.odaki@daynix.com>
include/ui/console.h   |   2 +-
include/ui/kbd-state.h |  11 ++++
ui/console-priv.h      |   2 +-
ui/console-vc-stubs.c  |   2 +-
ui/console-vc.c        |   7 ++-
ui/console.c           | 133 ++++++++++++-------------------------------------
ui/curses.c            |  48 ++++++++++--------
ui/kbd-state.c         |   6 +++
ui/vnc.c               |  14 ++++--
ui/cocoa.m             |  37 ++++++++++----
10 files changed, 118 insertions(+), 144 deletions(-)
[PATCH v2 0/4] ui/console: Remove console_select()
Posted by Akihiko Odaki 1 month, 1 week ago
ui/console has a concept of "active" console; the active console is used
when NULL is set for DisplayListener::con, and console_select() updates
the active console state. However, the global nature of the state cause
odd behaviors, and replacing NULL with the active console also resulted
in extra code. Remove it to solve these problems.

The active console state is shared, so if there are two displays
referring to the active console, switching the console for one will also
affect the other. All displays that use the active console state,
namely cocoa, curses, and vnc, need to reset some of its state before
switching the console, and such a reset operation cannot be performed if
the console is switched by another display. This can result in stuck
keys, for example.

While the active console state is shared, displays other than cocoa,
curses, and vnc don't update the state. A chardev-vc inherits the
size of the active console, but it does not make sense for such a
display.

This series removes the shared "active" console state from ui/console.
curses, cocoa, and vnc will hold the reference to the console currently
shown with DisplayListener::con. This also eliminates the need to
replace NULL with the active console and save code.

Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
---
Changes in v2:
- Changed to fall back to a text console if there is no graphical
  console as previously done.
- Link to v1: https://lore.kernel.org/r/20240318-console-v1-0-f4efbfa71253@daynix.com

---
Akihiko Odaki (4):
      ui/vc: Do not inherit the size of active console
      ui/vnc: Do not use console_select()
      ui/cocoa: Do not use console_select()
      ui/curses: Do not use console_select()

 include/ui/console.h   |   2 +-
 include/ui/kbd-state.h |  11 ++++
 ui/console-priv.h      |   2 +-
 ui/console-vc-stubs.c  |   2 +-
 ui/console-vc.c        |   7 ++-
 ui/console.c           | 133 ++++++++++++-------------------------------------
 ui/curses.c            |  48 ++++++++++--------
 ui/kbd-state.c         |   6 +++
 ui/vnc.c               |  14 ++++--
 ui/cocoa.m             |  37 ++++++++++----
 10 files changed, 118 insertions(+), 144 deletions(-)
---
base-commit: ba49d760eb04630e7b15f423ebecf6c871b8f77b
change-id: 20240317-console-6744d4ab8ba6

Best regards,
-- 
Akihiko Odaki <akihiko.odaki@daynix.com>
Re: [PATCH v2 0/4] ui/console: Remove console_select()
Posted by Marc-André Lureau 1 month, 1 week ago
Hi Akihiko

On Tue, Mar 19, 2024 at 7:09 AM Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
>
> ui/console has a concept of "active" console; the active console is used
> when NULL is set for DisplayListener::con, and console_select() updates
> the active console state. However, the global nature of the state cause
> odd behaviors, and replacing NULL with the active console also resulted
> in extra code. Remove it to solve these problems.
>
> The active console state is shared, so if there are two displays
> referring to the active console, switching the console for one will also
> affect the other. All displays that use the active console state,
> namely cocoa, curses, and vnc, need to reset some of its state before
> switching the console, and such a reset operation cannot be performed if
> the console is switched by another display. This can result in stuck
> keys, for example.
>
> While the active console state is shared, displays other than cocoa,
> curses, and vnc don't update the state. A chardev-vc inherits the
> size of the active console, but it does not make sense for such a
> display.
>
> This series removes the shared "active" console state from ui/console.
> curses, cocoa, and vnc will hold the reference to the console currently
> shown with DisplayListener::con. This also eliminates the need to
> replace NULL with the active console and save code.
>
> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>

lgtm
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>

I am willing to take that for 9.0. Is there any bug already opened
about the issues it solves?

> ---
> Changes in v2:
> - Changed to fall back to a text console if there is no graphical
>   console as previously done.
> - Link to v1: https://lore.kernel.org/r/20240318-console-v1-0-f4efbfa71253@daynix.com
>
> ---
> Akihiko Odaki (4):
>       ui/vc: Do not inherit the size of active console
>       ui/vnc: Do not use console_select()
>       ui/cocoa: Do not use console_select()
>       ui/curses: Do not use console_select()
>
>  include/ui/console.h   |   2 +-
>  include/ui/kbd-state.h |  11 ++++
>  ui/console-priv.h      |   2 +-
>  ui/console-vc-stubs.c  |   2 +-
>  ui/console-vc.c        |   7 ++-
>  ui/console.c           | 133 ++++++++++++-------------------------------------
>  ui/curses.c            |  48 ++++++++++--------
>  ui/kbd-state.c         |   6 +++
>  ui/vnc.c               |  14 ++++--
>  ui/cocoa.m             |  37 ++++++++++----
>  10 files changed, 118 insertions(+), 144 deletions(-)
> ---
> base-commit: ba49d760eb04630e7b15f423ebecf6c871b8f77b
> change-id: 20240317-console-6744d4ab8ba6
>
> Best regards,
> --
> Akihiko Odaki <akihiko.odaki@daynix.com>
>
>


-- 
Marc-André Lureau
Re: [PATCH v2 0/4] ui/console: Remove console_select()
Posted by Akihiko Odaki 1 month, 1 week ago
On 2024/03/19 17:29, Marc-André Lureau wrote:
> Hi Akihiko
> 
> On Tue, Mar 19, 2024 at 7:09 AM Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
>>
>> ui/console has a concept of "active" console; the active console is used
>> when NULL is set for DisplayListener::con, and console_select() updates
>> the active console state. However, the global nature of the state cause
>> odd behaviors, and replacing NULL with the active console also resulted
>> in extra code. Remove it to solve these problems.
>>
>> The active console state is shared, so if there are two displays
>> referring to the active console, switching the console for one will also
>> affect the other. All displays that use the active console state,
>> namely cocoa, curses, and vnc, need to reset some of its state before
>> switching the console, and such a reset operation cannot be performed if
>> the console is switched by another display. This can result in stuck
>> keys, for example.
>>
>> While the active console state is shared, displays other than cocoa,
>> curses, and vnc don't update the state. A chardev-vc inherits the
>> size of the active console, but it does not make sense for such a
>> display.
>>
>> This series removes the shared "active" console state from ui/console.
>> curses, cocoa, and vnc will hold the reference to the console currently
>> shown with DisplayListener::con. This also eliminates the need to
>> replace NULL with the active console and save code.
>>
>> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
> 
> lgtm
> Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> 
> I am willing to take that for 9.0. Is there any bug already opened
> about the issues it solves?

No, I'm not aware of one.

Re: [PATCH v2 0/4] ui/console: Remove console_select()
Posted by Marc-André Lureau 1 month, 1 week ago
Hi

On Wed, Mar 20, 2024 at 7:20 AM Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
>
> On 2024/03/19 17:29, Marc-André Lureau wrote:
> > Hi Akihiko
> >
> > On Tue, Mar 19, 2024 at 7:09 AM Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
> >>
> >> ui/console has a concept of "active" console; the active console is used
> >> when NULL is set for DisplayListener::con, and console_select() updates
> >> the active console state. However, the global nature of the state cause
> >> odd behaviors, and replacing NULL with the active console also resulted
> >> in extra code. Remove it to solve these problems.
> >>
> >> The active console state is shared, so if there are two displays
> >> referring to the active console, switching the console for one will also
> >> affect the other. All displays that use the active console state,
> >> namely cocoa, curses, and vnc, need to reset some of its state before
> >> switching the console, and such a reset operation cannot be performed if
> >> the console is switched by another display. This can result in stuck
> >> keys, for example.
> >>
> >> While the active console state is shared, displays other than cocoa,
> >> curses, and vnc don't update the state. A chardev-vc inherits the
> >> size of the active console, but it does not make sense for such a
> >> display.
> >>
> >> This series removes the shared "active" console state from ui/console.
> >> curses, cocoa, and vnc will hold the reference to the console currently
> >> shown with DisplayListener::con. This also eliminates the need to
> >> replace NULL with the active console and save code.
> >>
> >> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
> >
> > lgtm
> > Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> >
> > I am willing to take that for 9.0. Is there any bug already opened
> > about the issues it solves?
>
> No, I'm not aware of one.

The first patch "Do not inherit the size of active console" is not
directly related and may not be suitable for merge during freeze. Are
you ok with merging the rest for 9.0 or delay it for 9.1?


-- 
Marc-André Lureau
Re: [PATCH v2 0/4] ui/console: Remove console_select()
Posted by Akihiko Odaki 1 month, 1 week ago
On 2024/03/20 16:00, Marc-André Lureau wrote:
> Hi
> 
> On Wed, Mar 20, 2024 at 7:20 AM Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
>>
>> On 2024/03/19 17:29, Marc-André Lureau wrote:
>>> Hi Akihiko
>>>
>>> On Tue, Mar 19, 2024 at 7:09 AM Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
>>>>
>>>> ui/console has a concept of "active" console; the active console is used
>>>> when NULL is set for DisplayListener::con, and console_select() updates
>>>> the active console state. However, the global nature of the state cause
>>>> odd behaviors, and replacing NULL with the active console also resulted
>>>> in extra code. Remove it to solve these problems.
>>>>
>>>> The active console state is shared, so if there are two displays
>>>> referring to the active console, switching the console for one will also
>>>> affect the other. All displays that use the active console state,
>>>> namely cocoa, curses, and vnc, need to reset some of its state before
>>>> switching the console, and such a reset operation cannot be performed if
>>>> the console is switched by another display. This can result in stuck
>>>> keys, for example.
>>>>
>>>> While the active console state is shared, displays other than cocoa,
>>>> curses, and vnc don't update the state. A chardev-vc inherits the
>>>> size of the active console, but it does not make sense for such a
>>>> display.
>>>>
>>>> This series removes the shared "active" console state from ui/console.
>>>> curses, cocoa, and vnc will hold the reference to the console currently
>>>> shown with DisplayListener::con. This also eliminates the need to
>>>> replace NULL with the active console and save code.
>>>>
>>>> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
>>>
>>> lgtm
>>> Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>>>
>>> I am willing to take that for 9.0. Is there any bug already opened
>>> about the issues it solves?
>>
>> No, I'm not aware of one.
> 
> The first patch "Do not inherit the size of active console" is not
> directly related and may not be suitable for merge during freeze. Are
> you ok with merging the rest for 9.0 or delay it for 9.1?

The first patch needs to be applied before the others.

It removes qemu_console_get_width()/qemu_console_get_height() calls with 
NULL as QemuConsole *con argument, which is intended to be replaced with 
the active console. The calls will make no sense after the other patches 
are applied since the concept of the active console will be gone.