| Message ID | 1531917704-21355-2-git-send-email-thuth@redhat.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Clean-ups and fixes for the build_romfs tool | expand |
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 } ; Segher
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?)?
On 19.07.2018 11:34, 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?)? Looking at the code again, I think we don't need the FLASHFS_HEADER_DATA_SIZE macro at all anymore, so I'll remove it in v2. Thomas
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, but yes it helps to verify the sizes! And the offsets, and the endianness, and that there is no padding anywhere, etc. Segher
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".
On Fri, Jul 20, 2018 at 01:35:14PM +1000, Alexey Kardashevskiy wrote: > On 20/7/18 2:43 am, Segher Boessenkool wrote: > > 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? It depends on the ABI you are currently using, which makes the code much less portable than you may hope for. Segher
diff --git a/romfs/tools/create_crc.c b/romfs/tools/create_crc.c index 5a76b9c..d33dfe4 100644 --- a/romfs/tools/create_crc.c +++ b/romfs/tools/create_crc.c @@ -10,6 +10,7 @@ * IBM Corporation - initial implementation *****************************************************************************/ +#include <assert.h> #include <stdint.h> #include <stdio.h> #include <stdlib.h> @@ -71,21 +72,20 @@ createHeaderImage(int notime) char dastr[16] = { 0, }; unsigned long long da = 0; - 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); /* read driver info */ if (NULL != (pcVersion = getenv("DRIVER_NAME"))) { - strncpy(uHeader.stHeader.version, pcVersion, 16); + strncpy(stHeader.version, pcVersion, 16); } else if (NULL != (pcVersion = getenv("USER"))) { - strncpy(uHeader.stHeader.version, pcVersion, 16); + strncpy(stHeader.version, pcVersion, 16); } else if (pcVersion == NULL) { - strncpy(uHeader.stHeader.version, "No known user!", 16); + strncpy(stHeader.version, "No known user!", 16); } if (!notime) { @@ -104,18 +104,18 @@ createHeaderImage(int notime) } da = cpu_to_be64(strtoll(dastr, NULL, 16)); } - memcpy(uHeader.stHeader.date, &da, 8); + memcpy(stHeader.date, &da, 8); /* write Magic value into data stream */ - strncpy(uHeader.stHeader.magic, FLASHFS_MAGIC, 8); + strncpy(stHeader.magic, FLASHFS_MAGIC, 8); /* write platform name into data stream */ - strcpy(uHeader.stHeader.platform_name, FLASHFS_PLATFORM_MAGIC); + strcpy(stHeader.platform_name, FLASHFS_PLATFORM_MAGIC); /* write platform revision into data stream */ - strcpy(uHeader.stHeader.platform_revision, FLASHFS_PLATFORM_REVISION); + strcpy(stHeader.platform_revision, FLASHFS_PLATFORM_REVISION); /* fill end of file info (8 bytes of FF) into data stream */ - uHeader.stHeader.ui64FileEnd = -1; + stHeader.ui64FileEnd = -1; /* read address of next file and address of header date, both are 64 bit values */ ui64RomAddr = 0; @@ -143,8 +143,7 @@ createHeaderImage(int notime) /* fill free space in Header with zeros */ memset(&pucFileStream[ui64DataAddr], 0, (ui64RomAddr - ui64DataAddr)); /* place data to header */ - memcpy(&pucFileStream[ui64DataAddr], uHeader.pcArray, - FLASHFS_HEADER_DATA_SIZE); + memcpy(&pucFileStream[ui64DataAddr], &stHeader, sizeof(stHeader)); /* insert header length into data stream */ *(uint64_t *) (pucFileStream + FLASHFS_HEADER_SIZE_ADDR) =
Accessing the struct with memset and memcpy can also be done without the union wrapper. Signed-off-by: Thomas Huth <thuth@redhat.com> --- romfs/tools/create_crc.c | 29 ++++++++++++++--------------- 1 file changed, 14 insertions(+), 15 deletions(-)