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

[Dominik Csapak]

On 1/15/25 12:28, Thomas Lamprecht wrote:
> 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.
> 

not sure if that would work here since we do quite some redirection for
the worker task (to be able to display + putting it in the task log at
the same time), but yes, I'll try that

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

thats what i meant in that sentence. (replacing in yew widget toolkit)

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

sure, makes sense

> 
>> +	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.
> 

OK

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