From mboxrd@z Thu Jan 1 00:00:00 1970
Return-Path: <pve-devel-bounces@lists.proxmox.com>
Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68])
by lore.proxmox.com (Postfix) with ESMTPS id E4B761FF16F
for <inbox@lore.proxmox.com>; Thu, 16 Jan 2025 09:13:41 +0100 (CET)
Received: from firstgate.proxmox.com (localhost [127.0.0.1])
by firstgate.proxmox.com (Proxmox) with ESMTP id 2FFB819FE3;
Thu, 16 Jan 2025 09:13:39 +0100 (CET)
Message-ID: <27ec7af2-cfa8-4202-9137-eb9182b80e52@proxmox.com>
Date: Thu, 16 Jan 2025 09:13:05 +0100
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird Beta
To: Thomas Lamprecht <t.lamprecht@proxmox.com>,
Proxmox VE development discussion <pve-devel@lists.proxmox.com>
References: <20250115095901.809989-1-d.csapak@proxmox.com>
<4bf6dd93-1274-4c25-8bf8-5b1ba37d2f18@proxmox.com>
Content-Language: en-US
From: Dominik Csapak <d.csapak@proxmox.com>
In-Reply-To: <4bf6dd93-1274-4c25-8bf8-5b1ba37d2f18@proxmox.com>
X-SPAM-LEVEL: Spam detection results: 0
AWL 0.014 Adjusted score from AWL reputation of From: address
BAYES_00 -1.9 Bayes spam probability is 0 to 1%
DMARC_MISSING 0.1 Missing DMARC policy
KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment
RCVD_IN_VALIDITY_CERTIFIED_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to
Validity was blocked. See
https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more
information.
RCVD_IN_VALIDITY_RPBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to
Validity was blocked. See
https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more
information.
RCVD_IN_VALIDITY_SAFE_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to
Validity was blocked. See
https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more
information.
SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record
SPF_PASS -0.001 SPF: sender matches SPF record
Subject: Re: [pve-devel] [RFC PATCH storage] pvesm export: convert dd's \r
in status=progress output to \n
X-BeenThere: pve-devel@lists.proxmox.com
X-Mailman-Version: 2.1.29
Precedence: list
List-Id: Proxmox VE development discussion <pve-devel.lists.proxmox.com>
List-Unsubscribe: <https://lists.proxmox.com/cgi-bin/mailman/options/pve-devel>,
<mailto:pve-devel-request@lists.proxmox.com?subject=unsubscribe>
List-Archive: <http://lists.proxmox.com/pipermail/pve-devel/>
List-Post: <mailto:pve-devel@lists.proxmox.com>
List-Help: <mailto:pve-devel-request@lists.proxmox.com?subject=help>
List-Subscribe: <https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel>,
<mailto:pve-devel-request@lists.proxmox.com?subject=subscribe>
Reply-To: Proxmox VE development discussion <pve-devel@lists.proxmox.com>
Content-Transfer-Encoding: 7bit
Content-Type: text/plain; charset="us-ascii"; Format="flowed"
Errors-To: pve-devel-bounces@lists.proxmox.com
Sender: "pve-devel" <pve-devel-bounces@lists.proxmox.com>
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