| Message ID | 54DE89D6.7060404@suse.de |
|---|---|
| State | New |
| Headers | show |
* Alexander Graf (agraf@suse.de) wrote: <snip> > Can you please test whether the patch below makes things work for you again? The patch below fixes RDMA migration (same host); however, see comments. > Alex > > From ef6fde21007e62529799264f57a65c6bb3d0d414 Mon Sep 17 00:00:00 2001 > From: Alexander Graf <agraf@suse.de> > Date: Sat, 14 Feb 2015 00:21:01 +0100 > Subject: [PATCH] migration: Read JSON VM description on incoming migration > > One of the really nice things about the VM description format is that it > goes > over the wire when live migration is happening. Unfortunately QEMU today > closes > any socket once it sees VM_EOF coming, so we never give the VMDESC the > chance to > actually land on the wire. > > This patch makes QEMU read the description as well. This way we ensure that > anything wire tapping us in between will get the chance to also > interpret the > stream. > > Along the way we also fix virt tests that assume that number_bytes_sent > on the > sender side is equal to number_bytes_read which was true before the VMDESC > patches and is true again with this patch. > > Signed-off-by: Alexander Graf <agraf@suse.de> > > diff --git a/savevm.c b/savevm.c > index 8040766..ff4bead 100644 > --- a/savevm.c > +++ b/savevm.c > @@ -929,6 +929,7 @@ int qemu_loadvm_state(QEMUFile *f) > uint8_t section_type; > unsigned int v; > int ret; > + int file_error_after_eof = -1; > > if (qemu_savevm_state_blocked(&local_err)) { > error_report("%s", error_get_pretty(local_err)); > @@ -1034,6 +1035,22 @@ int qemu_loadvm_state(QEMUFile *f) > } > } > > + file_error_after_eof = qemu_file_get_error(f); > + > + /* > + * Try to read in the VMDESC section as well, so that dumping tools > that > + * intercept our migration stream have the chance to see it. > + */ > + if (qemu_get_byte(f) == QEMU_VM_VMDESCRIPTION) { You could use qemu_peek_byte for that? > + uint32_t size = qemu_get_be32(f); > + uint8_t *buf = g_malloc(size); > + > + if (buf) { > + qemu_get_buffer(f, buf, size); > + g_free(buf); > + } This is slightly dangerous; a malformed file could send you a huge value and get you to allocate lots of memory for no good reason. You could do some clever; but personally I'd just loop around a nice small buffer until it's gone. As mentioned on IRC; I'm still worried though that this is only a fix for loading on newer versions; migration to an older QEMU with the same machine type would fail. (Yes I know mythically that no one cares about this; but I do). Dave > + } > + > cpu_synchronize_all_post_init(); > > ret = 0; > @@ -1045,7 +1062,8 @@ out: > } > > if (ret == 0) { > - ret = qemu_file_get_error(f); > + /* We may not have a VMDESC section, so ignore relative errors */ > + ret = file_error_after_eof; > } > > return ret; -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
On 16.02.15 19:57, Dr. David Alan Gilbert wrote: > * Alexander Graf (agraf@suse.de) wrote: > > <snip> > >> Can you please test whether the patch below makes things work for you again? > > The patch below fixes RDMA migration (same host); however, see comments. > >> Alex >> >> From ef6fde21007e62529799264f57a65c6bb3d0d414 Mon Sep 17 00:00:00 2001 >> From: Alexander Graf <agraf@suse.de> >> Date: Sat, 14 Feb 2015 00:21:01 +0100 >> Subject: [PATCH] migration: Read JSON VM description on incoming migration >> >> One of the really nice things about the VM description format is that it >> goes >> over the wire when live migration is happening. Unfortunately QEMU today >> closes >> any socket once it sees VM_EOF coming, so we never give the VMDESC the >> chance to >> actually land on the wire. >> >> This patch makes QEMU read the description as well. This way we ensure that >> anything wire tapping us in between will get the chance to also >> interpret the >> stream. >> >> Along the way we also fix virt tests that assume that number_bytes_sent >> on the >> sender side is equal to number_bytes_read which was true before the VMDESC >> patches and is true again with this patch. >> >> Signed-off-by: Alexander Graf <agraf@suse.de> >> >> diff --git a/savevm.c b/savevm.c >> index 8040766..ff4bead 100644 >> --- a/savevm.c >> +++ b/savevm.c >> @@ -929,6 +929,7 @@ int qemu_loadvm_state(QEMUFile *f) >> uint8_t section_type; >> unsigned int v; >> int ret; >> + int file_error_after_eof = -1; >> >> if (qemu_savevm_state_blocked(&local_err)) { >> error_report("%s", error_get_pretty(local_err)); >> @@ -1034,6 +1035,22 @@ int qemu_loadvm_state(QEMUFile *f) >> } >> } >> >> + file_error_after_eof = qemu_file_get_error(f); >> + >> + /* >> + * Try to read in the VMDESC section as well, so that dumping tools >> that >> + * intercept our migration stream have the chance to see it. >> + */ >> + if (qemu_get_byte(f) == QEMU_VM_VMDESCRIPTION) { > > You could use qemu_peek_byte for that? It's what I had originally, but qemu_peek_byte() at the end of the day is the exact same as qemu_get_byte, but doesn't increment the internal buffer counter. So any error conditions that incur because the read failed still happen with peek_byte and are a lot less intuitive. > >> + uint32_t size = qemu_get_be32(f); >> + uint8_t *buf = g_malloc(size); >> + >> + if (buf) { >> + qemu_get_buffer(f, buf, size); >> + g_free(buf); >> + } > > This is slightly dangerous; a malformed file could send you a huge > value and get you to allocate lots of memory for no good reason. > > You could do some clever; but personally I'd just loop around a > nice small buffer until it's gone. Good idea. Will change. > As mentioned on IRC; I'm still worried though that this is only > a fix for loading on newer versions; migration to an older QEMU > with the same machine type would fail. > (Yes I know mythically that no one cares about this; but I do). Yeah, I guess I'll follow up with a fix to disable VMDESC submission on older versions, just to be on the safe side. Alex
On 16/02/2015 21:24, Alexander Graf wrote: >> As mentioned on IRC; I'm still worried though that this is only >> > a fix for loading on newer versions; migration to an older QEMU >> > with the same machine type would fail. >> > (Yes I know mythically that no one cares about this; but I do). > Yeah, I guess I'll follow up with a fix to disable VMDESC submission on > older versions, just to be on the safe side. Can you make it a capability? Paolo
On 16.02.15 22:06, Paolo Bonzini wrote: > > > On 16/02/2015 21:24, Alexander Graf wrote: >>> As mentioned on IRC; I'm still worried though that this is only >>>> a fix for loading on newer versions; migration to an older QEMU >>>> with the same machine type would fail. >>>> (Yes I know mythically that no one cares about this; but I do). >> Yeah, I guess I'll follow up with a fix to disable VMDESC submission on >> older versions, just to be on the safe side. > > Can you make it a capability? When did live migration start to have capability negotiation? :) Alex
On 16/02/2015 22:08, Alexander Graf wrote: > > Can you make it a capability? > When did live migration start to have capability negotiation? :) Only capability without negotiation. :) Negotiation is done above QEMU. Paolo
diff --git a/savevm.c b/savevm.c index 8040766..ff4bead 100644 --- a/savevm.c +++ b/savevm.c @@ -929,6 +929,7 @@ int qemu_loadvm_state(QEMUFile *f) uint8_t section_type; unsigned int v; int ret; + int file_error_after_eof = -1; if (qemu_savevm_state_blocked(&local_err)) { error_report("%s", error_get_pretty(local_err)); @@ -1034,6 +1035,22 @@ int qemu_loadvm_state(QEMUFile *f) } } + file_error_after_eof = qemu_file_get_error(f); + + /* + * Try to read in the VMDESC section as well, so that dumping tools that + * intercept our migration stream have the chance to see it. + */ + if (qemu_get_byte(f) == QEMU_VM_VMDESCRIPTION) { + uint32_t size = qemu_get_be32(f); + uint8_t *buf = g_malloc(size); + + if (buf) { + qemu_get_buffer(f, buf, size); + g_free(buf); + } + } + cpu_synchronize_all_post_init();