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

[Thomas Lamprecht <t.lamprecht@proxmox.com>]

On 11.10.21 15:30, Dominik Csapak wrote:
> 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)
> 

I do not mean the root cause that logged a newline at the end, we cannot avoid the future,
but the one that crates the TaskError enum, and if I'm not completely sleep deprived, that 
is create_state.

> 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

percent encoding is an actual format than can be rendered again nicely, just randomly
mapping the \n control code (0x0A) to 0x5C 0x6E, without escaping existing (literal) \n
just isn't a used format, at least FWICR, that's my point in provoking that question.

> 
>>
>>>>
>>>>> 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> {
>>>>>
>>>>
>>>
>>>
>>
> 
> 
> 
> _______________________________________________
> pbs-devel mailing list
> pbs-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel