On Mon, Nov 17, 2014 at 07:07:33PM +0000, Dr. David Alan Gilbert wrote:
> * David Gibson (da...@gibson.dropbear.id.au) wrote:
> > On Fri, Oct 03, 2014 at 06:47:42PM +0100, Dr. David Alan Gilbert (git)
> > wrote:
> > > From: "Dr. David Alan Gilbert" <dgilb...@redhat.com>
> > >
> > > On receiving MIG_RPCOMM_REQPAGES look up the address and
> > > queue the page.
> > >
> > > Signed-off-by: Dr. David Alan Gilbert <dgilb...@redhat.com>
> > > ---
> > > arch_init.c | 52
> > > +++++++++++++++++++++++++++++++++++++++++++
> > > include/migration/migration.h | 21 +++++++++++++++++
> > > include/qemu/typedefs.h | 3 ++-
> > > migration.c | 34 +++++++++++++++++++++++++++-
> > > 4 files changed, 108 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/arch_init.c b/arch_init.c
> > > index 4a03171..72f9e17 100644
> > > --- a/arch_init.c
> > > +++ b/arch_init.c
> > > @@ -660,6 +660,58 @@ static int ram_save_page(QEMUFile *f, RAMBlock*
> > > block, ram_addr_t offset,
> > > }
> > >
> > > /*
> > > + * Queue the pages for transmission, e.g. a request from postcopy
> > > destination
> > > + * ms: MigrationStatus in which the queue is held
> > > + * rbname: The RAMBlock the request is for - may be NULL (to mean
> > > reuse last)
> > > + * start: Offset from the start of the RAMBlock
> > > + * len: Length (in bytes) to send
> > > + * Return: 0 on success
> > > + */
> > > +int ram_save_queue_pages(MigrationState *ms, const char *rbname,
> > > + ram_addr_t start, ram_addr_t len)
> > > +{
> > > + RAMBlock *ramblock;
> > > +
> > > + if (!rbname) {
> > > + /* Reuse last RAMBlock */
> > > + ramblock = ms->last_req_rb;
> > > +
> > > + if (!ramblock) {
> > > + error_report("ram_save_queue_pages no previous block");
> > > + return -1;
> >
> > This should be an assert() shouldn't it?
> >
> > > + }
> > > + } else {
> > > + ramblock = ram_find_block(rbname);
> > > +
> > > + if (!ramblock) {
> > > + error_report("ram_save_queue_pages no block '%s'", rbname);
> > > + return -1;
> > > + }
> >
> > And maybe this one too - I would have expected the rb names to have
> > already been validated on the source machine at this stage.
>
> No to both:
> I've been trying to avoid asserts in migration outgoing code, because
> they shouldn't affect the state of your guest, so there's no reason
> to kill off what might still be a viable running guest just because
> migration failed.
Ah, ok, that makes sense. Maybe adding something to the error message
or a nearby comment indicating that if these happen it's certainly a
bug, not the result of some external problem?
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
pgpHMIr0c9EFO.pgp
Description: PGP signature
