"Dr. David Alan Gilbert (git)" <dgilb...@redhat.com> wrote:
> From: "Dr. David Alan Gilbert" <dgilb...@redhat.com>
>
> The state of the postcopy process is managed via a series of messages;
> * Add wrappers and handlers for sending/receiving these messages
> * Add state variable that track the current state of postcopy
>
> Signed-off-by: Dr. David Alan Gilbert <dgilb...@redhat.com>
> ---
> include/migration/migration.h | 16 +++
> include/sysemu/sysemu.h | 20 ++++
> migration/migration.c | 13 +++
> migration/savevm.c | 247
> ++++++++++++++++++++++++++++++++++++++++++
> trace-events | 10 ++
> 5 files changed, 306 insertions(+)
>
> diff --git a/migration/migration.c b/migration/migration.c
> index cd89a9b..34cd9a6 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -1128,3 +1128,16 @@ void migrate_fd_connect(MigrationState *s)
> qemu_thread_create(&s->thread, "migration", migration_thread, s,
> QEMU_THREAD_JOINABLE);
> }
> +
> +PostcopyState postcopy_state_get(MigrationIncomingState *mis)
> +{
> + return atomic_fetch_add(&mis->postcopy_state, 0);
What is wrong with atomic_read() here?
As the set of the state is atomic, even a normal read would do (I think)
> +void qemu_savevm_send_postcopy_ram_discard(QEMUFile *f, const char *name,
> + uint16_t len,
> + uint64_t *start_list,
> + uint64_t *end_list)
I haven't looked at the following patches where this function is used,
but it appears that getting an iovec could be a good idea?
> +{
> + uint8_t *buf;
> + uint16_t tmplen;
> + uint16_t t;
> + size_t name_len = strlen(name);
> +
> + trace_qemu_savevm_send_postcopy_ram_discard(name, len);
> + buf = g_malloc0(len*16 + name_len + 3);
I would suggest
gmalloc0(1 + 1 + name_len + 1 + (8 + 8) * len)
just to be clear where things came from.
I think that we don't need the \0 at all. If \0 is not there,
strlen() return is going to be "funny". So, we can just change
the assert to name_len < 255?
> + buf[0] = 0; /* Version */
> + assert(name_len < 256);
Can we move the assert before the malloc()?
My guess is that in a perfect world the assert would be a return
-EINVAL, but I know that it is complicated.
> + buf[1] = name_len;
> + memcpy(buf+2, name, name_len);
spaces around '+' (same around)
> + tmplen = 2+name_len;
> + buf[tmplen++] = '\0';
> +
> + for (t = 0; t < len; t++) {
> + cpu_to_be64w((uint64_t *)(buf + tmplen), start_list[t]);
> + tmplen += 8;
> + cpu_to_be64w((uint64_t *)(buf + tmplen), end_list[t]);
> + tmplen += 8;
trace_qemu_savevm_send_postcopy_range(name, start_list[t],
end_list[t]);
??
> + /* We're expecting a
> + * Version (0)
> + * a RAM ID string (length byte, name, 0 term)
> + * then at least 1 16 byte chunk
> + */
> + if (len < 20) { 1 +
1+1+1+1+2*8
Humm, thinking about it, .... why are we not needing a length field of
number of entries?
> + error_report("CMD_POSTCOPY_RAM_DISCARD invalid length (%d)", len);
> + return -1;
> + }
> +
> + tmp = qemu_get_byte(mis->file);
> + if (tmp != 0) {
I think that a constant telling POSTCOPY_VERSION0 or whatever?
