Re: [PATCH] livepatch: do not use .livepatch.funcs section to store internal state

[Jan Beulich <jbeulich@xxxxxxxx>]

On 16.11.2023 14:54, Roger Pau Monné wrote:
> On Thu, Nov 16, 2023 at 01:39:27PM +0100, Jan Beulich wrote:
>> On 16.11.2023 12:58, Roger Pau Monne wrote:
>>> --- a/xen/include/public/sysctl.h
>>> +++ b/xen/include/public/sysctl.h
>>> @@ -991,10 +991,7 @@ struct livepatch_func {
>>>      uint32_t new_size;
>>>      uint32_t old_size;
>>>      uint8_t version;        /* MUST be LIVEPATCH_PAYLOAD_VERSION. */
>>> -    uint8_t opaque[LIVEPATCH_OPAQUE_SIZE];
>>> -    uint8_t applied;
>>> -    uint8_t patch_offset;
>>> -    uint8_t _pad[6];
>>> +    uint8_t _pad[39];
>>
>> Should this be LIVEPATCH_OPAQUE_SIZE+8 and ...
> 
> Hm, I'm not sure that's any clearer.  In fact I think
> LIVEPATCH_OPAQUE_SIZE shouldn't have leaked into sysctl.h in the first
> place.
> 
> If we later want to add more fields and bump the version, isn't it
> easier to have the padding size clearly specified as a number?

If new fields (beyond the present padding size) would need adding,
that would constitute an incompatible change anyway. Until then imo
it would be clearer to keep the reference to the original constant.
But thinking about it again, the difference is perhaps indeed only
marginal. Anyway, I'll leave this to the livepatch maintainers.

One further related remark though: Now that you pointed me at the
other use of the constant in the public header, don't you want to
update the comment there, for it to not become stale (in referring
to struct livepatch_func)?

Jan