Re: [coreboot] r3474 - in trunk/payloads/libpayload: drivers i386 include

[Jordan Crouse]

On 09/08/08 12:04 +0200, Stefan Reinauer wrote:
> Peter Stuge wrote:
> > On Thu, Aug 07, 2008 at 12:21:05PM +0200, [e-mail] wrote:
> > > Author: stepan
> > > Date: 2008-08-07 12:21:05 +0200 (Thu, 07 Aug 2008)
> > > New Revision: 3474
> > >
> > > Added:
> > >    trunk/payloads/libpayload/drivers/options.c
> > > Modified:
> > >    trunk/payloads/libpayload/drivers/Makefile.inc
> > >    trunk/payloads/libpayload/i386/coreboot.c
> > >    trunk/payloads/libpayload/include/coreboot_tables.h
> > >    trunk/payloads/libpayload/include/sysinfo.h
> > > Log:
> > > add get_option to libpayload, so coreboot cmos options can be queried.
> > >
> > > Signed-off-by: Stefan Reinauer 
> > > Acked-by: Jordan Crouse 
> > > Acked-by: Peter Stuge
> >
> > Sorry.
> >
> > This breaks my build. I should have tested. :\
> >
> > cc -m32 -Wall -Werror -fno-stack-protector -nostdinc -I./include -I/usr/lib/gcc/i686-pc-linux-gnu/4.2.2/include -c -o drivers/options.o drivers/options.c
> > cc1: warnings being treated as errors
> > drivers/options.c: In function 'get_option':
> > drivers/options.c:93: warning: pointer targets in passing argument 1 of 'memcmp' differ in signedness
> > make: *** [drivers/options.o] Error 1
> >
> > From the patch:
> > > if (memcmp(cmos_entry->name, name, len))
> >
> > ..
> > > +#define CB_TAG_OPTION         0x00c9
> > > +#define CMOS_MAX_NAME_LENGTH    32
> > > +struct cb_cmos_entries {
> > > u32 tag;
> > > u32 size;
> > > u32 bit;
> > > u32 length;
> > > u32 config;
> > > u32 config_id;
> > > u8 name[CMOS_MAX_NAME_LENGTH];
> > >
> > > +};
> >
> > memcmp() expects char *
> >
> > What should change? I'm thinking memcmp() and friends.
> >
> > Later revisions breaks more stuff. :\
>
> Weird, it compiles fine here. I strongly suggest changing the Makefile,
> adding -Wno-pointer-sign. It's a dumb warning.

Nak.  This is not a dumb warning - I have seen it expose some real
issues.  Its not hard to make sure your code is sign correct,
casting doesn't cost us anything.

And the memory compare functions should probably be taking void *.

Jordan