On 1/12/18 12:57, Peter Maydell wrote: > On Fri, 30 Nov 2018 at 20:47, Corey Minyard <miny...@acm.org> wrote: >> >> On 11/30/18 11:39 AM, Peter Maydell wrote: >>> On Mon, 26 Nov 2018 at 20:04, <miny...@acm.org> wrote: >>>> From: Philippe Mathieu-Daudé <phi...@redhat.com> >>>> /* XXX: make this persistent */ >>>> - uint8_t *eeprom_buf = g_malloc0(8 * SMBUS_EEPROM_SIZE); >>>> + eeprom_buf = g_malloc0(nb_eeprom * SMBUS_EEPROM_SIZE); >>> So if we allocate N buffers as the caller requests, what >>> is the thing that means that more than 8 won't work ? >>> >>> We've now changed from allocating always 8 lots of >>> the EEPROM size to possibly allocating fewer than that. >>> How does the code in the device know what the size >>> of the buffer we're passing as the "data" property is >>> now? We don't pass it the number of EEPROMs as a property. >> >> It doesn't have to. Each EEPROM is 256 bytes and that's all the data >> it has. >> >> But this whole thing is confusing, I agree. The more I look at this >> particular file, the less I like it. But the caller is basically saying: >> "I need N EEPROMs, here's the initialization data". That's not >> really very flexible, IMHO the EEPROM devices should be created >> individually using standard qemu methods. > > Oh, yes, I see now. We pass in a block of N * 256 bytes, and > the function then hands a pointer to offsets 0, 256, ... > to each device it creates. > > I definitely don't see why we need to say "only a maximum of > 8" now, though -- where does that limit come from? If we > get passed in an arbitrary number of EEPROMs X, and we allocate > 256 * X bytes, and create X devices which each get one slice of > the buffer, what goes wrong when X > 8 ? > >> I'm tempted to rewrite this whole thing to make it cleaner. > > It's certainly pretty awkward code. I think personally given > this patchset is already pretty big I'd go for getting it into > master first and then doing a followup with further cleanup.
As suggested Peter, I'd drop this patch (in favor of a later clean rewrite) and simply add an assert(). Regards, Phil.