Re: [pbs-devel] [PATCH proxmox-backup v2 1/2] rest-server/worker-task: replace newlines with '\n' in task result

[Dominik Csapak <d.csapak@proxmox.com>]

On 10/11/21 15:23, Thomas Lamprecht wrote:
> On 11.10.21 15:14, Dominik Csapak wrote:
>> On 10/11/21 15:12, Thomas Lamprecht wrote:
>>> On 11.10.21 14:14, Dominik Csapak wrote:
>>>> we parse the task result from the last line, so we should not print a
>>>> new line in the task result, else we get an 'unknown' task state
>>>>
>>>
>>> isn't the issue that we already print the newline in the file_logger's log method
>>> this is using underneath and we could get two newlines (i.e. a last empty line)
>>> which then failed to parse?
>>
>> no in my case the issue is that the message contained a newline in the middle:
> 
> hmm, ok,  and example where/how it happens could have helped then here ;)

thats fair ;)

> 
>> --8<--
>> 2021-09-28T08:34:00+02:00: TASK ERROR: reopen commands failed, proxy: unable to parse parameters (expected json object)
>> ; api: unable to parse parameters (expected json object)
>> -->8--
> 
> Yeah, but why replacing it with a literal \n then? Why not percent encoding
> or fixing it at the root in create_state?

i was going to find the root cause (i do not think its in create state, 
but where the error is generated)

but wanted to have a generic method to not have a wrong task state even
when it happens again somewhere

imho, percent encoding(%0A) is not nicer in this context here than \n

> 
>>>
>>>> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
>>>> ---
>>>> no changes
>>>>    proxmox-rest-server/src/worker_task.rs | 2 +-
>>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/proxmox-rest-server/src/worker_task.rs b/proxmox-rest-server/src/worker_task.rs
>>>> index 51394549..a8899ab9 100644
>>>> --- a/proxmox-rest-server/src/worker_task.rs
>>>> +++ b/proxmox-rest-server/src/worker_task.rs
>>>> @@ -494,7 +494,7 @@ impl TaskState {
>>>>            match self {
>>>>                TaskState::Error { message, .. } => format!("TASK ERROR: {}", message),
>>>>                other => format!("TASK {}", other),
>>>> -        }
>>>> +        }.replace('\n', "\\n") // no newline in task result
>>>
>>> Why not `.trim_end()` ? A literal \n seems rather odd to me..
>>>
>>> Comment suggestion:
>>>
>>> // our consumer already add \n where required, so avoid double-newline
>>
>> again that was not the issue
> 
> well, the comment was not really helpful in that case either..
> 
> Also, in that case it would make more sense to operate on the TaskState::Error branch
> or avoid setting such a bad state in the first place..

you're right, thats a better place

> 
>>
>>>
>>>>        }
>>>>          fn from_endtime_and_message(endtime: i64, s: &str) -> Result<Self, Error> {
>>>>
>>>
>>
>>
>