1. Patchew
  2. Libvirt
  3. View series

[libvirt] [PATCH] qemu: command: Don't skip 'readonly' and throttling info for empty drive

Peter Krempa posted 1 patch 5 years, 9 months ago
Test syntax-check passed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/libvirt tags/patchew/eade103407d61d83273318f7893619f408e5fbba.1549040397.git.pkrempa@redhat.com
src/qemu/qemu_command.c                        | 18 +++++++++++++-----
tests/qemuxml2argvdata/disk-cdrom.args         |  4 ++--
.../disk-cdrom.x86_64-2.12.0.args              |  4 ++--
.../disk-cdrom.x86_64-latest.args              |  4 ++--
4 files changed, 19 insertions(+), 11 deletions(-)
[libvirt] [PATCH] qemu: command: Don't skip 'readonly' and throttling info for empty drive
Posted by Peter Krempa 5 years, 9 months ago 
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.

Signed-off-by: Peter Krempa <pkrempa@redhat.com>
---
 src/qemu/qemu_command.c                        | 18 +++++++++++++-----
 tests/qemuxml2argvdata/disk-cdrom.args         |  4 ++--
 .../disk-cdrom.x86_64-2.12.0.args              |  4 ++--
 .../disk-cdrom.x86_64-latest.args              |  4 ++--
 4 files changed, 19 insertions(+), 11 deletions(-)

diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index a59583fb75..6d3aa69569 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -1764,10 +1764,18 @@ qemuBuildDriveStr(virDomainDiskDefPtr disk,
         }
     }

