Bug 1377321 - libvirt should not pass image specific parameters (cache, AIO) to empty cdrom drives
Summary: libvirt should not pass image specific parameters (cache, AIO) to empty cdro...
Keywords:
Status: CLOSED DUPLICATE of bug 1672259
Alias: None
Product: Red Hat Enterprise Linux Advanced Virtualization
Classification: Red Hat
Component: libvirt
Version: 8.0
Hardware: x86_64
OS: Linux
unspecified
medium
Target Milestone: rc
: 8.0
Assignee: Peter Krempa
QA Contact: Han Han
URL:
Whiteboard:
Depends On: 1342999
Blocks: 1401400
TreeView+ depends on / blocked
 
Reported: 2016-09-19 12:37 UTC by Peter Krempa
Modified: 2021-07-10 14:03 UTC (History)
23 users (show)

Fixed In Version:
Doc Type: If docs needed, set a value
Doc Text:
Clone Of: 1342999
Environment:
Last Closed: 2019-06-04 11:57:05 UTC
Type: Bug
Target Upstream Version:


Attachments (Terms of Use)

Description Peter Krempa 2016-09-19 12:37:54 UTC
+++ This bug was initially created as a clone of Bug #1342999 +++

Description of problem:
Start qemu with an empty cdrom which with 'cache' option, qemu will prompt errors. 

Version-Release number of selected component (if applicable):
kernel: 3.10.0-394.el7.x86_64
qemu-kvm-rhev: qemu-kvm-rhev-2.6.0-3.el7.x86_64

How reproducible:
100%

Steps to Reproduce:
1. Start qemu with an empty cdrom, specify cache=none, qemu prompt errors:
# /usr/libexec/qemu-kvm \
-device virtio-scsi-pci,id=virtio_scsi_pci0,bus=pci.0,addr=06,disable-legacy=off,disable-modern=on \
-drive id=drive_cd1,if=none,media=cdrom,aio=native,snapshot=off,cache=none \
-device scsi-cd,id=cd1,drive=drive_cd1,bootindex=1,serial=X1CdkyW \

qemu-kvm: -drive id=drive_cd1,if=none,media=cdrom,aio=native,snapshot=off,cache=none: Must specify either driver or file

2. Libvirt hit same issue
<disk type='file' device='cdrom'>
   <driver name='qemu' type='raw' cache='none'/>
   <target dev='sdb' bus='scsi'/>
   <readonly/>
   <address type='drive' controller='0' bus='0' target='1' unit='0'/>
</disk>

# virsh start nfv_config2
error: Failed to start domain nfv_config2
error: internal error: process exited while connecting to monitor: 2016-06-06T08:24:50.944631Z qemu-kvm: -drive if=none,id=drive-scsi0-0-1-0,readonly=on,cache=none: Must specify either driver or file


Actual results:
qemu-kvm: -drive if=none,id=drive-scsi0-0-1-0,readonly=on,cache=none: Must specify either driver or file

Expected results:
1. 'cache' option should work well with empty cdrom.
2. Or qemu should prompt more clear log such as 'Don't specify cache option to an empty cdrom'.

Additional info:

--- Additional comment from John Snow on 2016-06-24 01:29:52 CEST ---

As much as it bothers me as well, this will be CLOSED NOTABUG.

The cache=x option is a shorthand for three other cache options:

#define BDRV_OPT_CACHE_WB       "cache.writeback"
#define BDRV_OPT_CACHE_DIRECT   "cache.direct"
#define BDRV_OPT_CACHE_NO_FLUSH "cache.no-flush"


The writeback option may be applied to an empty drive, but direct and no-flush may not be. By using the cache=x shorthand, you (inadvertently) set the other two options as well.

These options are then passed to blk_new_open, which rightly want a file to open.

Further, we are moving away from applying cache options to empty drives, so we will not be implementing the ability to apply cache options to empty drives (upstream).

The recommended workflow in the future is to use blockdev-add and specify cache options there (to open a new ISO) and then use x-blockdev-insert-medium to insert that disc into the drive.

We will continue to support (upstream) persistent cache options when using the legacy QMP/HMP commands "eject" and "change," but there are no plans to support persistent cache options on the CLI or through the newer QMP interface.

"OK, that's unfortunate, but the error message is very bad."

I know, and it's not easy to fix. It would definitely be an RFE, and not appropriate for qemu-kvm-rhev in 7.3.


As such, I will be closing this.


