Re: [Qemu-devel] [BUGFIX][PATCH for 2.2 1/1] hw/ide/core.c: Prevent SIGSEGV during migration

[Dr. David Alan Gilbert]

* Markus Armbruster (address@hidden) wrote:
> Don Slutz <address@hidden> writes:
> 
> > On 11/19/14 07:29, Markus Armbruster wrote:
> >> Don Slutz <address@hidden> writes:
> >>
> >>> The other callers to blk_set_enable_write_cache() in this file
> >>> already check for s->blk == NULL.
> >>>
> >>> Signed-off-by: Don Slutz <address@hidden>
> >>> ---
> >>>
> >>> I think this is a bugfix that should be back ported to stable
> >>> releases.
> >>>
> >>> I also think this should be done in xen's copy of QEMU for 4.5 with
> >>> back port(s) to active stable releases.
> >>>
> >>> Note: In 2.1 and earlier the routine is
> >>> bdrv_set_enable_write_cache(); variable is s->bs.
> >> Got a reproducer?
> >
> > yes.  Migrating a guest from xen 4.2 or 4.3 to xen 4.4 (or 4.5-unstable) on
> > CentOS 6.3 with xen_emul_unplug=unnecessary and no cdrom defined.
> >
> >
> >>
> >> I'm asking because I believe s->identify_set implies s->blk.
> >> s->identify_set is initialized to zero, and gets set to non-zero exactly
> >> on the first successful IDENTIFY DEVICE or IDENTIFY PACKET DEVICE, in
> >> ide_identify(), ide_atapi_identify() or ide_cfata_identify(),
> >> respectively.  Only called via cmd_identify() / cmd_identify_packet()
> >> via ide_exec_cmd().  The latter immediately fails when !s->blk:
> >>
> >>      s = idebus_active_if(bus);
> >>      /* ignore commands to non existent slave */
> >>      if (s != bus->ifs && !s->blk) {
> >>          return;
> >>      }
> >
> > I do think that you are right.  I have now spent more time on why I am
> > seeing this.
> >
> >
> >> Even if I'm right, your patch is fine, because it makes this spot more
> >> obviously correct, and consistent with the other uses of
> >> blk_set_enable_write_cache().  The case for stable is weak, though.
> >>
> >
> > I had not fully tracked down what is happening before sending the bugfix.
> > I have now done more debugging, and have tracked it down to xen 4.4
> > now using "-nodefaults" with QEMU.
> >
> > I needed to add output to QEMU to track this down because I have long
> > command lines...
> >
> > (all I get for ps -ef):
> [...]
> >
> >
> > Which is missing that option.
> >
> > The ide that was aborting in this case is the cdrom at hdc that is added
> > if you do not specify "-nodefaults".
> >
> > Since this is a "changed" machine config, I am no longer as sure as what
> > versions this needs to be in.
> >
> > If I put my QEMU hat on, it does not look like a back port is needed.
> > However
> > for xen it would be nice.
> >
> > I do not know how the QEMU community feels about migration from a config
> > without "-nodefaults" to one with "-nodefaults" as the only difference.
> 
> So you have a CD-ROM on the source, but not on the destination?
> 
> That can't work.  I guess it broke for you in an unusual way (target
> crashes) rather than the usual way (target rejects migration data for a
> device it doesn't have) due to our convoluted IDE data structures.  With
> your patch applied it should break the usual way.  Does it?
> 
> Management tools should use -nodefaults.  But if it mixes default and
> -nodefaults in migration, recreating the stuff it got by default but
> doesn't get with -nodefaults is its own responsibility.

Well, mostly - we wouldn't expect a migration to work if the source/dest
didn't match exactly; but QEMU shouldn't seg.

Dave

--
Dr. David Alan Gilbert / address@hidden / Manchester, UK