From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:47710) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZqkWg-0002vG-9d for qemu-devel@nongnu.org; Mon, 26 Oct 2015 12:21:39 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZqkWd-0007my-5r for qemu-devel@nongnu.org; Mon, 26 Oct 2015 12:21:38 -0400 Received: from mx1.redhat.com ([209.132.183.28]:49600) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZqkWd-0007mt-0b for qemu-devel@nongnu.org; Mon, 26 Oct 2015 12:21:35 -0400 Date: Mon, 26 Oct 2015 16:21:28 +0000 From: "Dr. David Alan Gilbert" Message-ID: <20151026162127.GE2500@work-vm> References: <1443515898-3594-1-git-send-email-dgilbert@redhat.com> <1443515898-3594-26-git-send-email-dgilbert@redhat.com> <87y4exr7rv.fsf@neno.neno> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <87y4exr7rv.fsf@neno.neno> Subject: Re: [Qemu-devel] [PATCH v8 25/54] MIG_CMD_PACKAGED: Send a packaged chunk of migration stream List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Juan Quintela Cc: aarcange@redhat.com, liang.z.li@intel.com, qemu-devel@nongnu.org, luis@cs.umu.se, bharata@linux.vnet.ibm.com, amit.shah@redhat.com, pbonzini@redhat.com * Juan Quintela (quintela@redhat.com) wrote: > "Dr. David Alan Gilbert (git)" wrote: > > From: "Dr. David Alan Gilbert" > > > > 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 > > Reviewed-by: Amit Shah > > > Reviewed-by: Juan Quintela > > 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 / dgilbert@redhat.com / Manchester, UK