Re: [Qemu-devel] [PATCH v8 25/54] MIG_CMD_PACKAGED: Send a packaged chunk of migration stream

[Juan Quintela]

"Dr. David Alan Gilbert (git)" <address@hidden> wrote:
> From: "Dr. David Alan Gilbert" <address@hidden>
>
> MIG_CMD_PACKAGED is a migration command that wraps a chunk of migration
> stream inside a package whose length can be determined purely by reading
> its header.  The destination guarantees that the whole MIG_CMD_PACKAGED
> is read off the stream prior to parsing the contents.
>
> This is used by postcopy to load device state (from the package)
> while leaving the main stream free to receive memory pages.
>
> Signed-off-by: Dr. David Alan Gilbert <address@hidden>
> Reviewed-by: Amit Shah <address@hidden>


Reviewed-by: Juan Quintela <address@hidden>

But I propose the change below


> +    size_t len = qsb_get_length(qsb);

....

> +    /* all the data follows (concatinating the iov's) */
> +    for (cur_iov = 0; cur_iov < qsb->n_iov; cur_iov++) {
> +        /* The iov entries are partially filled */
> +        size_t towrite = (qsb->iov[cur_iov].iov_len > len) ?
> +                              len :
> +                              qsb->iov[cur_iov].iov_len;

Or something have been very wrong here, or qsb->iov[cur_iov].iov_len can
never be > len.  So this should be the same than:

size_t towrite = MIN(qsb->iov[cur_iov].iov_len, len);

right?


> +        len -= towrite;
> +
> +        if (!towrite) {
> +            break;
> +        }

This should never happen, right?  And if we want to be extra safe,


> +    QEMUFile *packf = qemu_bufopen("r", qsb);
> +
> +    ret = qemu_loadvm_state_main(packf, mis);
> +    trace_loadvm_handle_cmd_packaged_main(ret);
> +    qemu_fclose(packf);
> +    qsb_free(qsb);

Migration code is re-entrant!!!!!  Who would have guessed O:-)

Later, Juan.