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

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

[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@proxmox.com>
---
Not sure if we want to take this approach because we lose the
functionalty of overwriting progress on the terminal.

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.

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
+	my ($line) = @_;
+	warn "$line\n";
+    });
     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";
+    });
 }
 
 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";
+	};
+
 	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';
-- 
2.39.5



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

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

[Dominik Csapak]

oops, i accidentally committed a change from 4k -> 64k in the bs parameter that i added for testing

for comments, please disregard this change, this was unrelated and only for me to test
if we could get more performance out of the export (did not do too much testing there
yet)

On 1/15/25 10:59, Dominik Csapak wrote:
> 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@proxmox.com>
> ---
> Not sure if we want to take this approach because we lose the
> functionalty of overwriting progress on the terminal.
> 
> 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.
> 
> 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
> +	my ($line) = @_;
> +	warn "$line\n";
> +    });
>       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";
> +    });
>   }
>   
>   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";
> +	};
> +
>   	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';



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

Re: [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@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';



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

Re: [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@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';
> 



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

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

[Fabian Grünbichler]

On January 16, 2025 9:13 am, Dominik Csapak wrote:
> 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@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)

one issue with that though would be that the line (in the task log/API
response) can get really long? not sure whether/where that might cause
problems..


_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

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

[Thomas Lamprecht]

Am 16.01.25 um 09:13 schrieb Dominik Csapak:
>> 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)

Ah OK, seems I sorta skipped reading after the first half of the
sentence, sorry.

Well, I see some merit in having this done at backend side, having some
consistency and avoiding that other API consumer need to to such
transformations, but it then might be worth thinking about doing it more
generally.

I shortly looked into that (fork_worker and run_command) and I initiially
got slightly confused, as in run_command we already do
`print STDERR "$laststderr\n" if $laststderr;` for the case there is no
errfunc defined, with $laststderr being a single line without any limiter
(\r and/or \n or \b, fwiw), but that's only called if there is a errmsg
parameter; bleh, run_command should really be reworked (or die).

So FWIW, this could be also "fixed" by passing something like
errmsg => "failed to write file '$file'" ... <.<

Another completely different option, skip DD and do the write ourself,
might be even be done relatively easily with the sendfile syscall in
a loop of 64K blocks and some output every few seconds, i.e. not
totally trivial, but also not _that_ hard, just mentioning for sake
of completeness, even if we want to go in that direction we can employ
transforming the \r to \n as a stop gap.


_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

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

[Dominik Csapak]

On 1/16/25 10:21, Thomas Lamprecht wrote:
> Am 16.01.25 um 09:13 schrieb Dominik Csapak:
>>> 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)
> 
> Ah OK, seems I sorta skipped reading after the first half of the
> sentence, sorry.
> 
> Well, I see some merit in having this done at backend side, having some
> consistency and avoiding that other API consumer need to to such
> transformations, but it then might be worth thinking about doing it more
> generally.

I agree with it being done on the backend side, otherwise the
output lands in the tasklog and the api as a single line which is imho confusing.

> 
> I shortly looked into that (fork_worker and run_command) and I initiially
> got slightly confused, as in run_command we already do
> `print STDERR "$laststderr\n" if $laststderr;` for the case there is no
> errfunc defined, with $laststderr being a single line without any limiter
> (\r and/or \n or \b, fwiw), but that's only called if there is a errmsg
> parameter; bleh, run_command should really be reworked (or die).
> 
> So FWIW, this could be also "fixed" by passing something like
> errmsg => "failed to write file '$file'" ... <.<

I find it weird behaviour of run_command do do that kind of output
transformation with a more or less "unrelated" option?
Shouldn't that transformation happen always or never? (or with
an explicit option?)

Anyway would you prefer this approach to the manual errfunc?
Would still warrant a comment IMHO since one might optimize
the option away in the future without noticing it's there for
the transformation...

> 
> Another completely different option, skip DD and do the write ourself,
> might be even be done relatively easily with the sendfile syscall in
> a loop of 64K blocks and some output every few seconds, i.e. not
> totally trivial, but also not _that_ hard, just mentioning for sake
> of completeness, even if we want to go in that direction we can employ
> transforming the \r to \n as a stop gap.

Also thought about something like this, Advantage would be that we're in control
of the output and could more easily integrate that into a (potential future) feature
where tasks can have meta info like percentage done/stages/etc.

I'd still probably do the transformation fix now (one way or another) and
look into how we can maybe more generalize such "streaming" commands away,
that maybe also does format conversion (instead of qemu-img) and other exports
(rbd/zfs/lvm?) as well? (Writing this, I'm noticing this easily becomes
a much larger change/feature than what I was trying to fix though :P)

I'm happy to do any of these, just let me know what you'd prefer.


_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel