Filip Hejsek <filip.hej...@gmail.com> writes:
> On Mon, 2025-09-15 at 08:35 +0200, Markus Armbruster wrote:
>> Filip Hejsek <filip.hej...@gmail.com> writes:
>>
>> > On Fri, 2025-09-12 at 16:01 +0200, Markus Armbruster wrote:
>> > > Cc: libvirt
>> > >
>> > > Filip Hejsek <filip.hej...@gmail.com> writes:
>> > >
>> > > > From: Szymon Lukasz <noh4...@gmail.com>
>> > > >
>> > > > The managment software can use this command to notify QEMU about the
>> > > > size of the terminal connected to a chardev, QEMU can then forward this
>> > > > information to the guest if the chardev is connected to a virtio
>> > > > console
>> > > > device.
>> > > >
>> > > > Signed-off-by: Szymon Lukasz <noh4...@gmail.com>
>> > > > Suggested-by: Daniel P. Berrangé <berra...@redhat.com>
>> > > > Signed-off-by: Filip Hejsek <filip.hej...@gmail.com>
>> > > > ---
>> > > > chardev/char.c | 14 ++++++++++++++
>> > > > qapi/char.json | 22 ++++++++++++++++++++++
>> > > > 2 files changed, 36 insertions(+)
>> > > >
>> > > > diff --git a/chardev/char.c b/chardev/char.c
>> > > > index
>> > > > b45d79cb9b57643827eb7479257fdda2cf6b0434..6e3ade98614c949be8041ec5905a490ff536dee9
>> > > > 100644
>> > > > --- a/chardev/char.c
>> > > > +++ b/chardev/char.c
>> > > > @@ -1269,6 +1269,20 @@ bool qmp_add_client_char(int fd, bool
>> > > > has_skipauth, bool skipauth,
>> > > > return true;
>> > > > }
>> > > >
>> > > > +void qmp_chardev_resize(const char *id, uint16_t cols, uint16_t rows,
>> > > > + Error **errp)
>> > > > +{
>> > > > + Chardev *chr;
>> > > > +
>> > > > + chr = qemu_chr_find(id);
>> > > > + if (chr == NULL) {
>> > > > + error_setg(errp, "Chardev '%s' not found", id);
>> > > > + return;
>> > > > + }
>> > > > +
>> > > > + qemu_chr_resize(chr, cols, rows);
>> > > > +}
>> > > > +
>> > > > /*
>> > > > * Add a timeout callback for the chardev (in milliseconds), return
>> > > > * the GSource object created. Please use this to add timeout hook for
>> > > > diff --git a/qapi/char.json b/qapi/char.json
>> > > > index
>> > > > f0a53f742c8bee24c377551803a864fd36ac78cf..0a26c5eee6b71bc5de127a91b253cc69a9fe8ce6
>> > > > 100644
>> > > > --- a/qapi/char.json
>> > > > +++ b/qapi/char.json
>> > > > @@ -874,6 +874,28 @@
>> > > > { 'command': 'chardev-send-break',
>> > > > 'data': { 'id': 'str' } }
>> > > >
>> > > > +##
>> > > > +# @chardev-resize:
>> > >
>> > > This name doesn't tell me what is being resized. PATCH 04 uses
>> > > "winsize", which is better. The (losely) related SIGWINCH suggests
>> > > "window change" or "window size change". Below, you use "terminal
>> > > size".
>> >
>> > How about chardev-console-resize? That would match the name of the
>> > virtio event (VIRTIO_CONSOLE_RESIZE).
>>
>> Not bad. It could become slightly bad if we make devices other than
>> "consoles" make us of it. Would that be possible?
>
> I don't think the size has any meaning for devices that are not
> connected to a console, although the code does not care whether it
> actually is a console and simply has a size for every chardev.
Double-checking: the command works for any ChardevBackendKind, doesn't
it?
> I guess I could also rename it to chardev-window-resize
> or chardev-set-window-size. Let me know if you prefer one of these.
I think I'd prefer "window" or "terminal".
"resize" and "set size" suggest that the command initiates a size
change. Not true, it notifies of a size change. Maybe
"chardev-window-size-changed", "chardev-terminal-size-changed",
"chardev-window-resized", or "chardev-terminal-resized".
>> > > > +#
>> > > > +# Notifies a chardev about the current size of the terminal connected
>> > > > +# to this chardev.
>> > >
>> > > Yes, but what is it good for? Your commit message tells: "managment
>> > > software can use this command to notify QEMU about the size of the
>> > > terminal connected to a chardev, QEMU can then forward this information
>> > > to the guest if the chardev is connected to a virtio console device."
>> >
>> > How about:
>> >
>> > Notifies a chardev about the current size of the terminal connected
>> > to this chardev. The information will be forwarded to the guest if
>> > the chardev is connected to a virtio console device.
>>
>> Works for me.
>>
>> > > > +#
>> > > > +# @id: the chardev's ID, must exist
>> > > > +# @cols: the number of columns
>> > > > +# @rows: the number of rows
>> > >
>> > > Blank lines between the argument descriptions, bease.
>> > >
>> > > What's the initial size?
>> >
>> > 0x0
Another question... 'vc' chardevs accept optional @rows, @cols (see
ChardevVC). Is this the same size or something else?
>> A clearly invalid size. I guess it effectively means "unknown size".
>> Should we document that?
>
> Probably. 0x0 is I think also the default size in the Linux kernel, but
> I don't think the Linux kernel documents this.
How does 0 x 0 behave compared to a valid size like 80 x 24?
> Another question is if
> the 0x0 size should be propagated to the guest over virtio. I think it
> should be, although the virtio spec says nothing about 0x0 size.
>
> I'm not sure what is the right place to document this.
I think the QAPI schema doc comment is as good a place as any.
>> > > Do we need a way to query the size?
>> >
>> > I don't think it is necessary. What would be the usecase for that?
>>
>> I don't know, but it's my standard question when I see an interface to
>> set something without an interface to get it. Its purpose is to make us
>> think, not to make us at the get blindly.
>
> I guess it might be useful for debugging. If the size is not propagated
> correctly, one might query it to find out on which side the problem is.
We have query-chardev. It doesn't return much.
>> > > > +#
>> > > > +# Since: 10.2
>> > > > +#
>> > > > +# .. qmp-example::
>> > > > +#
>> > > > +# -> { "execute": "chardev-resize", "arguments": { "id": "foo",
>> > > > "cols": 80, "rows": 24 } }
>> > > > +# <- { "return": {} }
>> > > > +##
>> > > > +{ 'command': 'chardev-resize',
>> > > > + 'data': { 'id': 'str',
>> > > > + 'cols': 'uint16',
>> > > > + 'rows': 'uint16' } }
>> > > > +
>> > > > ##
>> > > > # @VSERPORT_CHANGE:
>> > > > #
