Re: [PATCH for-5.0 v11 01/20] migration: Support QLIST migration

[Auger Eric]

Hi Juan,

On 1/8/20 2:51 PM, Juan Quintela wrote:
> Auger Eric <address@hidden> wrote:
>> Hi Juan,
>>
>> On 1/8/20 2:19 PM, Juan Quintela wrote:
>>> "Dr. David Alan Gilbert" <address@hidden> wrote:
>>>> * Eric Auger (address@hidden) wrote:
>>>>> Support QLIST migration using the same principle as QTAILQ:
>>>>> 94869d5c52 ("migration: migrate QTAILQ").
>>>>>
>>>>> The VMSTATE_QLIST_V macro has the same proto as VMSTATE_QTAILQ_V.
>>>>> The change mainly resides in QLIST RAW macros: QLIST_RAW_INSERT_HEAD
>>>>> and QLIST_RAW_REVERSE.
>>>>>
>>>>> Tests also are provided.
>>>>>
>>>>> Signed-off-by: Eric Auger <address@hidden>
>>>>>
>>>>> +    while (qemu_get_byte(f)) {
>>>>> +        elm = g_malloc(size);
>>>>> +        ret = vmstate_load_state(f, vmsd, elm, version_id);
>>>>> +        if (ret) {
>>>>> +            error_report("%s: failed to load %s (%d)", field->name,
>>>>> +                         vmsd->name, ret);
>>>>> +            g_free(elm);
>>>>> +            return ret;
>>>>> +        }
>>>>> +        QLIST_RAW_INSERT_HEAD(pv, elm, entry_offset);
>>>>> +    }
>>>>> +    QLIST_RAW_REVERSE(pv, elm, entry_offset);
>>>>
>>>> Can you explain why you need to do a REVERSE on the loaded list,
>>>> rather than using doing a QLIST_INSERT_AFTER to always insert at
>>>> the end?
>>>>
>>>> Other than that it looks good.
>>>
>>> This was my fault (integrated as this is).
>>>
>>> Old code had a "walk to the end of the list" and then insert.
>>> I told it was way faster just to insert and the beggining and then
>>> reverse.  I didn't noticed that we had the previous element to know
>>> where to insert.
>>
>> Not sure I get your comment. To insert at the end one needs to walk
>> though the list. The head has no prev pointer pointing to the tail as
>> opposed to the queue. So I understood Dave's comment as "just explain
>> why you prefered this solution against the QLIST_INSERT_AFTER alternative.
> 
> You have the previous inserted element, so it is kind of easy O:-)
> 
>     prev = NULL;
>     while (qemu_get_byte(f)) {
>         elm = g_malloc(size);
>         ret = vmstate_load_state(f, vmsd, elm, version_id);
>         if (ret) {
>             error_report("%s: failed to load %s (%d)", field->name,
>                          vmsd->name, ret);
>             g_free(elm);
>             return ret;
>         }
>         if (!prev) {
>             QLIST_RAW_INSERT_HEAD(pv, elm, entry_offset);
>         } else {
>             QLIST_RAW_INSERT_AFTER(prev, elm, entry_offset);
>         }
>         prev = elm;
>     }
> 
> And yes, I realize that there is no QLIST_RAW_INSTERT_AFTER() (it is
> QLIST_INSERT_AFTER).  And no, I haven't took the time to understand the
> different between QLIST and QLIST_RAW.  From a quick look, it seems that
> QLIST_RAW is embededed inside other structure.

Ah OK I get it now. Yes indeed that looks simpler.

> 
> But as said, we can move that to another patch.

OK.

Thanks

Eric
> 
> Later, Juan.
> 
>