From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:49989) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WuLnX-0004R5-P4 for qemu-devel@nongnu.org; Tue, 10 Jun 2014 09:09:16 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1WuLnO-0002dK-ET for qemu-devel@nongnu.org; Tue, 10 Jun 2014 09:09:07 -0400 Received: from mx-v6.kamp.de ([2a02:248:0:51::16]:42736 helo=mx01.kamp.de) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WuLnO-0002ZT-3s for qemu-devel@nongnu.org; Tue, 10 Jun 2014 09:08:58 -0400 Message-ID: <53970364.8080108@kamp.de> Date: Tue, 10 Jun 2014 15:08:52 +0200 From: Peter Lieven MIME-Version: 1.0 References: <1402392556-21844-1-git-send-email-pl@kamp.de> <53970052.4040402@redhat.com> <53970165.2050503@redhat.com> In-Reply-To: <53970165.2050503@redhat.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCHv4] migration: catch unknown flags in ram_load List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Eric Blake , qemu-devel@nongnu.org Cc: amit.shah@redhat.com, pbonzini@redhat.com, dgilbert@redhat.com, quintela@redhat.com On 10.06.2014 15:00, Eric Blake wrote: > On 06/10/2014 06:55 AM, Eric Blake wrote: >> On 06/10/2014 03:29 AM, Peter Lieven 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. Additionally error reports are thrown >>> if the migration aborts abnormally. >>> >>> } else if (flags & RAM_SAVE_FLAG_HOOK) { >>> ram_control_load_hook(f, flags); >>> + } else if (flags & RAM_SAVE_FLAG_EOS) { >> Umm, is the migration format specifically documented as having at most >> one flag per operation, or is it valid to send two flags at once? That >> is, can I send RAM_SAVE_FLAG_XBZRLE | RAM_SAVE_FLAG_HOOK on a single >> packet? Should we be flagging streams that send unexpected flag >> combinations as invalid, even when each flag is in isolation okay, >> rather than the current behavior of silently prioritizing one flag and >> ignoring the other? > For that matter, would it be better to change the if-tree into a switch, > so that the default case catches unsupported combinations? > > switch (flags) { > ... > case RAM_SAVE_FLAG_HOOK: ... > case RAM_SAVE_FLAG_EOS: ... > default: report unsupported flags value > } > The RAM_SAVE_FLAG_HOOK is the only real flag. It seems that the flag value is used at least somewhere in the code of RDMA. For that matter, we could handle the hook separately and everything else in the switch statement. This would immediately solve the issue of the very restricted space for the flags as we could use everything below RAM_SAVE_FLAG_HOOK as counter immediately. Looking at the code I further see that the hook function is made to return an error code which is not checked at the moment. Peter