From mboxrd@z Thu Jan  1 00:00:00 1970
Received: from eggs.gnu.org ([2001:4830:134:3::10]:44403)
	by lists.gnu.org with esmtp (Exim 4.71) (envelope-from <pl@kamp.de>)
	id 1WjnQP-0002m4-L1
	for qemu-devel@nongnu.org; Mon, 12 May 2014 06:25:42 -0400
Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)
	(envelope-from <pl@kamp.de>) id 1WjnQG-0001vE-9g
	for qemu-devel@nongnu.org; Mon, 12 May 2014 06:25:37 -0400
Received: from mx-v6.kamp.de ([2a02:248:0:51::16]:57778 helo=mx01.kamp.de)
	by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from <pl@kamp.de>)
	id 1WjnQF-0001qm-QV
	for qemu-devel@nongnu.org; Mon, 12 May 2014 06:25:28 -0400
Message-ID: <5370A193.7060309@kamp.de>
Date: Mon, 12 May 2014 12:25:23 +0200
From: Peter Lieven <pl@kamp.de>
MIME-Version: 1.0
References: <1399717549-10961-1-git-send-email-pl@kamp.de>
	<87lhu78fkw.fsf@elfo.mitica>
In-Reply-To: <87lhu78fkw.fsf@elfo.mitica>
Content-Type: text/plain; charset=ISO-8859-15
Content-Transfer-Encoding: 7bit
Subject: Re: [Qemu-devel] [PATCH] migration: catch unknown flags in ram_load
List-Id: <qemu-devel.nongnu.org>
List-Unsubscribe: <https://lists.nongnu.org/mailman/options/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=unsubscribe>
List-Archive: <http://lists.nongnu.org/archive/html/qemu-devel>
List-Post: <mailto:qemu-devel@nongnu.org>
List-Help: <mailto:qemu-devel-request@nongnu.org?subject=help>
List-Subscribe: <https://lists.nongnu.org/mailman/listinfo/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=subscribe>
To: quintela@redhat.com
Cc: pbonzini@redhat.com, qemu-stable@nongnu.org, qemu-devel@nongnu.org, dgilbert@redhat.com

Am 12.05.2014 12:19, schrieb Juan Quintela:
> Peter Lieven <pl@kamp.de> wrote:
>> 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.
>>
>> CC: qemu-stable@nongnu.org
>> Signed-off-by: Peter Lieven <pl@kamp.de>
> .....
>
> Once here, shouldn't be better to do this as:
>
> change do {} while ()   for while (true) {}
>
>>  
>> @@ -1121,6 +1119,9 @@ static int ram_load(QEMUFile *f, void *opaque, int version_id)
>>              }
>>          } else if (flags & RAM_SAVE_FLAG_HOOK) {
>>              ram_control_load_hook(f, flags);
>> +        } else if (!(flags & RAM_SAVE_FLAG_EOS)) {
>> +            ret = -EINVAL;
>> +            goto done;
>>          }
>>          error = qemu_file_get_error(f);
>>          if (error) {
>
>         } else if (flags & RAM_SAVE_FLAG_HOOK) {
>             ram_control_load_hook(f, flags);
> +       } else if (flags & RAM_SAVE_FLAG_EOS) {
> +           break;
> +       } else {
> +           ret = -EINVAL;
> +           goto done;
>         }
>           error = qemu_file_get_error(f);
>           if (error) {
>         }
>
>
> This way, we are checking RAM_SAVE_FLAG_EOS the same way than any other
> flag?  And we don't have to duplicate the FLAG_NAME?
Ok, I will send a v2.

>
> Unrelated to this patch, all the flags are a bitmap, but really, the
> ones that can be together are RAM_SAVE_FLAG_CONTINUE and the rest, all
> the others need to be alone.  I am telling this because we have used
> already 8 flags, and we are using the low bits of offset to save the
> flags, we have 10 flags?  Perhaps changing the last flag to mean that
> the low bits pass to be a counter?

Some better encoding would indeed be useful. I already thought
that we might run out of flags soon. We have 11 flags I think,
but there is not much space left. Reserving the last flag to indicate
that the lower 10 bits a are counter might be a good option.

Peter

>
> PD. No, I haven't investigated right now how RAM_SAVE_FLAG_HOOK works
> with all of this.
>
> Later, Juan.
>