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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.