[syslinux] [PATCH] pci: resize pci_device arrays

[Sebastian Herbszt]

H. Peter Anvin wrote:

>> I thought of this patch as one which does "just fix" the current overflow problems.
>> Since it's simple and doesn't require any "big" code changes it can be done fast so
>> syslinux doesn't ship with those overflow problems any longer. Later we can change
>> the pci code to use your pci_device[PCI_DEVICE][FUNC] array structure or linked
>> lists.
>> 
>>> Also, uint8_t for "count" is unsafe since we can have up to 256 devices. 
>> 
>> uint8_t count can hold values from 0 till 255 and the array can have 256 elements
>> (0 till 255) so it should be ok.
> 
> No... think about it.  Valid counts are from 0 (no devices) to 256 
> (every possible device) *inclusive*.

And uint8_t count would wrap to 0, d'oh.

> Furthermore, due to alignment issues there is no reason not to just use 
> "int" (or "unsigned int") here.

Will change to uint32_t.

>>>  On the other hand, if we use a geographical (uncompacted) 
>>> representation, perhaps the count is unnecessary.
>>>
>>>>  struct pci_device_list {
>>>> - struct pci_device pci_device[MAX_PCI_DEVICES];
>>>> + struct pci_device pci_device[MAX_PCI_BUSES * MAX_PCI_DEVICES * MAX_PCI_FUNC];
>>>>  uint8_t count;
>>>>  };
>>> Using uint8_t for "count" here is even more wrong since we can have up 
>>> to 65536 devices.
>> 
>> Here you are correct, I have missed this. uint16_t should be enough here, right?
>> This likely also requires changes to scan.c and maybe some users of pci_device_list.
> 
> Again, valid values are from 0-65536, inclusive.  Again, due to 
> alignment issues we might just as well use "int".

Will use uint32_t here too. This also applies to pci_bus_list->count which i will also
change from uint8_t to uint32_t. Code in scan.c and pcitest.c does already use
(unsigned) int, so it doesn't have to be changed. Only pci.h needs changes so far.
 
>>> Also, each structure like this is over a megabyte in size.  How many of 
>>> them will we *ever* have at the same time?
>> 
>> The current code only has one, but this array should be changed in the future (s.a).
>> 
>> Should i respin this patch with the missing changes or do you consider this approach not valid?
> 
> If it is only one, then it's good enough for now, and I'm keen on 
> getting a low-risk fix for 3.71, so please respin.

Will do.

- Sebastian