Re: [Qemu-devel] [PATCH 6/6] migration: catch unknown flags in ram_load

[Eric Blake]

On 06/17/2014 03:31 AM, Peter Lieven wrote:
>> 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)) {

> -                            ret =  -EINVAL;
> +        if (flags & RAM_SAVE_FLAG_HOOK) {
> +            ram_control_load_hook(f, flags);

Can RAM_SAVE_FLAG_HOOK | RAM_SAVE_FLAG_EOS ever be true?  If so, you
abort the loop early. (One possibility: solve it by adding 'flags = 0;'
here.)

Also, this patch was harder to read than necessary, because it
reindented everything, and threw off git's diff algorithm.  Use 'git
diff --patience' to make it a bit more legible as a bulk delete/replace
instead of dividing into hunks around common but unrelated context
lines.  Or even rewrite it slightly so that the bulk of the code is
still at the same indentation, perhaps by rearranging this if statement
to instead appear inside the default label of the switch:

while (!ret && !(flags & RAM_SAVE_FLAG_EOS)) {
    flags = ...;
    switch (flags & ~RAM_SAVE_FLAG_CONTINUE) {
 ...
    default:
        if (flags & RAM_SAVE_FLAG_HOOK) {
            ram_control_load_hook(f, flags);
            flags = 0;
        } else {
            report error on unknown flags
        }
        break;
    }
    ret = qemu_file_get_error(f);
}

But yes, this is what I had in mind - propose it as a formal patch with
S-o-B, and I'll review it.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

signature.asc
Description: OpenPGP digital signature