[SLOF] [PATCH 1/4] romfs/tools: Remove superfluous union around the rom header struct

[Alexey Kardashevskiy]

On 20/7/18 2:43 am, Segher Boessenkool wrote:
> On Thu, Jul 19, 2018 at 07:34:24PM +1000, Alexey Kardashevskiy wrote:
>> On 19/7/18 12:35 am, Segher Boessenkool wrote:
>>> On Wed, Jul 18, 2018 at 02:41:41PM +0200, Thomas Huth wrote:
>>>> -	union {
>>>> -		unsigned char pcArray[FLASHFS_HEADER_DATA_SIZE];
>>>> -		struct stH stHeader;
>>>> -	} uHeader;
>>>> +	struct stH stHeader;
>>>> +
>>>> +	assert(sizeof(stHeader) == FLASHFS_HEADER_DATA_SIZE);
>>>>  
>>>>  	/* initialize Header */
>>>> -	memset(uHeader.pcArray, 0x00, FLASHFS_HEADER_DATA_SIZE);
>>>> +	memset(&stHeader, 0x00, FLASHFS_HEADER_DATA_SIZE);
>>>
>>> It would be nice to have a build-time assert for this.
>>>
>>> You can also get rid of this explicit memset by doing this as
>>>
>>> 	assert(sizeof(stHeader) == FLASHFS_HEADER_DATA_SIZE);
>>>
>>> 	struct stH stHeader = { 0 } ;
>>
>>
>> I do not really know where this flashfs/romfs format came from so I'll
>> simply ask why precisely we want this assert? Increasing
>> FLASHFS_HEADER_DATA_SIZE to 0x88 does not make any difference as the
>> header size is stored in a file header (is that even correct thing to say?)?
> 
> The code is treating some binary data as a C struct.  Not a good idea at all,

Besides missing "__attribute__ ((packed))", what is wrong with that?

> but yes it helps to verify the sizes!  And the offsets, and the endianness,
> and that there is no padding anywhere, etc.

But the size itself does not matter there at all, why to stick to a
certain value? If we do not want the header to be too big, then there is
check with (mistyped) "Header File to long".


-- 
Alexey