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

[Juan Quintela <quintela@redhat.com>]

Auger Eric <eric.auger@redhat.com> wrote:
> Hi Juan,
>
> On 1/8/20 2:19 PM, Juan Quintela wrote:
>> "Dr. David Alan Gilbert" <dgilbert@redhat.com> wrote:
>>> * Eric Auger (eric.auger@redhat.com) 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 <eric.auger@redhat.com>
>>>>
>>>> +    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.

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

Later, Juan.