Re: [Qemu-devel] [PATCHv4] migration: catch unknown flags in ram_load

["Dr. David Alan Gilbert" <dgilbert@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