[pbs-devel] [PATCH proxmox-backup v2 3/4] datastore: data blob: increase compression throughput

[Thomas Lamprecht]

Seems I forgot to reply-all, so while this is outdated (i.e., already implemented
by Dominik in v3) it still is nice to have the list complete, for the record so
to say.

On 02/08/2024 14:38, Dominik Csapak wrote:
> ---
> fn map_error_code(code: usize) -> io::Error {
>     let msg = zstd_safe::get_error_name(code);
>     io::Error::new(io::ErrorKind::Other, msg.to_string())
> }
> ---
>
> which calls this:
>
> ---
> pub fn get_error_name(code: usize) -> &'static str {
>     unsafe {
>         // Safety: assumes ZSTD returns a well-formed utf8 string.
>         let name = zstd_sys::ZSTD_getErrorName(code);
>         c_char_to_str(name)
>     }
> }
> ---
>
> which is part of the zstd api and at the end it maps the error code like this:
>
> ---
> ERR_STATIC ERR_enum ERR_getErrorCode(size_t code) { if (!ERR_isError(code)) return (ERR_enum)0; return (ERR_enum) (0-code); }
> ---
>
> with that result, it maps the code to a string...
>
> which matches what i get, since
>
> 2^64 - 70 = 18446744073709551546 [0]
>

Thanks for looking into this and providing the explanation.

> but, i'm really not sure if we could rely in that since the function is in a 'error_private.c' which seems to me like it's an implementation detail only?
>

Yeah, it's not ideal... But it could be made safe enough by adding a test that runs
on build and triggers this error explicitly by passing a way to small target buffer,
that way we can notice when this internal error changes, which is IMO not _that_
likely, at least not during the same major Debian release, as there we normally
only get critical bug and security fixes, and while I don't want to curse it,
but I'd really be surprised if this particular code would change semantics, as
it's hard to envision that the widely used `- code` pattern to return errors
in C ABIs should be part of such a critical flaw.

And yeah, while that is not the interface I'd wish for, it doesn't really feels
significantly worse to me than doing matching on error string, as those aren't
guaranteed to be 100% stable either I'd think.