Re: [PATCH proxmox-backup 1/2] pbs-tape: fix undefined behavior in block header deallocation

[Robert Obkircher <r.obkircher@proxmox.com>]

On 10.08.26 11:51, Max R. Carrara wrote:
> [..]
>
> Could you perhaps show what would make it easier to reason about for you
> instead? Maybe I'm just missing something here.
The indirection just makes it easier to overlook things.

For example, if someone increased the size of PayloadType it is now
very easy to overlook that it could result in an allocation that is
bigger that size_of_val(t).

Not an actual problem, though.

>>>>> +
>>>>> [..]
>>>>>
>>>>> +// Putting underlying implementation in here to prevent access to private fields
>>>>>
>>>> This seems like a confusing, unrelated change with no benefit.
>>> The main benefit here is that private fields cannot be accessed anymore.
>> I'm not fully convinced that the definition of an extern format needs
>> to be hidden from someone willing to work at that level.
> What do you mean with hidden? All fields are still accessible through
> their respective methods, it's just that *directly* modifying those
> fields is not as easy as before anymore. There wasn't any point in
> exposing the fields as `pub` in the first place IMO, and I also think
> that doing that encourages bad patterns ...
TLDR, since we already discussed this offline:

I think fields are slightly better for this specific case because it's
plain old data and you can mutably borrow more than one at a time. Not
that important, though.
>
> For example, in both the [reader] and [writer] implementations we are
> casting the entire struct to a byte slice directly, even though that
> could've been a method each instead, keeping the implementation of such
> an operation local to the struct. That's why I introduced `as_bytes()`
> and `as_bytes_mut()`, which do exactly that.

Those two are great! By accessors I meant getters and setters.

We should seriously consider making zerocopy an explicit dependency.
It is already used by rand and hashbrown and it could handle this
exact case [1] and the byte-order aware U24 length field [2] without
any unsafe code on our part. I was already considering it for the
index readers and Fabian said he wouldn't mind if we could eventually
get rid of endian_trait(_derive) that way.

[1] https://docs.rs/zerocopy/latest/zerocopy/byteorder/index.html#example

[2] https://docs.rs/u24/latest/u24/unaligned/index.html

> [..]
>
>> There is a [2]nd one that I had posted on Zulip, and the third could
>> be a unit test that checks an Rc count.
>>
>> I just prefer mechanical changes like moving code to be in separate
>> commits.
>>
>> [2]
>> https://git.proxmox.com/?p=proxmox-backup.git;a=blob;f=pbs-tape/src/sgutils2.rs;h=340616c73675b994c8761a6981fd41e2e249f953;hb=HEAD#l447
> Hmm, fair point! I'll see if I can fix the second instance in v2 too,
> then.
>
> Which third one are you referring to exactly?
I just meant a unit test that can be run without
Zmiri-disable-isolation to verify that drop is invoked.
> [..]