[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v8 25/54] MIG_CMD_PACKAGED: Send a packaged chun
From: |
Dr. David Alan Gilbert |
Subject: |
Re: [Qemu-devel] [PATCH v8 25/54] MIG_CMD_PACKAGED: Send a packaged chunk of migration stream |
Date: |
Mon, 26 Oct 2015 16:21:28 +0000 |
User-agent: |
Mutt/1.5.24 (2015-08-30) |
* Juan Quintela (address@hidden) wrote:
> "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?
Done.
> > + len -= towrite;
> > +
> > + if (!towrite) {
> > + break;
> > + }
>
> This should never happen, right? And if we want to be extra safe,
qsb_get_length() returns the amount of data in the qsb, not the
amount of allocated space; so it's legal for the qsb to have
allocated an iov entry but not actually put any data in it yet.
Will it have done that in our case? I don't think so, but no reason
to make assumptions.
> > + 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:-)
To a very limited degree; there's global state shotgunned around everywhere
(e.g. in the RAM code).
Dave
> Later, Juan.
--
Dr. David Alan Gilbert / address@hidden / Manchester, UK
- Re: [Qemu-devel] [PATCH v8 21/54] Return path: Source handling of return path, (continued)
- [Qemu-devel] [PATCH v8 24/54] Add wrappers and handlers for sending/receiving the postcopy-ram migration messages., Dr. David Alan Gilbert (git), 2015/10/01
- [Qemu-devel] [PATCH v8 25/54] MIG_CMD_PACKAGED: Send a packaged chunk of migration stream, Dr. David Alan Gilbert (git), 2015/10/01
- [Qemu-devel] [PATCH v8 26/54] Modify save_live_pending for postcopy, Dr. David Alan Gilbert (git), 2015/10/01
- [Qemu-devel] [PATCH v8 27/54] postcopy: OS support test, Dr. David Alan Gilbert (git), 2015/10/01
- [Qemu-devel] [PATCH v8 28/54] migrate_start_postcopy: Command to trigger transition to postcopy, Dr. David Alan Gilbert (git), 2015/10/01
- [Qemu-devel] [PATCH v8 29/54] MIGRATION_STATUS_POSTCOPY_ACTIVE: Add new migration state, Dr. David Alan Gilbert (git), 2015/10/01