Re: [Qemu-devel] [PATCH v4 36/47] Page request: Process incoming page request

[David Gibson]

On Mon, Nov 17, 2014 at 07:07:33PM +0000, Dr. David Alan Gilbert wrote:
> * David Gibson (address@hidden) wrote:
> > On Fri, Oct 03, 2014 at 06:47:42PM +0100, Dr. David Alan Gilbert (git) 
> > wrote:
> > > From: "Dr. David Alan Gilbert" <address@hidden>
> > > 
> > > On receiving MIG_RPCOMM_REQPAGES look up the address and
> > > queue the page.
> > > 
> > > Signed-off-by: Dr. David Alan Gilbert <address@hidden>
> > > ---
> > >  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

pgp5MlCZuewUF.pgp
Description: PGP signature