+    if (disk->src->readonly)
+        virBufferAddLit(&opt, ",readonly=on");
+
+    /* qemu rejects some parameters for an empty -drive, so we need to skip
+     * them in that case:
+     * cache: modifies properties of the format driver which is not present
+     * copy_on_read: really only works for floppies
+     * discard: modifies properties of format driver
+     * detect_zeroes: works but really depends on discard so it's useless
+     * iomode: setting it to 'native' requires a specific cache mode
+     */
     if (!virStorageSourceIsEmpty(disk->src)) {
-        if (disk->src->readonly)
-            virBufferAddLit(&opt, ",readonly=on");
-
         if (disk->cachemode) {
             virBufferAsprintf(&opt, ",cache=%s",
                               qemuDiskCacheV2TypeToString(disk->cachemode));
@@ -1792,10 +1800,10 @@ qemuBuildDriveStr(virDomainDiskDefPtr disk,
             virBufferAsprintf(&opt, ",aio=%s",
                               virDomainDiskIoTypeToString(disk->iomode));
         }
-
-        qemuBuildDiskThrottling(disk, &opt);
     }

+    qemuBuildDiskThrottling(disk, &opt);
+
     if (virBufferCheckError(&opt) < 0)
         goto error;

diff --git a/tests/qemuxml2argvdata/disk-cdrom.args b/tests/qemuxml2argvdata/disk-cdrom.args
index a9f60aa477..4823ae82de 100644
--- a/tests/qemuxml2argvdata/disk-cdrom.args
+++ b/tests/qemuxml2argvdata/disk-cdrom.args
@@ -27,7 +27,7 @@ bootindex=1 \
 -drive file=/root/boot.iso,format=raw,if=none,id=drive-ide0-0-1,media=cdrom,\
 readonly=on \
 -device ide-drive,bus=ide.0,unit=1,drive=drive-ide0-0-1,id=ide0-0-1 \
--drive if=none,id=drive-ide0-1-0,media=cdrom \
+-drive if=none,id=drive-ide0-1-0,media=cdrom,readonly=on \
 -device ide-drive,bus=ide.1,unit=0,drive=drive-ide0-1-0,id=ide0-1-0 \
--drive if=none,id=drive-ide0-1-1,media=cdrom \
+-drive if=none,id=drive-ide0-1-1,media=cdrom,readonly=on \
 -device ide-drive,bus=ide.1,unit=1,drive=drive-ide0-1-1,id=ide0-1-1
diff --git a/tests/qemuxml2argvdata/disk-cdrom.x86_64-2.12.0.args b/tests/qemuxml2argvdata/disk-cdrom.x86_64-2.12.0.args
index a39d920f67..2fe84177b8 100644
--- a/tests/qemuxml2argvdata/disk-cdrom.x86_64-2.12.0.args
+++ b/tests/qemuxml2argvdata/disk-cdrom.x86_64-2.12.0.args
@@ -28,10 +28,10 @@ file=/tmp/lib/domain--1-QEMUGuest1/master-key.aes \
 -device ide-hd,bus=ide.0,unit=0,drive=drive-ide0-0-0,id=ide0-0-0,bootindex=1 \
 -drive file=/root/boot.iso,format=raw,if=none,id=drive-ide0-0-1,readonly=on \
 -device ide-cd,bus=ide.0,unit=1,drive=drive-ide0-0-1,id=ide0-0-1 \
--drive if=none,id=drive-ide0-1-0 \
+-drive if=none,id=drive-ide0-1-0,readonly=on \
 -device ide-cd,bus=ide.1,unit=0,drive=drive-ide0-1-0,id=ide0-1-0,\
 write-cache=on \
--drive if=none,id=drive-ide0-1-1 \
+-drive if=none,id=drive-ide0-1-1,readonly=on \
 -device ide-cd,bus=ide.1,unit=1,drive=drive-ide0-1-1,id=ide0-1-1 \
 -sandbox on,obsolete=deny,elevateprivileges=deny,spawn=deny,\
 resourcecontrol=deny \
diff --git a/tests/qemuxml2argvdata/disk-cdrom.x86_64-latest.args b/tests/qemuxml2argvdata/disk-cdrom.x86_64-latest.args
index 029ae23dfa..9b9451f435 100644
--- a/tests/qemuxml2argvdata/disk-cdrom.x86_64-latest.args
+++ b/tests/qemuxml2argvdata/disk-cdrom.x86_64-latest.args
@@ -28,10 +28,10 @@ file=/tmp/lib/domain--1-QEMUGuest1/master-key.aes \
 -device ide-hd,bus=ide.0,unit=0,drive=drive-ide0-0-0,id=ide0-0-0,bootindex=1 \
 -drive file=/root/boot.iso,format=raw,if=none,id=drive-ide0-0-1,readonly=on \
 -device ide-cd,bus=ide.0,unit=1,drive=drive-ide0-0-1,id=ide0-0-1 \
--drive if=none,id=drive-ide0-1-0 \
+-drive if=none,id=drive-ide0-1-0,readonly=on \
 -device ide-cd,bus=ide.1,unit=0,drive=drive-ide0-1-0,id=ide0-1-0,\
 write-cache=on \
--drive if=none,id=drive-ide0-1-1 \
+-drive if=none,id=drive-ide0-1-1,readonly=on \
 -device ide-cd,bus=ide.1,unit=1,drive=drive-ide0-1-1,id=ide0-1-1 \
 -sandbox on,obsolete=deny,elevateprivileges=deny,spawn=deny,\
 resourcecontrol=deny \
-- 
2.20.1

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] qemu: command: Don't skip 'readonly' and throttling info for empty drive
Posted by Daniel P. Berrangé 5 years, 9 months ago 
On Fri, Feb 01, 2019 at 06:00:37PM +0100, Peter Krempa wrote:
> 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.
> 
> Signed-off-by: Peter Krempa <pkrempa@redhat.com>
> ---
>  src/qemu/qemu_command.c                        | 18 +++++++++++++-----
>  tests/qemuxml2argvdata/disk-cdrom.args         |  4 ++--
>  .../disk-cdrom.x86_64-2.12.0.args              |  4 ++--
>  .../disk-cdrom.x86_64-latest.args              |  4 ++--
>  4 files changed, 19 insertions(+), 11 deletions(-)

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Patchew 2.1-devel-b67e4c2

© 2016 - 2024 Red Hat, Inc.