first, thanks for the patch, comments inline.
On 6/12/19 4:52 PM, Stefan Reiter wrote:
> Signed-off-by: Stefan Reiter <s.reiter at proxmox.com>
> ---
> PVE/API2/Storage/Status.pm | 25 +++++++++++++++++++++----
> 1 file changed, 21 insertions(+), 4 deletions(-)
>
> diff --git a/PVE/API2/Storage/Status.pm b/PVE/API2/Storage/Status.pm
> index 9a5a952..8649a7d 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,10 +427,15 @@ __PACKAGE__->register_method ({
> my $dest = "$path/$filename";
> my $dirname = dirname($dest);
>
> - # we simply overwrite when destination when file already exists
> + # we simply overwrite when destination file already exists
> +
> + # default mode for $dest is 600, templates have default 644
> + # let's keep them in sync
> + my $mode_cmd = ['chmod', 'a+r', '--', PVE::Tools::shell_quote($dest)];
>
> my $cmd;
> if ($node ne 'localhost' && $node ne PVE::INotify::nodename()) {
> + # remote node, execute commands over ssh
nit-pick: comment is not really necessary, i.e., what vs. why comment
and has not much to do with the patch here.
> my $remip = PVE::Cluster::remote_node_ip($node);
>
> my @ssh_options = ('-o', 'BatchMode=yes');
> @@ -446,8 +451,9 @@ __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)];
> + $mode_cmd = [@remcmd, @$mode_cmd];
why not just pass "-p" to scp (maybe we want to use rsync anyway, but if,
that's best left for another patch to do), then it tries to preserve some
metadata like mtime/atime and modes, so we probably just would need to ensure
that $tmpfile is readable and we'd be good to go.. For this, one could use
perl's builtin "chmod"[0], so we save a additional SSH connection (always good,
their not really cheap) and a run_command in the non-proxied part..
[0]: https://perldoc.perl.org/functions/chmod.html
also, the 'apl_download' call in pve-manager Nodes API perl module
does no special chmod or the like, AFAIS, could you take a look what
happens there, i.e., why's the permission already OK there? Does
wget downloads it to a world-readable file already? (this is more
out of interests sake)
> } else {
> PVE::Storage::activate_storage($cfg, $param->{storage});
> File::Path::make_path($dirname);
> @@ -456,7 +462,7 @@ __PACKAGE__->register_method ({
>
> my $worker = sub {
> my $upid = shift;
> -
> +
> print "starting file import from: $tmpfilename\n";
> print "target node: $node\n";
> print "target file: $dest\n";
> @@ -468,6 +474,17 @@ __PACKAGE__->register_method ({
> unlink $dest;
> die $err;
> }
> +
> + print "set file mode: " . join(' ', @$mode_cmd) . "\n";
> +
> + # set file mode
> + eval { PVE::Tools::run_command($mode_cmd, errmsg => 'chmod error'); };
> + if (my $err = $@) {
> + # pve works with the default mode too, so this error has
> + # "cosmetic" consequences only - hence we warn, not die
my motto: conciser comments are better comments, they tend to less clutter
the code and be read more often :-)
maybe something like:
# best effort, to match apl_download call from manager, only
(just as idea)
> + warn $err;
> + }
> +
> print "finished file import successfully\n";
> };
>
>