Re: [PATCH 1/2] ui: Allow injection of vnc display name

[Andrew Keesler <ankeesler@google.com>]

[-- Attachment #1: Type: text/plain, Size: 9723 bytes --]

Hi - gentle ping on my last email. Would love to hear any thoughts about
moving
this forward.

Thank you for your thoughts thus far.

On Mon, Oct 28, 2024 at 3:25 PM Andrew Keesler <ankeesler@google.com> wrote:

> Hi Daniel and Marc-André - please excuse my delay (I was traveling
> last week).
>
> I see 2 primary takeaways here:
>
> 1. Updating the name field from the ServerInit RFB message to be
>    controlled by the 'name' VNC option
>
> 2. Updating where we store the display 'name' in memory
>
> For 1 - are we amenable to me updating this display name field to be
> controlled by 'name' in a future patch? I'm still learning what the
> QEMU community prefers with respect to patch sizes, but in general I
> have been trying to keep the patches small.
>
> For 2 - have we landed on storing the per-display 'name' in
> DisplayOptions? I can't quite tell if we've converged here. It also
> seems like there is some more plumbing to install here before we
> leverage DisplayOptions, so I am curious what the recommendation is
> for this patch - is there a temporary solution we could pursue and do
> refactors later to unlock the CLI functionality that has been brought
> up?
>
> Correct me if I am wrong about any of this.
>
> On Tue, Oct 22, 2024 at 4:36 AM Marc-André Lureau <
> marcandre.lureau@gmail.com> wrote:
>
>> Hi
>>
>> On Tue, Oct 22, 2024 at 12:10 PM Daniel P. Berrangé <berrange@redhat.com>
>> wrote:
>>
>>> On Tue, Oct 22, 2024 at 12:04:29PM +0400, Marc-André Lureau wrote:
>>> > Hi
>>> >
>>> > On Tue, Oct 22, 2024 at 11:54 AM Daniel P. Berrangé <
>>> berrange@redhat.com>
>>> > wrote:
>>> >
>>> > > On Mon, Oct 21, 2024 at 03:14:39PM +0400, Marc-André Lureau wrote:
>>> > > > Hi Roque
>>> > > >
>>> > > > On Fri, Oct 18, 2024 at 1:53 AM Roque Arcudia Hernandez
>>> > > > <roqueh@google.com> wrote:
>>> > > > >
>>> > > > > From: Andrew Keesler <ankeesler@google.com>
>>> > > > >
>>> > > > > Thanks to 72d277a7, 1ed2cb32, and others, EDID (Extended Display
>>> > > Identification
>>> > > > > Data) is propagated by QEMU such that a virtual display presents
>>> > > legitimate
>>> > > > > metadata (e.g., name, serial number, preferred resolutions,
>>> etc.) to
>>> > > its
>>> > > > > connected guest.
>>> > > > >
>>> > > > > This change propagates an optional user-provided display name to
>>> > > > > QemuConsole. Future changes will update downstream devices to
>>> leverage
>>> > > this
>>> > > > > display name for various uses, the primary one being providing a
>>> > > custom EDID
>>> > > > > name to guests. Future changes will also update other displays
>>> (e.g.,
>>> > > spice)
>>> > > > > with a similar option to propagate a display name to downstream
>>> > > devices.
>>> > > > >
>>> > > > > Currently, every virtio-gpu virtual display has the same name:
>>> "QEMU
>>> > > > > Monitor". We hope to be able to inject the EDID name of virtual
>>> > > displays in
>>> > > > > order to test guest behavior that is specific to display names.
>>> We
>>> > > provide the
>>> > > > > ability to inject the display name from the display
>>> configuration as
>>> > > that most
>>> > > > > closely resembles how real displays work (hardware displays
>>> contain
>>> > > static EDID
>>> > > > > information that is provided to every connected host).
>>> > > > >
>>> > > > > It should also be noted that EDID names longer than 12 bytes
>>> will be
>>> > > truncated
>>> > > > > per spec (I think?).
>>> > > > >
>>> > > > > Signed-off-by: Andrew Keesler <ankeesler@google.com>
>>> > > > > Signed-off-by: Roque Arcudia Hernandez <roqueh@google.com>
>>> > > > > ---
>>> > > > >  include/ui/console.h | 1 +
>>> > > > >  ui/console-priv.h    | 1 +
>>> > > > >  ui/console.c         | 8 ++++++++
>>> > > > >  ui/vnc.c             | 8 +++++++-
>>> > > > >  4 files changed, 17 insertions(+), 1 deletion(-)
>>> > > > >
>>> > > > > diff --git a/include/ui/console.h b/include/ui/console.h
>>> > > > > index 5832d52a8a..74ab03ed72 100644
>>> > > > > --- a/include/ui/console.h
>>> > > > > +++ b/include/ui/console.h
>>> > > > > @@ -408,6 +408,7 @@ int qemu_console_get_index(QemuConsole *con);
>>> > > > >  uint32_t qemu_console_get_head(QemuConsole *con);
>>> > > > >  int qemu_console_get_width(QemuConsole *con, int fallback);
>>> > > > >  int qemu_console_get_height(QemuConsole *con, int fallback);
>>> > > > > +void qemu_console_set_name(QemuConsole *con, const char *name);
>>> > > > >  /* Return the low-level window id for the console */
>>> > > > >  int qemu_console_get_window_id(QemuConsole *con);
>>> > > > >  /* Set the low-level window id for the console */
>>> > > > > diff --git a/ui/console-priv.h b/ui/console-priv.h
>>> > > > > index 43ceb8122f..9f2769843f 100644
>>> > > > > --- a/ui/console-priv.h
>>> > > > > +++ b/ui/console-priv.h
>>> > > > > @@ -18,6 +18,7 @@ struct QemuConsole {
>>> > > > >      Object parent;
>>> > > > >
>>> > > > >      int index;
>>> > > > > +    const char *name;
>>> > > > >      DisplayState *ds;
>>> > > > >      DisplaySurface *surface;
>>> > > > >      DisplayScanout scanout;
>>> > > > > diff --git a/ui/console.c b/ui/console.c
>>> > > > > index 5165f17125..f377fd8417 100644
>>> > > > > --- a/ui/console.c
>>> > > > > +++ b/ui/console.c
>>> > > > > @@ -1452,6 +1452,14 @@ int qemu_console_get_height(QemuConsole
>>> *con,
>>> > > int fallback)
>>> > > > >      }
>>> > > > >  }
>>> > > > >
>>> > > > > +void qemu_console_set_name(QemuConsole *con, const char *name)
>>> > > > > +{
>>> > > > > +    if (con == NULL) {
>>> > > > > +        return;
>>> > > > > +    }
>>> > > > > +    con->name = name;
>>> > > > > +}
>>> > > > > +
>>> > > > >  int qemu_invalidate_text_consoles(void)
>>> > > > >  {
>>> > > > >      QemuConsole *s;
>>> > > > > diff --git a/ui/vnc.c b/ui/vnc.c
>>> > > > > index 93a8dbd253..7d6acc5c2e 100644
>>> > > > > --- a/ui/vnc.c
>>> > > > > +++ b/ui/vnc.c
>>> > > > > @@ -3595,6 +3595,9 @@ static QemuOptsList qemu_vnc_opts = {
>>> > > > >          },{
>>> > > > >              .name = "power-control",
>>> > > > >              .type = QEMU_OPT_BOOL,
>>> > > > > +        },{
>>> > > > > +            .name = "name",
>>> > > > > +            .type = QEMU_OPT_STRING,
>>> > > > >          },
>>> > > > >          { /* end of list */ }
>>> > > > >      },
>>> > > > > @@ -4016,7 +4019,7 @@ void vnc_display_open(const char *id, Error
>>> > > **errp)
>>> > > > >      QemuOpts *opts = qemu_opts_find(&qemu_vnc_opts, id);
>>> > > > >      g_autoptr(SocketAddressList) saddr_list = NULL;
>>> > > > >      g_autoptr(SocketAddressList) wsaddr_list = NULL;
>>> > > > > -    const char *share, *device_id;
>>> > > > > +    const char *share, *device_id, *name;
>>> > > > >      QemuConsole *con;
>>> > > > >      bool password = false;
>>> > > > >      bool reverse = false;
>>> > > > > @@ -4217,6 +4220,9 @@ void vnc_display_open(const char *id, Error
>>> > > **errp)
>>> > > > >      }
>>> > > > >      qkbd_state_set_delay(vd->kbd, key_delay_ms);
>>> > > > >
>>> > > > > +    name = qemu_opt_get(opts, "name");
>>> > > > > +    qemu_console_set_name(vd->dcl.con, name);
>>> > > >
>>> > > > Why not expose a "head_name" property in QemuGraphicConsole?
>>> > >
>>> > > QemuGraphicConsole isn't mapped to any CLI though, is it ?
>>> > >
>>> > >
>>> > No, it would be a bit tedious to do so with multi-head -devices.
>>> >
>>> >
>>> > > In QAPI we have DisplayOptions union  for all the local displays,
>>> > > and as a user I think I'd expect 'name' to be settable from
>>> > > those.
>>> > >
>>> > >
>>> > DisplayOptions is meant for the UI display.. Here, the intent is
>>> really to
>>> > set the HW EDID name field.
>>>
>>> But it is also applicable to the backend, all of which have a
>>> name for the display set in the window titlebar. We should
>>> be looking at both sides IMHO.
>>>
>>
>> Ok, if we consider both should be treated similarly / reflect each other.
>>
>>
>>>
>>> > Also DisplayOptions doesn't map to a specific console.
>>>
>>> It could be made to contain per-head information if we desired
>>> though, and would be more useful than just the name. There were
>>> some patches a while ago trying to express per-console placement
>>> of windows onto host monitor outputs, for example.
>>>
>>
>> [RFC PATCH v2 0/2] ui/gtk: Introduce new param - Connectors
>> https://patchew.org/QEMU/20240531185804.119557-1-dongwon.kim@intel.com/
>>
>>>
>>> > > own CLI options we can expose.
>>> > >
>>> > > For runtime setting, we have a QMP  "display-update" command, that
>>> > > currently just lets you change VNC listening address, but was
>>> intended
>>> > > to allow for any runtime display changes.
>>> > >
>>> > > > This way it should be possible to set the name with QMP qom-set.
>>> > >
>>> > > qom-set isn't a particularly nice interface, as things you can set
>>> > > from that are not introspectable and have no type information that
>>> > > can be queried.
>>> > >
>>> >
>>> > fwiw, it could be easily exposed to D-Bus, for ex:
>>> >
>>> > busctl --user set-property org.qemu /org/qemu/Display1/Console_1
>>> > org.qemu.Display1.Console HeadName s "First Monitor"
>>>
>>> That could be mapped to whatever interface we expose on the QEMU side,
>>> it doesn't have to be qom-set.
>>>
>>
>> It seems to me the main problem is that consoles are dynamically created
>> by devices, and it's hard for the ui/display to map options to a specific
>> console.
>>
>> The other issue is handling arrays with CLI in general...
>>
>> --
>> Marc-André Lureau
>>
>

[-- Attachment #2: Type: text/html, Size: 13368 bytes --]