[pve-devel] [PATCH container 0/1] Add support for custom timeout in vncproxy method

[pve-devel] [PATCH container 0/1] Add support for custom timeout in vncproxy method

[Matthieu Malvache]

Dear Maintainer,

This patch 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.

The purpose of this change is to allow users to configure the timeout
duration for the VNC proxy connection to better suit their needs. By
default, the timeout is set to 10 seconds, which should work well for
most use cases. However, some users may need to increase or decrease
the timeout based on factors such as network latency.

Thank you for considering this patch.

Sincerely,

Matthieu Malvache

Matthieu Malvache (1):
  vnc: Allow custom timeout value in vncproxy method

 PVE/API2/Qemu.pm | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

-- 
2.30.2

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

[Matthieu Malvache]

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.",
+	    },
 	},
     },
     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;
 
-- 
2.30.2

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

[Matthias Heiserer]

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.

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

[Matthieu Malvache]

Hi Matthias,

Thank you for your email. I sent the CLA earlier this afternoon. 

This is actually the first time I'm contributing to a project like this, so thank you for your guidance 
and pointing out the issues with the patch.

Regarding the tab issue, should I submit a fix for this separately, or can I include it in the same patch?

Also, could you please let me know how I can "rename" the subject from [PATCH container] to [PATCH qemu-server]?

Thank you again for your help.

Matthieu Malvache
Ma2t.
matthieu@ma2t.com


> Le 3 mars 2023 à 13:06, Matthias Heiserer <m.heiserer@proxmox.com> a écrit :
> 
> 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.
> 

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

[Matthias Heiserer]

On 03.03.2023 14:43, Matthieu Malvache wrote:
> Hi Matthias,
> 
> Thank you for your email. I sent the CLA earlier this afternoon.
> 
> This is actually the first time I'm contributing to a project like this, 
> so thank you for your guidance
> and pointing out the issues with the patch.
> 
> Regarding the tab issue, should I submit a fix for this separately, or 
> can I include it in the same patch?
Please send a new patch, mark it as "v2" (i.e. [PATCH v2 qemu-server] , 
and include the change there. You should also briefly describe the 
changes you made, the wiki describes this, or you can take a look at 
other patches in the mailing list.
> 
> Also, could you please let me know how I can "rename" the subject from 
> [PATCH container] to [PATCH qemu-server]?
Our wiki gives the command "git config --local format.subjectprefix 
'PATCH container'"
But that only works for the container repos. For the qemu-server repo, it's
"git config --local format.subjectprefix 'PATCH qemu-server'"

> 
> Thank you again for your help.
> 
> Matthieu Malvache
> Ma2t.
> matthieu@ma2t.com <mailto:matthieu@ma2t.com>
> 
> 
>> Le 3 mars 2023 à 13:06, Matthias Heiserer <m.heiserer@proxmox.com 
>> <mailto:m.heiserer@proxmox.com>> a écrit :
>>
>> Hi,
>>
>> Did you already submit a CLA? 
>> https://pve.proxmox.com/wiki/Developer_Documentation#Software_License_and_Copyright <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 
>>> <mailto: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.
>>
> 

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

[Dominik Csapak]

one additional comment inline:

On 3/3/23 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.",
> +	    },
>   	},
>       },
>       returns => {
> @@ -2192,6 +2199,7 @@ __PACKAGE__->register_method({
>   	my $vmid = $param->{vmid};
>   	my $node = $param->{node};
>   	my $websocket = $param->{websocket};
> +	my $timeout = $param->{timeout};

that has actually a bug ;)

the 'default' option in the parameter definition is not set anywhere, it's just
used for the documentation generation


to actually make it use the default of 10 you should do:

my $timeout = $param->{timeout} // 10;


this way it is set to 10 when the parameter is not set

>   
>   	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;
>