RE: [PATCH v3 2/2] Implement SSH commands in QEMU GA for Windows

[Aidan Leuck]

-----Original Message-----
From: Philippe Mathieu-Daudé <philmd@linaro.org> 
Sent: Wednesday, March 27, 2024 9:38 AM
To: Aidan Leuck <aidan_leuck@selinc.com>; qemu-devel@nongnu.org; Markus 
Armbruster <armbru@redhat.com>
Cc: kkostiuk@redhat.com; berrange@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é <philmd@linaro.org>
> Sent: Monday, March 25, 2024 11:51 AM
> To: Aidan Leuck <aidan_leuck@selinc.com>; qemu-devel@nongnu.org
> Cc: kkostiuk@redhat.com; berrange@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_leuck@selinc.com wrote:
>> From: Aidan Leuck <aidan_leuck@selinc.com>
>>
>> Signed-off-by: Aidan Leuck <aidan_leuck@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.