-----Original Message----- From: Philippe Mathieu-Daudé <phi...@linaro.org> Sent: Wednesday, March 27, 2024 9:38 AM To: Aidan Leuck <aidan_le...@selinc.com>; qemu-devel@nongnu.org; Markus Armbruster <arm...@redhat.com> Cc: kkost...@redhat.com; berra...@redhat.com Subject: Re: [PATCH v3 2/2] Implement SSH commands in QEMU GA for Windows
[Caution - External] On 27/3/24 15:38, Aidan Leuck wrote: > Hi Philippe, > Thank you for your feedback I will get these issues addressed. I left one > small comment on the QAPI schema JSON. > > Aidan Leuck > > -----Original Message----- > From: Philippe Mathieu-Daudé <phi...@linaro.org> > Sent: Monday, March 25, 2024 11:51 AM > To: Aidan Leuck <aidan_le...@selinc.com>; qemu-devel@nongnu.org > Cc: kkost...@redhat.com; berra...@redhat.com > Subject: Re: [PATCH v3 2/2] Implement SSH commands in QEMU GA for > Windows > > [Caution - External] > > On 22/3/24 18:46, aidan_le...@selinc.com wrote: >> From: Aidan Leuck <aidan_le...@selinc.com> >> >> Signed-off-by: Aidan Leuck <aidan_le...@selinc.com> >> --- >> qga/commands-windows-ssh.c | 791 >> +++++++++++++++++++++++++++++++++++++ > > Huge file, I'm skipping it. > >> qga/commands-windows-ssh.h | 26 ++ >> qga/meson.build | 5 +- >> qga/qapi-schema.json | 17 +- >> 4 files changed, 828 insertions(+), 11 deletions(-) >> create mode 100644 qga/commands-windows-ssh.c >> create mode 100644 qga/commands-windows-ssh.h > > >> diff --git a/qga/qapi-schema.json b/qga/qapi-schema.json index >> 9554b566a7..a64a6d91cf 100644 >> --- a/qga/qapi-schema.json >> +++ b/qga/qapi-schema.json >> @@ -1562,9 +1562,8 @@ >> { 'struct': 'GuestAuthorizedKeys', >> 'data': { >> 'keys': ['str'] >> - }, >> - 'if': 'CONFIG_POSIX' } >> - > > For Windows you have to check the CONFIG_WIN32 definition, so you want: > > I don't think this is necessary, the QEMU guest agent is compiled for only > POSIX and Windows. I don't see this pattern being used elsewhere in the qapi > schema file. I would be interested in what the maintainers think? $ git grep -w CONFIG_WIN32 qapi/ qapi/char.json:490: { 'name': 'console', 'if': 'CONFIG_WIN32' }, qapi/char.json:663: 'if': 'CONFIG_WIN32' }, qapi/misc.json:293:{ 'command': 'get-win32-socket', 'data': {'info': 'str', 'fdname': 'str'}, 'if': 'CONFIG_WIN32' } > > 'if': { 'any': [ 'CONFIG_POSIX', > 'CONFIG_WIN32' ] }, > >> + } >> +} >> >> ## >> # @guest-ssh-get-authorized-keys: >> @@ -1580,8 +1579,8 @@ >> ## >> { 'command': 'guest-ssh-get-authorized-keys', >> 'data': { 'username': 'str' }, >> - 'returns': 'GuestAuthorizedKeys', >> - 'if': 'CONFIG_POSIX' } >> + 'returns': 'GuestAuthorizedKeys' >> +} >> >> ## >> # @guest-ssh-add-authorized-keys: >> @@ -1599,8 +1598,8 @@ >> # Since: 5.2 >> ## >> { 'command': 'guest-ssh-add-authorized-keys', >> - 'data': { 'username': 'str', 'keys': ['str'], '*reset': 'bool' }, >> - 'if': 'CONFIG_POSIX' } >> + 'data': { 'username': 'str', 'keys': ['str'], '*reset': 'bool' } } >> >> ## >> # @guest-ssh-remove-authorized-keys: >> @@ -1617,8 +1616,8 @@ >> # Since: 5.2 >> ## >> { 'command': 'guest-ssh-remove-authorized-keys', >> - 'data': { 'username': 'str', 'keys': ['str'] }, >> - 'if': 'CONFIG_POSIX' } >> + 'data': { 'username': 'str', 'keys': ['str'] } } >> >> ## >> # @GuestDiskStats: > Hi Philippe, thank you for getting back to me so quickly. Looking at the grep you gave me seems to confirm what I was saying if I am not mistaken? It looks like CONFIG_WIN32 and CONFIG_POSIX if conditionals are only used when you need to enable a command on one operating system and not the other. I do believe that your code snippet is correct it is just overly verbose. The QGA has both windows and SSH implementations and looking at the guest agent QAPI file when a command supports both POSIX and Windows the if gate is removed. I am happy to discuss this further if you have more concerns.