On Thu, 10 Jan 2019 10:50:30 -0500 Tony Krowiak <akrow...@linux.ibm.com> wrote:
> On 1/9/19 12:35 PM, Halil Pasic wrote: > > On Wed, 9 Jan 2019 10:36:11 -0500 > > Tony Krowiak <akrow...@linux.ibm.com> wrote: > > > >> On 1/9/19 5:14 AM, Cornelia Huck wrote: [..] > >> A search reveals that max_index is used in only two places: It is used > >> to set the index for a child of the bus (i.e., bus_add_child function in > >> hw/core/qdev.c); and prior to this patch, to determine if max_dev has > >> been exceeded (i.e., qbus_is_full function in qdev_monitor.c). From > >> what I can see, the bus child index is used only to generate a property > >> name of the format "child[%d]" so the child can be linked as a property > >> of the bus (see bus_add_child and bus_remove_child functions in > >> hw/core/qdev.c). Wraparound of the max_index would most likely result in > >> generating a duplicate property name for the child. > >> > >> I propose two possible solutions: > >> > >> 1. When max_index reaches INT_MAX, do not allow any additional children > >> to be added to the bus. > >> > >> 2. Set a max_dev value of INT_MAX for the BusClass instance if the value > >> is not set (in the bus_class_init function in hw/core/bus.c). > >> > >> 3. Instead of generating the property name from the BusChild index > >> value, generate a UUID string. Since the index field of the BusChild > >> appears to be used only to generate the child's name, maybe change > >> the index field to a UUID field or a name field. > >> > > > > Separate problem, separate patch, or? > > Good question. > IMHO this patch should not be delayed because of the mostly unrelated discussion on max_index wrapping. BTW Tony do you want to do a patch on max_index? > > > > UUID instead of index solves the problem of unique names I guess, but I > > can't tell if that would be acceptable (interface stability, etc.). > > I may have missed something, but as currently used, the BusChild index > is accessed in only two places; when the child is added to the bus and > when it is removed from the bus. In both cases, it is used to derive > a unique property name for the child device. > The name is probably externally observable (via QMP). It could be also a stable part of an (external) API. We would need damn good reasons to break an external API. > Speaking of index, it implies ordering of the bus's children. IMHO, it > only makes semantical sense if the index changes when a child device > with a lower index value is removed from the bus. If a child device > has an index of 5 - i.e., the fifth child on the bus - and the child > device with index 4 is removed, then the child device with index 5 is > no longer the fifth child on the bus. Maybe its just the fact that > these fields are named 'index'. The fact that they are not really used > as indexes caused a bit of confusion for me when analyzing this code and > seems like a misnomer to me. > No comment. > > > > The max_dev won't help because we can get a collision while maintaining > > the invariant 'not more than 2 devices on the bus'. > > I don't understand, can you better explain what you mean? When you > say "we can get a collision", are you talking about the property > name? What does max_dev have to do with that? Please explain. AFAIU the whole point of the exercise with max_index is to generate bus unique names. By we can get a collision (please see https://en.oxforddictionaries.com/definition/collision), I mean we can end up assigning the same name to more than one device on the very same bus. > What do > you mean by "maintaining the invariant 'not more than 2 devices on the > bus'"? > Let me describe the scenario. Let's have a bus with dev_max == 2. We first add one device, then another to that bus. Then we unplug and replug the second device. The name goes from 'child[n]' to 'child[n+1]' with each iteration of unplug/replug with int arithmetic. That is after a number of iterations we will reach the name 'child[0]'. But we already have a child[0] on the bus. > > > > So if we don't want to limit the number of hotplug operations, and we do > > want to keep the allocation scheme before wrapping, the only solution I > > see is looking for the first free identifier after we wrap. > > Are you are saying to look for the first index value that is not > assigned to a BusChild object? > Only after we reach the biggest integer. We want to keep everything as is for the cases that work today. > > > > BTW I wonder if making max_index and index unsigned make things bit less > > ugly. > > I thought the same. They could also be made unsigned long or > unsigned long long to increase the number of child devices that can be > plugged in before having to deal with exceeding the index value. > My rationale is: names with the negatives would look weird. Regards, Halil
