Patchwork Radix for layout ranges

login
register
about
Submitter David Hendricks
Date 2010-11-22 22:09:37
Message ID <AANLkTin=KrkgfmAuXtRFhQK3xdgQkkPZUL3dXeaNww2g@mail.gmail.com>
Download mbox | patch
Permalink /patch/2349/
State Rejected
Headers show

Comments

David Hendricks - 2010-11-22 22:09:37
Hi,
I just noticed (read: hit an issue) that ranges specified in layout files
are always assumed to be hex. From layout.c:
rom_entries[romimages].start = strtol(tstr1, (char **)NULL, 16);
 rom_entries[romimages].end = strtol(tstr2, (char **)NULL, 16);

Is there a specific reason base 16 is used? Should we just leave it at "0"
and let strtol decide, depending on the prefix (0x for hex, dec otherwise)?

The reason I bring this up is because I am working with testing scripts that
generate ranges on the fly. It's an extra step to convert values to hex
(printf %x $value?) and I want to reduce dependency on external programs /
maximize portability.

If this sounds reasonable, the patch attached simply changes the radix used
by strtol to "0" do dec is chosen if there is no prefix and hex is chosen if
the number is prefixed with 0x.
Signed-off-by: David Hendricks <dhedndrix@google.com>
Carl-Daniel Hailfinger - 2010-11-22 22:44:10
On 22.11.2010 23:09, David Hendricks wrote:
> I just noticed (read: hit an issue) that ranges specified in layout files
> are always assumed to be hex. From layout.c:
> rom_entries[romimages].start = strtol(tstr1, (char **)NULL, 16);
>  rom_entries[romimages].end = strtol(tstr2, (char **)NULL, 16);
>
> Is there a specific reason base 16 is used? Should we just leave it at "0"
> and let strtol decide, depending on the prefix (0x for hex, dec otherwise)?
>
> The reason I bring this up is because I am working with testing scripts that
> generate ranges on the fly. It's an extra step to convert values to hex
> (printf %x $value?) and I want to reduce dependency on external programs /
> maximize portability.
>
> If this sounds reasonable, the patch attached simply changes the radix used
> by strtol to "0" do dec is chosen if there is no prefix and hex is chosen if
> the number is prefixed with 0x.
> Signed-off-by: David Hendricks <dhedndrix@google.com>
>   

AFAICS that breaks ranges with leading zero, which are assumed to be
octal after this change.
Besides that, it breaks all existing layout files which are missing the
0x prefix as documented in the man page.
The man page has the following layout file example:
00000000:00008fff gfxrom
00009000:0003ffff normal
00040000:0007ffff fallback
Note the missing 0x in front of the values.

Given that this is a major user interface change, we might as well
switch to a new layout file format which has better syntax.

Regards,
Carl-Daniel
Uwe Hermann - 2010-11-23 00:11:45
On Mon, Nov 22, 2010 at 11:44:10PM +0100, Carl-Daniel Hailfinger wrote:
> AFAICS that breaks ranges with leading zero, which are assumed to be
> octal after this change.
> Besides that, it breaks all existing layout files which are missing the
> 0x prefix as documented in the man page.
> The man page has the following layout file example:
> 00000000:00008fff gfxrom
> 00009000:0003ffff normal
> 00040000:0007ffff fallback
> Note the missing 0x in front of the values.
> 
> Given that this is a major user interface change, we might as well
> switch to a new layout file format which has better syntax.

Yeah, I'd change it to whatever is the best-suited syntax rather sooner
than later. I doubt this functionality has been used much at all anyway,
to be honest. I for one didn't ever use or need it so far.


Uwe.
Carl-Daniel Hailfinger - 2010-11-23 20:10:04
On 23.11.2010 01:11, Uwe Hermann wrote:
> On Mon, Nov 22, 2010 at 11:44:10PM +0100, Carl-Daniel Hailfinger wrote:
>   
>> AFAICS that breaks ranges with leading zero, which are assumed to be
>> octal after this change.
>> Besides that, it breaks all existing layout files which are missing the
>> 0x prefix as documented in the man page.
>> The man page has the following layout file example:
>> 00000000:00008fff gfxrom
>> 00009000:0003ffff normal
>> 00040000:0007ffff fallback
>> Note the missing 0x in front of the values.
>>
>> Given that this is a major user interface change, we might as well
>> switch to a new layout file format which has better syntax.
>>     
>
> Yeah, I'd change it to whatever is the best-suited syntax rather sooner
> than later. I doubt this functionality has been used much at all anyway,
> to be honest. I for one didn't ever use or need it so far.
>   

Ron used it, and a few mainboard vendors use it internally.

Regards,
Carl-Daniel

Patch

Index: layout.c
===================================================================
--- layout.c	(revision 1234)
+++ layout.c	(working copy)
@@ -167,8 +167,8 @@ 
 			fclose(romlayout);
 			return 1;
 		}
-		rom_entries[romimages].start = strtol(tstr1, (char **)NULL, 16);
-		rom_entries[romimages].end = strtol(tstr2, (char **)NULL, 16);
+		rom_entries[romimages].start = strtol(tstr1, (char **)NULL, 0);
+		rom_entries[romimages].end = strtol(tstr2, (char **)NULL, 0);
 		rom_entries[romimages].included = 0;
 		romimages++;
 	}