On 16.07.2025 21:34, Petr Beneš wrote:
> On Wed, Jul 16, 2025 at 12:27 PM Jan Beulich <jbeul...@suse.com> wrote:
>>> + if ( next != start )
>>> + {
>>> /* Set current chassis handle if present */
>>> - if ( p->header.length > 13 )
>>> + if ( p->header.length >= offsetof(struct smbios_type_2,
>>> board_type) )
>>
>> Comment and code don't fit together, unless one goes check that board_type
>> is the field immediately following chassis_handle.
>
> That's the tragedy of using offsetof in this situation. What is mostly
> needed throughout this code is "offsetof(x) + sizeof(x)".
Which is what, in the end, I was alluding to. Sorry if I didn't make that
clear enough.
> Instead, I'm
> mostly using offsetof(a-field-that-is-following-the-field-that-i-really-meant)
> which leads to comments that seemingly don't make sense.
>
> How should I ideally proceed? Should I introduce a new macro?
Perhaps. Maybe something like endof_field(), along the lines of the
sizeof_field() that we have in the hypervisor.
>>> if ( *((uint16_t*)ptr) != 0 )
>>> *((uint16_t*)ptr) = SMBIOS_HANDLE_TYPE3;
>>
>> Why not switch to p->chassis_handle, without any use of "ptr"? Yet then I
>> fear I don't really understand what is being done here.
>
> Right, that would make sense. I left the original code intact.
>
>> Why would an arbitrary non-zero value be overwritten with a fixed value?
>
> That's a question for the original author. FWIW, qemu does not coerce
> these values.
>
> But if I had to guess, the original author wanted to make sure that
> the SMBIOS data do not reference nonsensical handles.
>
> I would argue that if a user decided to fiddle with these values, they
> know what they're doing and I would let them shoot in the foot if they
> desire to do so (in other words, I would remove this coercion; but
> that's not up to me to decide).
>
>> The other comment may want retaining, though.
>
> Which one do you mean? This one?
>
>> - /* Only present when passed in */
Yes.
> If so, I should probably add this comment to all the newly introduced
> tables as well.
I was indeed wondering ...
Jan
