On 11/03/2016 07:40 PM, Jianjun Duan wrote: > > > On 11/03/2016 10:17 AM, Halil Pasic wrote: >> >> >> On 11/03/2016 05:47 PM, Jianjun Duan wrote: >>> >>> On 11/03/2016 05:22 AM, Halil Pasic wrote: >>>>> >>>>> >>>>> On 11/02/2016 11:47 AM, Juan Quintela wrote: >>>>>>> Jianjun Duan <du...@linux.vnet.ibm.com> wrote: >>>>>>>>> Add a test for QTAILQ migration to tests/test-vmstate.c. >>>>>>>>> >>>>>>>>> Signed-off-by: Jianjun Duan <du...@linux.vnet.ibm.com> >>>>>>> >>>>>>> Reviewed-by: Juan Quintela <quint...@redhat.com> >>>>>>> >>>>> >>>>> Empty QTAILQ seems to be broken. Have written a small >>>>> test to prove my point. It May even make sense to have such >>>>> a test in the test-suite (some prettyfication might be >>>>> necessary though). >>>>> >>> It is working as intended. >>> >> >> My train of thought was that the object holding the queue might >> be dynamically allocated by the migration code or otherwise >> uninitialized. I was unaware these scenarios are prohibited. >> >> > This is a valid point. To get this covered vmstate_load_state needs to > be revised so that at any moment of recursion we know if the field is in > a dynamic created structure. If yes the structures which need > initialization such as QTAILQ can be initialized. >
Or you just zero out the head in VMStateInfo.get right away and to not care what was there. Of course this is only if loading to non-empty lists is invalid. This is why I cared describing why I think it is (invalid). > I would leave this until the need is there. In current device migration > code I imagine such scenarios would be rare if they should appear at > all. Because all the devices (even the hotplugged ones) are already > initialized on target. So a QTAILQ in such context should already be > initialized. Otherwise it should be fixed. > I agree that this type of a solution is an overkill for something nobody needs at the moment. >>> The current design is to append the qtailq from source to the >>> corresponding one on target. >> >> I do not see this documented. I'm used to vmstate_load overwriting >> values and following pointers, so IMHO it is not obvious that >> qtailq load does append. >> > > I will document this. > I'm fine with this option too but I think I would slightly prefer the solution described above. Maybe some of the more experienced guys (think Paolo, Juan, Dave) will come up with some guidance. >>> It works well for the task in hard >>> such as migrating ccs_list and pending_events for DRC objects. >>> >> >> Because target head is always properly initialized to empty queue? >> > They may not be empty. But they should be initialized. >>> I suspect in most cases the qtailqs on target are empty. >> >> If I think about migration having no queues populated with >> elements on a target site sounds very reasonable since IFAIU >> the target should not do any work which would populate these >> data structures. >> > See above. >> > >> >>> If not, >>> appending to them is a good choice. Clearing them is tricky since >>> each queue probably require a specialized routine to clean. If they >>> are not empty there are must be good reasons for that. >> >> Have you some code or a scenario in mind where this is legit? I >> mean creating a mix of the state(?) we found at the target and >> the state captured at the source does not sound right. I would >> argue that the target should not have any state which is subject >> to migration. >> >> You are right a non-empty queue is trouble, and frankly I never >> considered it as a valid scenario. >> > It may not be a mix of state. It really depends on how the overall state > of the devices is designed. If there are dependence between different > elements of the state, then difference in these elements may break some > consistency. One example is that migrating the length of the qtailq but > appending the content to a non-empty qtailq. In such a case the length > should not be migrated. It should be calculated on target instead. > > It should be treated case by case. > I did not get your argument here, but I think we can both live without me understanding this. Halil
