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:37, Thomas Lamprecht wrote:
>>
> 
> 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.

sorry, i misunderstood you, yes, the TaskState::Error gets created in 
'create_state', and yes, it makes sense to do it already there...

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

while we could decode it again, existing tasks did not get encoded, so 
we would potentially decode some strings that shouldn't be decoded...

we could also simply remove the newlines at all, i don't think we would
benefit from having multiline error messages in tasks..
since most of the time we want to show a single line per task

or what about trying to parse the task log backwards
until we find a correct date+taskerror or at least a correct date?
(in the first case we can assume that the newlines are part of
the error, in the second the state would still be 'unknown' but
with a correct date)

if you don't like either approach, i'd send a
version with percent-encoded/decoded, it's imho more important
that we get the correct task state on task parsing than how we
solve it exactly

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