Re: [Qemu-devel] [PATCH v3 4/9] ide: Drop redundant IDEState member bs

[Markus Armbruster <armbru@redhat.com>]

Markus Armbruster <armbru@redhat.com> writes:

> Stefano Stabellini <stefano.stabellini@eu.citrix.com> writes:
>
>> On Thu, 6 Feb 2014, Markus Armbruster wrote:
>>> Stefano Stabellini <stefano.stabellini@eu.citrix.com> writes:
>>> 
>>> > On Wed, 5 Feb 2014, Markus Armbruster wrote:
>>> >> [Note cc: Stefano]
>>> >> 
>>> >> Kevin Wolf <kwolf@redhat.com> writes:
>>> >> 
>>> >> > Am 30.01.2014 um 13:16 hat Markus Armbruster geschrieben:
>>> >> >> It's a copy of dev->conf.bs.  The copy was needed for non-qdevified
>>> >> >> controllers, which lacked dev.
>>> >> >> 
>>> >> >> Note how pci_piix3_xen_ide_unplug() cleared the copy.  We'll get back
>>> >> >> to that in the next few commits.
>>> >> >> 
>>> >> >> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>>> >> >
>>> >> > So this pci_piix3_xen_ide_unplug() is what happens here:
>>> >> >
>>> >> >> --- a/hw/ide/piix.c
>>> >> >> +++ b/hw/ide/piix.c
>>> >> >> @@ -169,12 +169,9 @@ static int pci_piix_ide_initfn(PCIDevice *dev)
>>> >> >>  
>>> >> >>  static int pci_piix3_xen_ide_unplug(DeviceState *dev)
>>> >> >>  {
>>> >> >> -    PCIIDEState *pci_ide;
>>> >> >>      DriveInfo *di;
>>> >> >>      int i = 0;
>>> >> >>  
>>> >> >> -    pci_ide = PCI_IDE(dev);
>>> >> >> -
>>> >> >>      for (; i < 3; i++) {
>>> >> >>          di = drive_get_by_index(IF_IDE, i);
>>> >> >>          if (di != NULL && !di->media_cd) {
>>> >> >> @@ -183,7 +180,6 @@ static int pci_piix3_xen_ide_unplug(DeviceState *dev)
>>> >> >>                  bdrv_detach_dev(di->bdrv, ds);
>>> >> >>              }
>>> >> >>              bdrv_close(di->bdrv);
>>> >> >> -            pci_ide->bus[di->bus].ifs[di->unit].bs = NULL;
>>> >> >>              drive_put_ref(di);
>>> >> >>          }
>>> >> >>      }
>>> >> >
>>> >> > Probably I'm just missing the obvious, but it seems to me that the
>>> >> > copy was cleared here, while the original was left around. This was
>>> >> > no problem because the original was unused anyway after device
>>> >> > initialisation.
>>> >> >
>>> >> > Now that the copy doesn't exist any more, we can't clear it, obviously,
>>> >> > but why don't we have to clear the original? Won't we still run the
>>> >> > "device is attached" code branches even though the device is really
>>> >> > unplugged?
>>> >> 
>>> >> It's been a while since I wrote this.  Almost 14 months, in fact.
>>> >> 
>>> >> No other IDE controller implements DeviceClass method unplug().  I can't
>>> >> remember why the normal code to detach the backend (release_drive())
>>> >> doesn't do here.  Stefano, can you help?
>>> >
>>> > Too long to be able to remember the exact details :-/
>>> > However if you point me to a branch I can give it a try and tell you if
>>> > the unplug still works as it used to.
>>> 
>>> Series trivially rebased to current master available at
>>> git://repo.or.cz/qemu/armbru.git branch kill-non-qdev-ide
>>
>> Unfortunately it doesn't work: I can see both sda and xvda being present
>> after the system has booted.
>
> Thank you for testing.  I'll try to clear the originals like Kevin
> suggested.  Hopefully, that'll pass your test.

Stefano, I updated the branch, please retest it.