From: Peter Lieven <pl@kamp.de>
To: Eric Blake <eblake@redhat.com>,
Juan Quintela <quintela@redhat.com>,
qemu-devel@nongnu.org, Amit Shah <amit.shah@redhat.com>
Subject: Re: [Qemu-devel] [PATCH 6/6] migration: catch unknown flags in ram_load
Date: Tue, 17 Jun 2014 11:31:27 +0200 [thread overview]
Message-ID: <53A00AEF.2090304@kamp.de> (raw)
In-Reply-To: <539F2A0A.40401@redhat.com>
On 16.06.2014 19:31, Eric Blake wrote:
> On 06/15/2014 09:15 PM, Juan Quintela wrote:
>> From: Peter Lieven <pl@kamp.de>
>>
>> if a saved vm has unknown flags in the memory data qemu
>> currently simply ignores this flag and continues which
>> yields in an unpredictable result.
>>
>> This patch catches all unknown flags and aborts the
>> loading of the vm. Additionally error reports are thrown
>> if the migration aborts abnormally.
> This patch is a strict improvement, so I'm glad it went in. However, I
> still feel that we aren't doing a good job of silently ignoring
> unexpected combinations of flag bits, and had suggestions in the
> original thread on further followups that are worth having before the
> 2.1 release.
>
>> -
>> - if (flags & RAM_SAVE_FLAG_COMPRESS) {
>> + } else if (flags & RAM_SAVE_FLAG_COMPRESS) {
>> void *host;
> Among other things, switching from a chain of if-else to a switch might
> make it easier to document explicit supported combinations of flags and
> reject other values from an invalid stream.
>
Is this what you have in mind?
diff --git a/arch_init.c b/arch_init.c
index 8ddaf35..925cc66 100644
--- a/arch_init.c
+++ b/arch_init.c
@@ -1039,7 +1039,7 @@ void ram_handle_compressed(void *host, uint8_t ch, uint64_t size)
static int ram_load(QEMUFile *f, void *opaque, int version_id)
{
ram_addr_t addr;
- int flags, ret = 0;
+ int flags = 0, ret = 0;
static uint64_t seq_iter;
seq_iter++;
@@ -1048,97 +1048,96 @@ static int ram_load(QEMUFile *f, void *opaque, int version_id)
ret = -EINVAL;
}
- while (!ret) {
+ while (!ret && !(flags & RAM_SAVE_FLAG_EOS)) {
addr = qemu_get_be64(f);
flags = addr & ~TARGET_PAGE_MASK;
addr &= TARGET_PAGE_MASK;
- if (flags & RAM_SAVE_FLAG_MEM_SIZE) {
- /* Synchronize RAM block list */
- char id[256];
- ram_addr_t length;
- ram_addr_t total_ram_bytes = addr;
-
- while (total_ram_bytes) {
- RAMBlock *block;
- uint8_t len;
-
- len = qemu_get_byte(f);
- qemu_get_buffer(f, (uint8_t *)id, len);
- id[len] = 0;
- length = qemu_get_be64(f);
-
- QTAILQ_FOREACH(block, &ram_list.blocks, next) {
- if (!strncmp(id, block->idstr, sizeof(id))) {
- if (block->length != length) {
- error_report("Length mismatch: %s: " RAM_ADDR_FMT
- " in != " RAM_ADDR_FMT, id, length,
- block->length);
- ret = -EINVAL;
+ if (flags & RAM_SAVE_FLAG_HOOK) {
+ ram_control_load_hook(f, flags);
+ } else {
+ ram_addr_t total_ram_bytes;
+ void *host;
+ uint8_t ch;
+ switch (flags & ~RAM_SAVE_FLAG_CONTINUE) {
+ case RAM_SAVE_FLAG_MEM_SIZE:
+ /* Synchronize RAM block list */
+ total_ram_bytes = addr;
+ while (total_ram_bytes) {
+ RAMBlock *block;
+ uint8_t len;
+ char id[256];
+ ram_addr_t length;
+
+ len = qemu_get_byte(f);
+ qemu_get_buffer(f, (uint8_t *)id, len);
+ id[len] = 0;
+ length = qemu_get_be64(f);
+
+ QTAILQ_FOREACH(block, &ram_list.blocks, next) {
+ if (!strncmp(id, block->idstr, sizeof(id))) {
+ if (block->length != length) {
+ error_report("Length mismatch: %s: " RAM_ADDR_FMT
+ " in != " RAM_ADDR_FMT, id, length,
+ block->length);
+ ret = -EINVAL;
+ }
+ continue;
}
- break;
}
+ if (!block) {
+ error_report("Unknown ramblock \"%s\", cannot "
+ "accept migration", id);
+ ret = -EINVAL;
+ }
+ if (ret) {
+ continue;
+ }
+ total_ram_bytes -= length;
}
-
- if (!block) {
- error_report("Unknown ramblock \"%s\", cannot "
- "accept migration", id);
+ break;
+ case RAM_SAVE_FLAG_COMPRESS:
+ host = host_from_stream_offset(f, addr, flags);
+ if (!host) {
+ error_report("Illegal RAM offset " RAM_ADDR_FMT, addr);
ret = -EINVAL;
+ continue;
}
- if (ret) {
- break;
- }
-
- total_ram_bytes -= length;
- }
- } else if (flags & RAM_SAVE_FLAG_COMPRESS) {
- void *host;
- uint8_t ch;
-
- host = host_from_stream_offset(f, addr, flags);
- if (!host) {
- error_report("Illegal RAM offset " RAM_ADDR_FMT, addr);
- ret = -EINVAL;
+ ch = qemu_get_byte(f);
+ ram_handle_compressed(host, ch, TARGET_PAGE_SIZE);
break;
- }
-
- ch = qemu_get_byte(f);
- ram_handle_compressed(host, ch, TARGET_PAGE_SIZE);
- } else if (flags & RAM_SAVE_FLAG_PAGE) {
- void *host;
-
- host = host_from_stream_offset(f, addr, flags);
- if (!host) {
- error_report("Illegal RAM offset " RAM_ADDR_FMT, addr);
- ret = -EINVAL;
+ case RAM_SAVE_FLAG_PAGE:
+ host = host_from_stream_offset(f, addr, flags);
+ if (!host) {
+ error_report("Illegal RAM offset " RAM_ADDR_FMT, addr);
+ ret = -EINVAL;
+ continue;
+ }
+ qemu_get_buffer(f, host, TARGET_PAGE_SIZE);
break;
- }
-
- qemu_get_buffer(f, host, TARGET_PAGE_SIZE);
- } else if (flags & RAM_SAVE_FLAG_XBZRLE) {
- void *host = host_from_stream_offset(f, addr, flags);
- if (!host) {
- error_report("Illegal RAM offset " RAM_ADDR_FMT, addr);
- ret = -EINVAL;
+ case RAM_SAVE_FLAG_XBZRLE:
+ host = host_from_stream_offset(f, addr, flags);
+ if (!host) {
+ error_report("Illegal RAM offset " RAM_ADDR_FMT, addr);
+ ret = -EINVAL;
+ continue;
+ }
+ if (load_xbzrle(f, addr, host) < 0) {
+ error_report("Failed to decompress XBZRLE page at "
+ RAM_ADDR_FMT, addr);
+ ret = -EINVAL;
+ continue;
+ }
break;
- }
-
- if (load_xbzrle(f, addr, host) < 0) {
- error_report("Failed to decompress XBZRLE page at "
- RAM_ADDR_FMT, addr);
+ case RAM_SAVE_FLAG_EOS:
+ /* normal exit */
+ continue;
+ default:
+ error_report("Unknown migration flags: %#x", flags);
ret = -EINVAL;
- break;
+ continue;
}
- } else if (flags & RAM_SAVE_FLAG_HOOK) {
- ram_control_load_hook(f, flags);
- } else if (flags & RAM_SAVE_FLAG_EOS) {
- /* normal exit */
- break;
- } else {
- error_report("Unknown migration flags: %#x", flags);
- ret = -EINVAL;
- break;
}
ret = qemu_file_get_error(f);
}
next prev parent reply other threads:[~2014-06-17 9:31 UTC|newest]
Thread overview: 11+ messages / expand[flat|nested] mbox.gz Atom feed top
2014-06-16 3:15 [Qemu-devel] [PULL 0/6] migration queue Juan Quintela
2014-06-16 3:15 ` [Qemu-devel] [PATCH 1/6] savevm: Remove all the unneeded version_minimum_id_old (ppc) Juan Quintela
2014-06-16 3:15 ` [Qemu-devel] [PATCH 2/6] savevm: Remove all the unneeded version_minimum_id_old (x86) Juan Quintela
2014-06-16 3:15 ` [Qemu-devel] [PATCH 3/6] vmstate: Refactor opening of files Juan Quintela
2014-06-16 3:15 ` [Qemu-devel] [PATCH 4/6] migration: Increase default max_downtime from 30ms to 300ms Juan Quintela
2014-06-16 3:15 ` [Qemu-devel] [PATCH 5/6] rdma: Fix block during rdma migration Juan Quintela
2014-06-16 3:15 ` [Qemu-devel] [PATCH 6/6] migration: catch unknown flags in ram_load Juan Quintela
2014-06-16 17:31 ` Eric Blake
2014-06-17 9:31 ` Peter Lieven [this message]
2014-06-17 20:32 ` Eric Blake
2014-06-16 10:39 ` [Qemu-devel] [PULL 0/6] migration queue Peter Maydell
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=53A00AEF.2090304@kamp.de \
--to=pl@kamp.de \
--cc=amit.shah@redhat.com \
--cc=eblake@redhat.com \
--cc=qemu-devel@nongnu.org \
--cc=quintela@redhat.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).