[pve-devel] [PATCH v2 storage] fix #1427: Set file mode on uploaded templates/ISOs

[Thomas Lamprecht]

On 6/13/19 11:33 AM, Stefan Reiter wrote:
> Signed-off-by: Stefan Reiter <s.reiter at proxmox.com>
> ---
> 
> Much simpler than v1, simply chmods the temp file before copying.
> Interestingly, on my machine anyway, scp seems to preserve file modes
> without the -p flag too, but I added it in for consistency.
> 

applied, thanks!
FYI, I moved some of above info to the actual commit message

> wget (and thus apl_download) creates a new file, and follows the umask.
> The tempfile we copy here is created in AnyEvent.pm (line 1288), where
> it is handled as an unspecified REST upload and manually has its file
> mode set:

OK, makes sense, thanks for looking into this.

> 
>> ...
>> my $outfh = IO::File->new($tmpfilename, O_RDWR|O_CREAT|O_EXCL, 0600)
>> ...
> 
>  PVE/API2/Storage/Status.pm | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/PVE/API2/Storage/Status.pm b/PVE/API2/Storage/Status.pm
> index 9a5a952..a56a02b 100644
> --- a/PVE/API2/Storage/Status.pm
> +++ b/PVE/API2/Storage/Status.pm
> @@ -369,7 +369,7 @@ __PACKAGE__->register_method ({
>  		type => 'string',
>  	    },
>  	    tmpfilename => { 
> -		description => "The source file name. This parameter is usually set by the REST handler. You can only overwrite it when connecting to the trustet port on localhost.",
> +		description => "The source file name. This parameter is usually set by the REST handler. You can only overwrite it when connecting to the trusted port on localhost.",
>  		type => 'string',
>  		optional => 1,
>  	    },
> @@ -427,7 +427,10 @@ __PACKAGE__->register_method ({
>  	my $dest = "$path/$filename";
>  	my $dirname = dirname($dest);
>  
> -	# we simply overwrite when destination when file already exists
> +	# best effort to match apl_download behaviour
> +	chmod 0644, $tmpfilename;
> +
> +	# we simply overwrite when destination file already exists

while the new comment here is definitively more correct than the
old one I changed it to something (at least for me :) easier to
understand in a followup, i.e.:

# we simply overwrite the destination file if it already exists

>  
>  	my $cmd;
>  	if ($node ne 'localhost' && $node ne PVE::INotify::nodename()) {
> @@ -447,7 +450,7 @@ __PACKAGE__->register_method ({
>  	    PVE::Tools::run_command([@remcmd, '/bin/mkdir', '-p', '--', PVE::Tools::shell_quote($dirname)],
>  				    errmsg => "mkdir failed");
>   
> -	    $cmd = ['/usr/bin/scp', @ssh_options, '--', $tmpfilename, "[$remip]:" . PVE::Tools::shell_quote($dest)];
> +	    $cmd = ['/usr/bin/scp', @ssh_options, '-p', '--', $tmpfilename, "[$remip]:" . PVE::Tools::shell_quote($dest)];
>  	} else {
>  	    PVE::Storage::activate_storage($cfg, $param->{storage});
>  	    File::Path::make_path($dirname);
>