If anyone should by chance happen across this bug in the future and has strong feelings about the usability of the error messages, please file a bug for the upstream QEMU bug tracker (https://bugs.launchpad.net/qemu).

Thank you,
--John Snow

--- Additional comment from Fam Zheng on 2016-06-24 02:13:28 CEST ---

I think there is a valid configuration from virt-manager and libvirt that triggers this error and therefore QEMU fails to start.

In virt-manager, add an CDROM device, leave the path text box blank, and select a cache mode for it. The result xml in libvirt looks like:

    <disk type='file' device='cdrom'>
      <driver name='qemu' type='raw' cache='unsafe'/>
      <target dev='hda' bus='ide'/>
      <readonly/>
      <address type='drive' controller='0' bus='0' target='0' unit='0'/>
    </disk>

But QEMU refuses to start it:

$ virsh start rhel7.2
error: Failed to start domain rhel7.2
error: internal error: process exited while connecting to monitor: 2016-06-24T00:11:20.660157Z qemu-system-x86_64: -drive if=none,id=drive-ide0-0-0,readonly=on,cache=unsafe: Must specify either driver or file

We need a further look, John. Sorry about not noticing this earlier, but in fact I should say thanks to your question!


--- Additional comment from Josh Stone on 2016-06-30 19:02:56 CEST ---

But the media state of a cdrom drive is dynamic -- it may start empty, but what is supposed to happen with caching when media is inserted at runtime?  I'm not familiar with how libvirt actually communicates this to qemu, but is it able to specify the cache at insertion time?


--- Additional comment from Kevin Wolf on 2016-09-14 10:57:42 CEST ---

(In reply to Peter Krempa from comment #11)
> Unfortunately there's a problem. The 'change' command doesn't allow to set
> any cache mode.  Libvirt does not allow changing the cache mode during the
> lifetime but it depends on setting the mode at start. This is fine with
> loaded cdroms, but breaks the use case of starting a VM with an empty cdrom
> drive.

The assumption that you're making here is totally reasonable, but unfortunately
it is wrong. The 'change' command has never kept the cache mode, but always
reset it to 'writethrough'. You may call it a bug, but it's been there from the
beginning.

From the commit message of commit 91a097e74:

    Note that this forbids specifying the cache mode for empty drives. It
    didn't make sense anyway to specify it there, because it didn't have any
    effect.

It seems that we actually do keep the cache mode across 'change' now, but that
is a change that was made later than the removal of the 'cache' option for
empty drives (didn't check it in detail, but commit 5433c24f the earliest). And
I'm not sure if it was intentional...

Of course, for read-only devices like CD-ROMs the cache mode doesn't make that
much of a difference anyway, so it shouldn't matter either way. We just need to
figure out whether libvirt stops setting the option, or whether qemu goes back
to accepting and silently ignoring it for empty drives.

Comment 5 Peter Krempa 2018-03-08 15:56:47 UTC
*** Bug 1553255 has been marked as a duplicate of this bug. ***

Comment 7 Peter Krempa 2019-01-22 08:32:59 UTC
Upstream fixes this for non-blockdev cases by:

commit f80eae8c2ae0c62ecaa550ab6353cf871bb17d4e (HEAD -> master, origin/master, origin/HEAD)
Author: Peter Krempa <pkrempa>
Date:   Tue Jan 15 17:28:21 2019 +0100

    qemu: command: Don't format image properties for empty -drive
    
    If a -drive has no image, using image properties makes qemu whine that
    they should not be used.
    
    This patch stops formating cache/readonly/... for empty drives
    for the pre-blockdev syntax. Unfortunately those parameters can't be
    added later when inserting media, but on the other hand qemu will start
    with an empty drive.
    
    Since we already were able to start a VM with such config previously due
    to qemu ignoring them I've opted just to skip formatting them.
    Additionally with -blockdev support it will work as expected as the
    image properties will be formatted when adding the image itself which is
    not possible without it.

Comment 11 Peter Krempa 2019-02-04 12:30:04 UTC
One more commit is necessary to avoid regression with sVirt:

commit 6db0d033839807aec885e10b5a45748da016e261
Author: Peter Krempa <pkrempa>
Date:   Fri Feb 1 17:54:46 2019 +0100

    qemu: command: Don't skip 'readonly' and throttling info for empty drive
    
    In commit f80eae8c2ae I was too agresive in removing properties of
    -drive for empty drives. It turns out that qemu actually persists the
    state of 'readonly' and the throttling information even for the empty
    drive.
    
    Removing 'readonly' thus made qemu open any subsequent images added via
    the 'change' command as RW which was forbidden by selinux thanks to the
    restrictive sVirt label for readonly media.
    
    Fix this by formating the property again and bump the tests and leave a
    note detailing why the rest of the properties needs to be skipped.

Comment 13 Peter Krempa 2019-06-04 11:57:05 UTC

*** This bug has been marked as a duplicate of bug 1672259 ***


Note You need to log in before you can comment on or make changes to this bug.