Re: [pve-devel] [PATCH container 1/1] vnc: Allow custom timeout value in vncproxy method

[Matthias Heiserer <m.heiserer@proxmox.com>]

Hi,

Did you already submit a CLA? 
https://pve.proxmox.com/wiki/Developer_Documentation#Software_License_and_Copyright

I noticed two issues:

This patch is not for pve-container, but rather qemu-server, so the 
subject should include [PATCH qemu-server] rather than [PATCH container]

On 03.03.2023 12:16, Matthieu Malvache wrote:
> This commit adds support for a custom timeout value in the 'vncproxy'
> method of the Proxmox PVE REST API. The timeout can be specified using
> the 'timeout' parameter and defaults to 10 seconds if not set.
> 
> Signed-off-by: Matthieu Malvache <matthieu@ma2t.com>
> ---
>   PVE/API2/Qemu.pm | 10 ++++++++--
>   1 file changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/PVE/API2/Qemu.pm b/PVE/API2/Qemu.pm
> index 587bb22..4ca5842 100644
> --- a/PVE/API2/Qemu.pm
> +++ b/PVE/API2/Qemu.pm
> @@ -2164,6 +2164,13 @@ __PACKAGE__->register_method({
>   		default => 0,
>   		description => "Generates a random password to be used as ticket instead of the API ticket.",
>   	    },
> +	    timeout => {
> +		    optional => 1,
> +		    type => 'integer',
> +		    minimum => 5,
> +		    default => 10,
> +		    description => "Timeout in seconds for the vnc proxy connection.",
> +	    },
The indent here should be (\tab.... is a tab, \w.. are four spaces)
\tab....\w..timeout
\tab....\tab....optional => 1

so the difference in indents is always 4 spaces (1 tab = 8 spaces wide)
>   	},
>       },
>       returns => {
> @@ -2192,6 +2199,7 @@ __PACKAGE__->register_method({
>   	my $vmid = $param->{vmid};
>   	my $node = $param->{node};
>   	my $websocket = $param->{websocket};
> +	my $timeout = $param->{timeout};
>   
>   	my $conf = PVE::QemuConfig->load_config($vmid, $node); # check if VM exists
>   
> @@ -2226,8 +2234,6 @@ __PACKAGE__->register_method({
>   
>   	my $port = PVE::Tools::next_vnc_port($family);
>   
> -	my $timeout = 10;
> -
>   	my $realcmd = sub {
>   	    my $upid = shift;
>   
Other than that, I'd say it looks fine.