From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:52091) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WuLth-0007OU-Jv for qemu-devel@nongnu.org; Tue, 10 Jun 2014 09:15:35 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1WuLtb-0005Mn-RY for qemu-devel@nongnu.org; Tue, 10 Jun 2014 09:15:29 -0400 Received: from mx1.redhat.com ([209.132.183.28]:28570) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WuLtb-0005Lo-Ir for qemu-devel@nongnu.org; Tue, 10 Jun 2014 09:15:23 -0400 Date: Tue, 10 Jun 2014 14:15:11 +0100 From: "Dr. David Alan Gilbert" Message-ID: <20140610131511.GC2475@work-vm> References: <1402392556-21844-1-git-send-email-pl@kamp.de> <53970052.4040402@redhat.com> <53970165.2050503@redhat.com> <53970364.8080108@kamp.de> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <53970364.8080108@kamp.de> 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: Peter Lieven Cc: amit.shah@redhat.com, pbonzini@redhat.com, qemu-devel@nongnu.org, quintela@redhat.com * Peter Lieven (pl@kamp.de) wrote: > 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. There's also RAM_SAVE_FLAG_CONTINUE that's used as a tweak to make for smaller headers. Dave > 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 -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK