[pve-devel] [RFC PATCH storage] pvesm export: convert dd's \r in status=progress output to \n

[Thomas Lamprecht]

Am 15.01.25 um 10:59 schrieb Dominik Csapak:
> pvesm export is mostly used for (remote) migrations, where the
> status progress output lands in a task log. For task logs we want to
> have line based output (since it's not a terminal), but dd uses \r
> to overwrite the same line which does not work in every situation, e.g.
> browsers sometimes simply don't show them, making the dd output a long
> line instead of separate ones.
> 
> To fix this, use run_command's `errfunc` to log the lines. run_command
> will split also on \r, but with warn we print a \n so this does the
> conversion.
> 
> This fixes an issue where the remote migration task log on PDM does not
> display that part in new lines. (ExtJS works because it does things
> differently and some browser quirks convert \r to \n)
> 
> Signed-off-by: Dominik Csapak <d.csapak at proxmox.com>
> ---
> Not sure if we want to take this approach because we lose the
> functionalty of overwriting progress on the terminal.

FWIW, you could test with `-t STDERR` if the std error FD is a terminal
and differ between replacing \r or not.

> 
> AFAICS there is no easy way to only do this for the task log, since
> we simply pipe the output fh of the worker task to the task log fh.
> 
> Alternatively we could patch the task log api to parse \r as newlines or
> patch the yew widget toolkit to replace \r with \n.

Wouldn't be one alternative also be to do that in the UI?

> 
> No real preference from my side, but this patch fixes the task log file
> without touching our central worker task code, so it seemed sensible.
> 
>  src/PVE/Storage/ISCSIPlugin.pm |  6 +++++-
>  src/PVE/Storage/LVMPlugin.pm   |  6 +++++-
>  src/PVE/Storage/Plugin.pm      | 10 ++++++++--
>  3 files changed, 18 insertions(+), 4 deletions(-)
> 
> diff --git a/src/PVE/Storage/ISCSIPlugin.pm b/src/PVE/Storage/ISCSIPlugin.pm
> index eb70453..33604cd 100644
> --- a/src/PVE/Storage/ISCSIPlugin.pm
> +++ b/src/PVE/Storage/ISCSIPlugin.pm
> @@ -631,7 +631,11 @@ sub volume_export {
>  	$size = int($1);
>      });
>      PVE::Storage::Plugin::write_common_header($fh, $size);
> -    run_command(['dd', "if=$file", "bs=64k", "status=progress"], output => '>&'.fileno($fh));
> +    run_command(['dd', "if=$file", "bs=64k", "status=progress"], output => '>&'.fileno($fh), errfunc => sub {
> +	# convert dd's \r to \n

I'd move the comment into the line re-printing the output, shorter
and also avoids having something before extracting the parameters,
which should always be first in perl for now; once we widely switch
to using signatures that wouldn't be a problem anymore.

Maybe also add some reasoning to the comment, like: "convert \r to \n to avoid issues in browsers"

> +	my ($line) = @_;
> +	warn "$line\n";
> +    });

Why not use `print STDERR "$line\n";`?

Because warn could be caught by a $SIG{__WARN__} handler from the call chain and
interpreted as problem. For relaying stderr messages to stderr again printing
directly to STDERR feels a bit more expressive and safer to me.

>      return;
>  }
>  
> diff --git a/src/PVE/Storage/LVMPlugin.pm b/src/PVE/Storage/LVMPlugin.pm
> index 38f7fa1..d41647b 100644
> --- a/src/PVE/Storage/LVMPlugin.pm
> +++ b/src/PVE/Storage/LVMPlugin.pm
> @@ -647,7 +647,11 @@ sub volume_export {
>  	$size = int($1);
>      });
>      PVE::Storage::Plugin::write_common_header($fh, $size);
> -    run_command(['dd', "if=$file", "bs=64k", "status=progress"], output => '>&'.fileno($fh));
> +    run_command(['dd', "if=$file", "bs=64k", "status=progress"], output => '>&'.fileno($fh), errfunc => sub {
> +	# convert dd's \r to \n
> +	my ($line) = @_;
> +	warn "$line\n";

same as above

> +    });
>  }
>  
>  sub volume_import_formats {
> diff --git a/src/PVE/Storage/Plugin.pm b/src/PVE/Storage/Plugin.pm
> index 65cf43f..c42a675 100644
> --- a/src/PVE/Storage/Plugin.pm
> +++ b/src/PVE/Storage/Plugin.pm
> @@ -1703,11 +1703,17 @@ sub volume_export {
>  	my $file_format = ($class->parse_volname($volname))[6];
>  	my $size = file_size_info($file, undef, $file_format);
>  
> +	# convert dd's \r to \n
> +	my $errfunc = sub {
> +	    my ($line) = @_;
> +	    warn "$line\n";

same here

> +	};
> +
>  	if ($format eq 'raw+size') {
>  	    die $err_msg if $with_snapshots || $file_format eq 'subvol';
>  	    write_common_header($fh, $size);
>  	    if ($file_format eq 'raw') {
> -		run_command(['dd', "if=$file", "bs=4k", "status=progress"], output => '>&'.fileno($fh));
> +		run_command(['dd', "if=$file", "bs=64k", "status=progress"], output => '>&'.fileno($fh), errfunc => $errfunc);
>  	    } else {
>  		run_command(['qemu-img', 'convert', '-f', $file_format, '-O', 'raw', $file, '/dev/stdout'],
>  		            output => '>&'.fileno($fh));
> @@ -1717,7 +1723,7 @@ sub volume_export {
>  	    my $data_format = $1;
>  	    die $err_msg if !$with_snapshots || $file_format ne $data_format;
>  	    write_common_header($fh, $size);
> -	    run_command(['dd', "if=$file", "bs=4k", "status=progress"], output => '>&'.fileno($fh));
> +	    run_command(['dd', "if=$file", "bs=64k", "status=progress"], output => '>&'.fileno($fh), errfunc => $errfunc);
>  	    return;
>  	} elsif ($format eq 'tar+size') {
>  	    die $err_msg if $file_format ne 'subvol';