Bug 1326660

Summary: Update-device fail to update floppy with an unknown error
Product: Red Hat Enterprise Linux 7 Reporter: Pei Zhang <pzhang>
Component: libvirtAssignee: Peter Krempa <pkrempa>
Status: CLOSED ERRATA QA Contact: Virtualization Bugs <virt-bugs>
Severity: medium Docs Contact:
Priority: medium    
Version: 7.3CC: crobinso, dyuan, hhan, hreitz, lmen, mzhan, rbalakri, xuzhang
Target Milestone: rc   
Target Release: ---   
Hardware: Unspecified   
OS: Unspecified   
Whiteboard:
Fixed In Version: libvirt-1.3.5-1.el7 Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of: Environment:
Last Closed: 2016-11-03 18:42:07 UTC Type: Bug
Regression: --- Mount Type: ---
Documentation: --- CRM:
Verified Versions: Category: ---
oVirt Team: --- RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: --- Target Upstream Version:
Embargoed:

Description Pei Zhang 2016-04-13 10:05:53 UTC
Description of problem:
Update-device fail to update floppy with unknown error

Version-Release number of selected component (if applicable):
libvirt-1.3.3-1.el7.x86_64
qemu-kvm-rhev-2.5.0-4.el7.x86_64

How reproducible:
100%

Steps to Reproduce:

1. define and start with floppy device withour source
# virsh dumpxml r72 |grep floppy -A 9
    <disk type='file' device='floppy'>
      <driver name='qemu' type='raw'/>
      <backingStore/>
      <target dev='fda' bus='fdc'/>
      <alias name='fdc0-0-0'/>
      <address type='drive' controller='0' bus='0' target='0' unit='0'/>
    </disk>

2. update floppy device
# cat floppy.xml
<disk type='file' device='floppy'>
        <driver name='qemu' type='raw'/>
        <source file='/mnt/at-dt/change-media/a.iso'/>
        <target dev='fda' bus='fdc'/>
        <address type='drive' controller='0' bus='0' target='0' unit='0'/>
</disk>

3. update device
# virsh update-device r72 floppy.xml
error: Failed to update device from floppy.xml
error: An error occurred, but the cause is unknown


Actual results:
As step 3, fail update with unknown error.

Expected results:
update successfully.

Additional info:

Comment 2 Peter Krempa 2016-05-02 07:13:36 UTC
commit a34faf33011c5c0d7b47ee0849bf1e11635e17c5
Author: Peter Krempa <pkrempa>
Date:   Fri Apr 29 13:38:51 2016 +0200

    qemu: process: Refresh ejectable media tray state on VM start
    
    Empty floppy drives start with tray in "open" state and libvirt did not
    refresh it after startup. The code that inserts media into the tray then
    waited until the tray was open before inserting the media and thus
    floppies could not be inserted.

Comment 3 Cole Robinson 2016-05-17 12:27:47 UTC
Note this doesn't work again with qemu 2.6.0. I bisected to:

commit abb3e55b5b718d6392441f56ba0729a62105ac56
Author: Max Reitz <mreitz>
Date:   Fri Jan 29 20:49:12 2016 +0100

    Revert "hw/block/fdc: Implement tray status"

The cover letter here has an explanation:
http://lists.nongnu.org/archive/html/qemu-devel/2016-01/msg04471.html

So libvirt is waiting for a TRAY event that qemu is never going to send

Comment 4 Hanna Czenczek 2016-05-18 12:26:55 UTC
Apart from waiting for a TRAY_MOVED event that will never occur, there is the issue of Peter's commit making use of the "tray_open" status of the device. However, the change Cole presented above did not only make those events disappear but also the "tray_open" field in the query-block response for floppy disk drives. Instead, you could check query-block's "inserted" field. Its existence is equivalent to the presence of a medium in the drive.

I have not checked it, but I think that this should even be true for previous versions of qemu. The only problem is that for devices with an actual tray it is possible for no medium to be inserted but the tray to be closed. I'm not sure whether this was possible before qemu 2.5, though.

