qemu-devel
[Top][All Lists]
Advanced

[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



reply via email to

[Prev in Thread] Current Thread [Next in Thread]