Re: [Qemu-devel] [PATCH 1/4] savevm: refactor qemu_loadvm_state().

[Yoshiaki Tamura <tamura.yoshiaki@lab.ntt.co.jp>]

Anthony Liguori wrote:
> On 06/03/2010 02:22 AM, Yoshiaki Tamura wrote:
>> Split qemu_loadvm_state(), and introduce
>> qemu_loadvm_state_{begin,iterate,complete,async}.
>> qemu_loadvm_state_async() is a function to handle a single incoming
>> section.
>>
>> Signed-off-by: Yoshiaki Tamura<tamura.yoshiaki@lab.ntt.co.jp>
>> ---
>> savevm.c | 206
>> +++++++++++++++++++++++++++++++++++++++++++-------------------
>> sysemu.h | 2 +
>> 2 files changed, 146 insertions(+), 62 deletions(-)
>>
>> diff --git a/savevm.c b/savevm.c
>> index dc20390..aa4f98c 100644
>> --- a/savevm.c
>> +++ b/savevm.c
>> @@ -1005,6 +1005,8 @@ typedef struct SaveStateEntry {
>>
>> static QTAILQ_HEAD(savevm_handlers, SaveStateEntry) savevm_handlers =
>> QTAILQ_HEAD_INITIALIZER(savevm_handlers);
>> +static QLIST_HEAD(, LoadStateEntry) loadvm_handlers =
>> + QLIST_HEAD_INITIALIZER(loadvm_handlers);
>> static int global_section_id;
>>
>> static int calculate_new_instance_id(const char *idstr)
>> @@ -1460,14 +1462,9 @@ typedef struct LoadStateEntry {
>> int version_id;
>> } LoadStateEntry;
>>
>> -int qemu_loadvm_state(QEMUFile *f)
>> +int qemu_loadvm_state_begin(QEMUFile *f)
>> {
>> - QLIST_HEAD(, LoadStateEntry) loadvm_handlers =
>> - QLIST_HEAD_INITIALIZER(loadvm_handlers);
>> - LoadStateEntry *le, *new_le;
>> - uint8_t section_type;
>> unsigned int v;
>> - int ret;
>>
>> v = qemu_get_be32(f);
>> if (v != QEMU_VM_FILE_MAGIC)
>> @@ -1481,73 +1478,157 @@ int qemu_loadvm_state(QEMUFile *f)
>> if (v != QEMU_VM_FILE_VERSION)
>> return -ENOTSUP;
>>
>> - while ((section_type = qemu_get_byte(f)) != QEMU_VM_EOF) {
>> - uint32_t instance_id, version_id, section_id;
>> - SaveStateEntry *se;
>> - char idstr[257];
>> - int len;
>> + return 0;
>> +}
>>
>> - switch (section_type) {
>> - case QEMU_VM_SECTION_START:
>> - case QEMU_VM_SECTION_FULL:
>> - /* Read section start */
>> - section_id = qemu_get_be32(f);
>> - len = qemu_get_byte(f);
>> - qemu_get_buffer(f, (uint8_t *)idstr, len);
>> - idstr[len] = 0;
>> - instance_id = qemu_get_be32(f);
>> - version_id = qemu_get_be32(f);
>> -
>> - /* Find savevm section */
>> - se = find_se(idstr, instance_id);
>> - if (se == NULL) {
>> - fprintf(stderr, "Unknown savevm section or instance '%s' %d\n",
>> idstr, instance_id);
>> - ret = -EINVAL;
>> - goto out;
>> - }
>> +static int qemu_loadvm_state_iterate(QEMUFile *f)
>> +{
>> + LoadStateEntry *le;
>> + uint32_t section_id;
>> + int ret;
>>
>> - /* Validate version */
>> - if (version_id> se->version_id) {
>> - fprintf(stderr, "savevm: unsupported version %d for '%s' v%d\n",
>> - version_id, idstr, se->version_id);
>> - ret = -EINVAL;
>> - goto out;
>> - }
>> + section_id = qemu_get_be32(f);
>
> But we're still blocking in qemu_get_be32() so I don't see how this
> makes it async.

In that sense, it's not completely async, but still better than being in the 
while loop that doesn't accept any commands on the incoming side.
To make things completely async, I think we should solve the lock issue and add 
a thread for incoming migration, but I don't think that's your opinion.

Did I misunderstand your comment for the previous approach?

Thanks,

Yoshi

>
> Regards,
>
> Anthony Liguori
>
>
>
>