Maybe the presence of the "tray_open" field can help a bit. libvirt claims that "'tray_open' is presented iff medium is ejected" which is not true: It is always presented if a device supports removable media, and as far as I can see this has been so since its introduction (commit e4def80b). Starting from 2.6 this changed and now it is only presented iff a device actually has a tray.

In 2.5, floppy disk drives basically had a tray and therefore apparently can be handled just like any other tray device. So I guess libvirt could make a distinction based on the presence of the "tray_open" field: If it is present, treat the device just like it has always been. If it is not (but it supports removable media as indicated by the "removable" field), then treat it as a tray-less slot device (i.e. what floppy disk and SD drives are) that will not send TRAY_MOVED events nor have a "tray_open" status.

Max

Comment 5 Peter Krempa 2016-05-18 13:50:23 UTC
(In reply to Max Reitz from comment #4)
> Apart from waiting for a TRAY_MOVED event that will never occur, there is
> the issue of Peter's commit making use of the "tray_open" status of the
> device. However, the change Cole presented above did not only make those

My code just refreshed the state of the tray since for empty floppies it was open and for cdroms it was closed by default. The real bug is still in the logic that is handling the re-issue of the legacy 'eject' command.

> events disappear but also the "tray_open" field in the query-block response
> for floppy disk drives. Instead, you could check query-block's "inserted"
> field. Its existence is equivalent to the presence of a medium in the drive.

In libvirt we store the info in two places. One is that the medium is empty and one is that the tray is open. In case when the tray is open we consider that it's empty. With the possibility to retract the cdrom via the guest this might not be entirely a good model.

 
> I have not checked it, but I think that this should even be true for
> previous versions of qemu. The only problem is that for devices with an
> actual tray it is possible for no medium to be inserted but the tray to be
> closed. I'm not sure whether this was possible before qemu 2.5, though.

As we are using just the legacy 'eject' command this should not be a problem though. I'll need to check if we remove the source if we get the TRAY_MOVED event or not. That might be still tricky.

> 
> Maybe the presence of the "tray_open" field can help a bit. libvirt claims
> that "'tray_open' is presented iff medium is ejected" which is not true: It
> is always presented if a device supports removable media, and as far as I
> can see this has been so since its introduction (commit e4def80b). Starting
> from 2.6 this changed and now it is only presented iff a device actually has
> a tray.

Perhaps time to use 3-state logic rather than 2-state.

> 
> In 2.5, floppy disk drives basically had a tray and therefore apparently can
> be handled just like any other tray device. So I guess libvirt could make a
> distinction based on the presence of the "tray_open" field: If it is
> present, treat the device just like it has always been. If it is not (but it
> supports removable media as indicated by the "removable" field), then treat
> it as a tray-less slot device (i.e. what floppy disk and SD drives are) that
> will not send TRAY_MOVED events nor have a "tray_open" status.

More fun. SD-drives are not handled as removable in libvirt :)

Comment 6 Peter Krempa 2016-06-02 10:15:29 UTC
Following commits should fix it properly including the possible presence of the tray:

commit 72a7ff6b507bcf389cc493ac0ba07d32de266d6e
Author: Peter Krempa <pkrempa>
Date:   Thu May 19 15:30:12 2016 +0200

    qemu: hotplug: wait for the tray to eject only for drives with a tray
    
    Use the detected tray presence flag to trigger the tray waiting code
    only if the given storage device in qemu reports to have a tray.
    
    This is necessary as the floppy device lost it's tray as of qemu commit:
    
    commit abb3e55b5b718d6392441f56ba0729a62105ac56
    Author: Max Reitz <mreitz>
    Date:   Fri Jan 29 20:49:12 2016 +0100
    
        Revert "hw/block/fdc: Implement tray status"

commit 2e75da42e41af0cd48ca6f75d0606b40a366cc54
Author: Peter Krempa <pkrempa>
Date:   Mon May 23 16:32:06 2016 +0200

    qemu: hotplug: Fix error reported when cdrom tray is locked
    
    Commit 1fad65d49aae364576bd91352a001249510f8d4e used a really big hammer
    and overwrote the error message that might be reported by qemu if the
    tray is locked. Fix it by reporting the error only if no error is
    currently set.
    
    Error after commit mentioned above:
    error: internal error: timed out waiting for disk tray status update
    
    New error:
    error: internal error: unable to execute QEMU command 'eject': Tray of
    device 'drive-ide0-0-0' is not open

commit 0aa19f35e0f3c1712f2569986d4d0a93b488c35c
Author: Peter Krempa <pkrempa>
Date:   Mon May 23 14:50:17 2016 +0200

    qemu: hotplug: Extract code for waiting for tray eject
    
    The code grew rather convoluted. Extract it to a separate function.

commit 894dc85fd1ebcd63d8c897b355c550e68a5f432d
Author: Peter Krempa <pkrempa>
Date:   Thu May 19 15:29:02 2016 +0200

    qemu: process: Fix and improve disk data extraction
    
    Extract information for all disks and update tray state and source only
    for removable drives. Additionally store whether a drive is removable
    and whether it has a tray.

commit d9bee413ade28e1e43ef222c7aaaa3c6d6fda0f1
Author: Peter Krempa <pkrempa>
Date:   Mon May 23 14:00:35 2016 +0200

    qemu: Move and rename qemuDomainCheckEjectableMedia to qemuProcessRefreshDisks
    
    Move it to a more sane place since it's refreshing data about disks.

commit f1690dc3d7934bf70f4fbc84d55bf210276c6f27
Author: Peter Krempa <pkrempa>
Date:   Thu May 19 14:57:41 2016 +0200

    qemu: Extract more information about qemu drives
    
    Extract whether a given drive has a tray and whether there is no image
    inserted.
    
    Negative logic for the image insertion is chosen so that the flag is set
    only if we are certain of the fact.

commit 5f963d89b1220460fadb1bf6fc347d26b311c1b2
Author: Peter Krempa <pkrempa>
Date:   Fri May 20 07:21:04 2016 +0200

    qemu: Move struct qemuDomainDiskInfo to qemu_domain.h

Comment 8 lijuan men 2016-06-20 09:47:29 UTC
verify the bug

version:
libvirt-1.3.5-1.el7.x86_64
qemu-kvm-rhev-2.6.0-7.el7.x86_64
kernel-3.10.0-445.el7

steps:
1. define and start with floppy device without source
 <disk type='file' device='floppy'>
      <driver name='qemu' type='raw'/>
      <backingStore/>
      <target dev='fda' bus='fdc'/>
      <alias name='fdc0-0-0'/>
      <address type='drive' controller='0' bus='0' target='0' unit='0'/>
    </disk>

2. prepare xml file
[root@localhost ~]# cat floppy.xml
<disk type='file' device='floppy'>
        <driver name='qemu' type='raw'/>
        <source file='/var/lib/libvirt/images/temp1.iso'/>
        <target dev='fda' bus='fdc'/>
        <address type='drive' controller='0' bus='0' target='0' unit='0'/>
</disk>


3. update device
[root@localhost ~]# virsh update-device bios floppy.xml 
Device updated successfully

[root@localhost ~]# virsh domblklist bios
Target     Source
------------------------------------------------
sda        /var/lib/libvirt/images/admin.qcow2
fda        /var/lib/libvirt/images/temp1.iso

Comment 10 errata-xmlrpc 2016-11-03 18:42:07 UTC
Since the problem described in this bug report should be
resolved in a recent advisory, it has been closed with a
resolution of ERRATA.

For information on the advisory, and where to find the updated
files, follow the link below.

If the solution does not work for you, open a new bug report.

https://rhn.redhat.com/errata/RHSA-2016-2577.html