[PATCH 4/5] qemu: add QEMU_CAPS_VIRTIO_BLK_QUEUE_SIZE capability
To support virtio-blk queue-size option, this commit adds capability detection
to the option.
Signed-off-by: Hiroki Narukawa
---
src/qemu/qemu_capabilities.c | 2 ++
src/qemu/qemu_capabilities.h | 1 +
tests/qemucapabilitiesdata/caps_2.12.0.aarch64.xml | 1 +
tests/qemucapabilitiesdata/caps_2.12.0.ppc64.xml | 1 +
tests/qemucapabilitiesdata/caps_2.12.0.s390x.xml | 1 +
tests/qemucapabilitiesdata/caps_2.12.0.x86_64.xml | 1 +
tests/qemucapabilitiesdata/caps_3.0.0.ppc64.xml| 1 +
tests/qemucapabilitiesdata/caps_3.0.0.s390x.xml| 1 +
tests/qemucapabilitiesdata/caps_3.0.0.x86_64.xml | 1 +
tests/qemucapabilitiesdata/caps_3.1.0.ppc64.xml| 1 +
tests/qemucapabilitiesdata/caps_3.1.0.x86_64.xml | 1 +
tests/qemucapabilitiesdata/caps_4.0.0.aarch64.xml | 1 +
tests/qemucapabilitiesdata/caps_4.0.0.ppc64.xml| 1 +
tests/qemucapabilitiesdata/caps_4.0.0.riscv32.xml | 1 +
tests/qemucapabilitiesdata/caps_4.0.0.riscv64.xml | 1 +
tests/qemucapabilitiesdata/caps_4.0.0.s390x.xml| 1 +
tests/qemucapabilitiesdata/caps_4.0.0.x86_64.xml | 1 +
tests/qemucapabilitiesdata/caps_4.1.0.x86_64.xml | 1 +
tests/qemucapabilitiesdata/caps_4.2.0.aarch64.xml | 1 +
tests/qemucapabilitiesdata/caps_4.2.0.ppc64.xml| 1 +
tests/qemucapabilitiesdata/caps_4.2.0.s390x.xml| 1 +
tests/qemucapabilitiesdata/caps_4.2.0.x86_64.xml | 1 +
tests/qemucapabilitiesdata/caps_5.0.0.aarch64.xml | 1 +
tests/qemucapabilitiesdata/caps_5.0.0.ppc64.xml| 1 +
tests/qemucapabilitiesdata/caps_5.0.0.riscv64.xml | 1 +
tests/qemucapabilitiesdata/caps_5.0.0.x86_64.xml | 1 +
tests/qemucapabilitiesdata/caps_5.1.0.x86_64.xml | 1 +
tests/qemucapabilitiesdata/caps_5.2.0.aarch64.xml | 1 +
tests/qemucapabilitiesdata/caps_5.2.0.ppc64.xml| 1 +
tests/qemucapabilitiesdata/caps_5.2.0.riscv64.xml | 1 +
tests/qemucapabilitiesdata/caps_5.2.0.s390x.xml| 1 +
tests/qemucapabilitiesdata/caps_5.2.0.x86_64.xml | 1 +
tests/qemucapabilitiesdata/caps_6.0.0.aarch64.xml | 1 +
tests/qemucapabilitiesdata/caps_6.0.0.s390x.xml| 1 +
tests/qemucapabilitiesdata/caps_6.0.0.x86_64.xml | 1 +
tests/qemucapabilitiesdata/caps_6.1.0.x86_64.xml | 1 +
36 files changed, 37 insertions(+)
diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
index 70c3ec2f0c..9534641f81 100644
--- a/src/qemu/qemu_capabilities.c
+++ b/src/qemu/qemu_capabilities.c
@@ -638,6 +638,7 @@ VIR_ENUM_IMPL(virQEMUCaps,
"query-display-options", /* QEMU_CAPS_QUERY_DISPLAY_OPTIONS */
"s390-pv-guest", /* QEMU_CAPS_S390_PV_GUEST */
"set-action", /* QEMU_CAPS_SET_ACTION */
+ "virtio-blk.queue-size", /* QEMU_CAPS_VIRTIO_BLK_QUEUE_SIZE */
);
@@ -1398,6 +1399,7 @@ static struct virQEMUCapsDevicePropsFlags
virQEMUCapsDevicePropsVirtioBlk[] = {
{ "scsi", QEMU_CAPS_VIRTIO_BLK_SCSI,
virQEMUCapsDevicePropsVirtioBlkSCSIDefault },
{ "logical_block_size", QEMU_CAPS_BLOCKIO, NULL },
{ "num-queues", QEMU_CAPS_VIRTIO_BLK_NUM_QUEUES, NULL },
+{ "queue-size", QEMU_CAPS_VIRTIO_BLK_QUEUE_SIZE, NULL },
{ "share-rw", QEMU_CAPS_DISK_SHARE_RW, NULL },
{ "disable-legacy", QEMU_CAPS_VIRTIO_PCI_DISABLE_LEGACY, NULL },
{ "iommu_platform", QEMU_CAPS_VIRTIO_PCI_IOMMU_PLATFORM, NULL },
diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h
index bc762d1916..268c8350da 100644
--- a/src/qemu/qemu_capabilities.h
+++ b/src/qemu/qemu_capabilities.h
@@ -618,6 +618,7 @@ typedef enum { /* virQEMUCapsFlags grouping marker for
syntax-check */
QEMU_CAPS_QUERY_DISPLAY_OPTIONS, /* 'query-display-options' qmp command
present */
QEMU_CAPS_S390_PV_GUEST, /* -object s390-pv-guest,... */
QEMU_CAPS_SET_ACTION, /* 'set-action' QMP command */
+QEMU_CAPS_VIRTIO_BLK_QUEUE_SIZE, /* virtio-blk-*.queue-size */
QEMU_CAPS_LAST /* this must always be the last item */
} virQEMUCapsFlags;
diff --git a/tests/qemucapabilitiesdata/caps_2.12.0.aarch64.xml
b/tests/qemucapabilitiesdata/caps_2.12.0.aarch64.xml
index 3cd71919bc..4c7208d641 100644
--- a/tests/qemucapabilitiesdata/caps_2.12.0.aarch64.xml
+++ b/tests/qemucapabilitiesdata/caps_2.12.0.aarch64.xml
@@ -158,6 +158,7 @@
+
2012000
0
61700289
diff --git a/tests/qemucapabilitiesdata/caps_2.12.0.ppc64.xml
b/tests/qemucapabilitiesdata/caps_2.12.0.ppc64.xml
index 2081592b51..2b6e7e7135 100644
--- a/tests/qemucapabilitiesdata/caps_2.12.0.ppc64.xml
+++ b/tests/qemucapabilitiesdata/caps_2.12.0.ppc64.xml
@@ -157,6 +157,7 @@
+
2011090
0
42900289
diff --git a/tests/qemucapabilitiesdata/caps_2.12.0.s390x.xml
b/tests/qemucapabilitiesdata/caps_2.12.0.s390x.xml
index deabb614ba..05bd930e64 100644
--- a/tests/qemucapabilitiesdata/caps_2.12.0.s390x.xml
+++ b/tests/qemucapabilitiesdata/caps_2.12.0.s390x.xml
@@ -122,6 +122,7 @@
+
2012000
0
39100289
diff --git
[PATCH 5/5] qemu: add virtio-blk queue-size option
The option "queue-size" in virtio-blk was added in qemu-2.12.0, and default
value increased from qemu-5.0.0.
However, increasing this value may lead to drop of random access performance.
This is configurable value, so we want to use it via libvirt.
Signed-off-by: Hiroki Narukawa
---
src/qemu/qemu_command.c | 3 ++
src/qemu/qemu_validate.c | 7
.../disk-virtio-queue-size.args | 35 +
.../disk-virtio-queue-size.x86_64-latest.args | 35 +
.../disk-virtio-queue-size.xml| 38 +++
tests/qemuxml2argvtest.c | 1 +
.../disk-virtio-queue-size.x86_64-latest.xml | 38 +++
.../disk-virtio-queue-size.xml| 35 +
tests/qemuxml2xmltest.c | 1 +
9 files changed, 193 insertions(+)
create mode 100644 tests/qemuxml2argvdata/disk-virtio-queue-size.args
create mode 100644
tests/qemuxml2argvdata/disk-virtio-queue-size.x86_64-latest.args
create mode 100644 tests/qemuxml2argvdata/disk-virtio-queue-size.xml
create mode 100644
tests/qemuxml2xmloutdata/disk-virtio-queue-size.x86_64-latest.xml
create mode 100644 tests/qemuxml2xmloutdata/disk-virtio-queue-size.xml
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index b230314f7f..f0319d2234 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -1738,6 +1738,9 @@ qemuBuildDiskDeviceStr(const virDomainDef *def,
if (disk->queues) {
virBufferAsprintf(, ",num-queues=%u", disk->queues);
}
+if (disk->queue_size > 0) {
+virBufferAsprintf(, ",queue-size=%u", disk->queue_size);
+}
qemuBuildVirtioOptionsStr(, disk->virtio);
diff --git a/src/qemu/qemu_validate.c b/src/qemu/qemu_validate.c
index 1a470f1ff5..16ccf2e76d 100644
--- a/src/qemu/qemu_validate.c
+++ b/src/qemu/qemu_validate.c
@@ -2823,6 +2823,13 @@ qemuValidateDomainDeviceDefDiskFrontend(const
virDomainDiskDef *disk,
"QEMU binary"));
return -1;
}
+if (disk->queue_size &&
+!virQEMUCapsGet(qemuCaps, QEMU_CAPS_VIRTIO_BLK_NUM_QUEUES)) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+ _("queue-size property isn't supported by this "
+ "QEMU binary"));
+return -1;
+}
break;
case VIR_DOMAIN_DISK_BUS_USB:
diff --git a/tests/qemuxml2argvdata/disk-virtio-queue-size.args
b/tests/qemuxml2argvdata/disk-virtio-queue-size.args
new file mode 100644
index 00..2985e1726c
--- /dev/null
+++ b/tests/qemuxml2argvdata/disk-virtio-queue-size.args
@@ -0,0 +1,35 @@
+LC_ALL=C \
+PATH=/bin \
+HOME=/tmp/lib/domain--1-QEMUGuest1 \
+USER=test \
+LOGNAME=test \
+XDG_DATA_HOME=/tmp/lib/domain--1-QEMUGuest1/.local/share \
+XDG_CACHE_HOME=/tmp/lib/domain--1-QEMUGuest1/.cache \
+XDG_CONFIG_HOME=/tmp/lib/domain--1-QEMUGuest1/.config \
+/usr/bin/qemu-system-i386 \
+-name guest=QEMUGuest1,debug-threads=on \
+-S \
+-object
'{"qom-type":"secret","id":"masterKey0","format":"raw","file":"/tmp/lib/domain--1-QEMUGuest1/master-key.aes"}'
\
+-machine pc,accel=tcg,usb=off,dump-guest-core=off,memory-backend=pc.ram \
+-cpu qemu64 \
+-m 214 \
+-object '{"qom-type":"memory-backend-ram","id":"pc.ram","size":224395264}' \
+-overcommit mem-lock=off \
+-smp 1,sockets=1,cores=1,threads=1 \
+-uuid c7a5fdbd-edaf-9455-926a-d65c16db1809 \
+-display none \
+-no-user-config \
+-nodefaults \
+-chardev socket,id=charmonitor,fd=1729,server=on,wait=off \
+-mon chardev=charmonitor,id=monitor,mode=control \
+-rtc base=utc \
+-no-shutdown \
+-no-acpi \
+-boot strict=on \
+-device piix3-usb-uhci,id=usb,bus=pci.0,addr=0x1.0x2 \
+-blockdev
'{"driver":"file","filename":"/tmp/data.img","node-name":"libvirt-1-storage","auto-read-only":true,"discard":"unmap"}'
\
+-blockdev
'{"node-name":"libvirt-1-format","read-only":false,"driver":"raw","file":"libvirt-1-storage"}'
\
+-device
virtio-blk-pci,queue-size=4,bus=pci.0,addr=0x3,drive=libvirt-1-format,id=virtio-disk0,bootindex=1
\
+-audiodev id=audio1,driver=none \
+-sandbox
on,obsolete=deny,elevateprivileges=deny,spawn=deny,resourcecontrol=deny \
+-msg timestamp=on
diff --git a/tests/qemuxml2argvdata/disk-virtio-queue-size.x86_64-latest.args
b/tests/qemuxml2argvdata/disk-virtio-queue-size.x86_64-latest.args
new file mode 100644
index 00..2985e1726c
--- /dev/null
+++ b/tests/qemuxml2argvdata/disk-virtio-queue-size.x86_64-latest.args
@@ -0,0 +1,35 @@
+LC_ALL=C \
+PATH=/bin \
+HOME=/tmp/lib/domain--1-QEMUGuest1 \
+USER=test \
+LOGNAME=test \
+XDG_DATA_HOME=/tmp/lib/domain--1-QEMUGuest1/.local/share \
+XDG_CACHE_HOME=/tmp/lib/domain--1-QEMUGuest1/.cache \
+XDG_CONFIG_HOME=/tmp/lib/domain--1-QEMUGuest1/.config \
+/usr/bin/qemu-system-i386 \
+-name guest=QEMUGuest1,debug-threads=on \
+-S \
+-object
[PATCH 2/5] qemu: add disk queue count ABI stability check
virtio-blk num-queue is visible to guest OS, so this must be kept while live
migration.
Signed-off-by: Hiroki Narukawa
---
src/conf/domain_conf.c | 7 +++
1 file changed, 7 insertions(+)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index dbefc98ee8..6cc1f78ec2 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -20766,6 +20766,13 @@ virDomainDiskDefCheckABIStability(virDomainDiskDef
*src,
return false;
}
+if (src->queues != dst->queues) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("Target disk queue count %u does not match source
%u"),
+ dst->queues, src->queues);
+return false;
+}
+
if (!virDomainVirtioOptionsCheckABIStability(src->virtio, dst->virtio))
return false;
--
2.17.1
[PATCH 1/5] qemu: Make disk-virtio-queues tests use DO_TEST_CAPS_LATEST
Currently disk-virtio-queues test is now using specifying a fake capability.
By this commit this test will make use of DO_TEST_CAPS_LATEST.
Signed-off-by: Hiroki Narukawa
---
.../qemuxml2argvdata/disk-virtio-queues.args | 20 +++
.../disk-virtio-queues.x86_64-latest.args | 35 +++
tests/qemuxml2argvdata/disk-virtio-queues.xml | 5 ++-
tests/qemuxml2argvtest.c | 3 +-
.../disk-virtio-queues.x86_64-latest.xml | 1 +
tests/qemuxml2xmltest.c | 2 +-
6 files changed, 55 insertions(+), 11 deletions(-)
create mode 100644 tests/qemuxml2argvdata/disk-virtio-queues.x86_64-latest.args
create mode 12
tests/qemuxml2xmloutdata/disk-virtio-queues.x86_64-latest.xml
diff --git a/tests/qemuxml2argvdata/disk-virtio-queues.args
b/tests/qemuxml2argvdata/disk-virtio-queues.args
index c0166357e7..6637088a35 100644
--- a/tests/qemuxml2argvdata/disk-virtio-queues.args
+++ b/tests/qemuxml2argvdata/disk-virtio-queues.args
@@ -6,24 +6,30 @@ LOGNAME=test \
XDG_DATA_HOME=/tmp/lib/domain--1-QEMUGuest1/.local/share \
XDG_CACHE_HOME=/tmp/lib/domain--1-QEMUGuest1/.cache \
XDG_CONFIG_HOME=/tmp/lib/domain--1-QEMUGuest1/.config \
-QEMU_AUDIO_DRV=none \
/usr/bin/qemu-system-i386 \
-name guest=QEMUGuest1,debug-threads=on \
-S \
--machine pc,accel=tcg,usb=off,dump-guest-core=off \
+-object
'{"qom-type":"secret","id":"masterKey0","format":"raw","file":"/tmp/lib/domain--1-QEMUGuest1/master-key.aes"}'
\
+-machine pc,accel=tcg,usb=off,dump-guest-core=off,memory-backend=pc.ram \
+-cpu qemu64 \
-m 214 \
--realtime mlock=off \
+-object '{"qom-type":"memory-backend-ram","id":"pc.ram","size":224395264}' \
+-overcommit mem-lock=off \
-smp 1,sockets=1,cores=1,threads=1 \
-uuid c7a5fdbd-edaf-9455-926a-d65c16db1809 \
-display none \
-no-user-config \
-nodefaults \
--chardev
socket,id=charmonitor,path=/tmp/lib/domain--1-QEMUGuest1/monitor.sock,server=on,wait=off
\
+-chardev socket,id=charmonitor,fd=1729,server=on,wait=off \
-mon chardev=charmonitor,id=monitor,mode=control \
-rtc base=utc \
-no-shutdown \
-no-acpi \
--usb \
--drive file=/tmp/data.img,format=raw,if=none,id=drive-virtio-disk0 \
--device
virtio-blk-pci,num-queues=4,bus=pci.0,addr=0x3,drive=drive-virtio-disk0,id=virtio-disk0,bootindex=1
\
+-boot strict=on \
+-device piix3-usb-uhci,id=usb,bus=pci.0,addr=0x1.0x2 \
+-blockdev
'{"driver":"file","filename":"/tmp/data.img","node-name":"libvirt-1-storage","auto-read-only":true,"discard":"unmap"}'
\
+-blockdev
'{"node-name":"libvirt-1-format","read-only":false,"driver":"raw","file":"libvirt-1-storage"}'
\
+-device
virtio-blk-pci,num-queues=4,bus=pci.0,addr=0x3,drive=libvirt-1-format,id=virtio-disk0,bootindex=1
\
+-audiodev id=audio1,driver=none \
+-sandbox
on,obsolete=deny,elevateprivileges=deny,spawn=deny,resourcecontrol=deny \
-msg timestamp=on
diff --git a/tests/qemuxml2argvdata/disk-virtio-queues.x86_64-latest.args
b/tests/qemuxml2argvdata/disk-virtio-queues.x86_64-latest.args
new file mode 100644
index 00..6637088a35
--- /dev/null
+++ b/tests/qemuxml2argvdata/disk-virtio-queues.x86_64-latest.args
@@ -0,0 +1,35 @@
+LC_ALL=C \
+PATH=/bin \
+HOME=/tmp/lib/domain--1-QEMUGuest1 \
+USER=test \
+LOGNAME=test \
+XDG_DATA_HOME=/tmp/lib/domain--1-QEMUGuest1/.local/share \
+XDG_CACHE_HOME=/tmp/lib/domain--1-QEMUGuest1/.cache \
+XDG_CONFIG_HOME=/tmp/lib/domain--1-QEMUGuest1/.config \
+/usr/bin/qemu-system-i386 \
+-name guest=QEMUGuest1,debug-threads=on \
+-S \
+-object
'{"qom-type":"secret","id":"masterKey0","format":"raw","file":"/tmp/lib/domain--1-QEMUGuest1/master-key.aes"}'
\
+-machine pc,accel=tcg,usb=off,dump-guest-core=off,memory-backend=pc.ram \
+-cpu qemu64 \
+-m 214 \
+-object '{"qom-type":"memory-backend-ram","id":"pc.ram","size":224395264}' \
+-overcommit mem-lock=off \
+-smp 1,sockets=1,cores=1,threads=1 \
+-uuid c7a5fdbd-edaf-9455-926a-d65c16db1809 \
+-display none \
+-no-user-config \
+-nodefaults \
+-chardev socket,id=charmonitor,fd=1729,server=on,wait=off \
+-mon chardev=charmonitor,id=monitor,mode=control \
+-rtc base=utc \
+-no-shutdown \
+-no-acpi \
+-boot strict=on \
+-device piix3-usb-uhci,id=usb,bus=pci.0,addr=0x1.0x2 \
+-blockdev
'{"driver":"file","filename":"/tmp/data.img","node-name":"libvirt-1-storage","auto-read-only":true,"discard":"unmap"}'
\
+-blockdev
'{"node-name":"libvirt-1-format","read-only":false,"driver":"raw","file":"libvirt-1-storage"}'
\
+-device
virtio-blk-pci,num-queues=4,bus=pci.0,addr=0x3,drive=libvirt-1-format,id=virtio-disk0,bootindex=1
\
+-audiodev id=audio1,driver=none \
+-sandbox
on,obsolete=deny,elevateprivileges=deny,spawn=deny,resourcecontrol=deny \
+-msg timestamp=on
diff --git a/tests/qemuxml2argvdata/disk-virtio-queues.xml
b/tests/qemuxml2argvdata/disk-virtio-queues.xml
index c758d21894..ea1001bdd4 100644
--- a/tests/qemuxml2argvdata/disk-virtio-queues.xml
+++ b/tests/qemuxml2argvdata/disk-virtio-queues.xml
@@ -8,6 +8,9 @@
hvm
+
+qemu64
+
[PATCH 0/5] qemu: add virtio-blk queue-size option
This is resubmit of "qemu: add virtio-blk queue-size option" after following the review. The option "queue-size" in virtio-blk was added in qemu-2.12.0, and default value increased from qemu-5.0.0. However, increasing this value may lead to drop of random access performance. This is configurable value, so we want to use it via libvirt. Hiroki Narukawa (5): qemu: Make disk-virtio-queues tests use DO_TEST_CAPS_LATEST qemu: add disk queue count ABI stability check qemu: add queue_size option to disk qemu: add QEMU_CAPS_VIRTIO_BLK_QUEUE_SIZE capability qemu: add virtio-blk queue-size option docs/formatdomain.rst | 4 +- docs/schemas/domaincommon.rng | 5 +++ src/conf/domain_conf.c| 20 ++ src/conf/domain_conf.h| 1 + src/qemu/qemu_capabilities.c | 2 + src/qemu/qemu_capabilities.h | 1 + src/qemu/qemu_command.c | 3 ++ src/qemu/qemu_validate.c | 7 .../caps_2.12.0.aarch64.xml | 1 + .../caps_2.12.0.ppc64.xml | 1 + .../caps_2.12.0.s390x.xml | 1 + .../caps_2.12.0.x86_64.xml| 1 + .../qemucapabilitiesdata/caps_3.0.0.ppc64.xml | 1 + .../qemucapabilitiesdata/caps_3.0.0.s390x.xml | 1 + .../caps_3.0.0.x86_64.xml | 1 + .../qemucapabilitiesdata/caps_3.1.0.ppc64.xml | 1 + .../caps_3.1.0.x86_64.xml | 1 + .../caps_4.0.0.aarch64.xml| 1 + .../qemucapabilitiesdata/caps_4.0.0.ppc64.xml | 1 + .../caps_4.0.0.riscv32.xml| 1 + .../caps_4.0.0.riscv64.xml| 1 + .../qemucapabilitiesdata/caps_4.0.0.s390x.xml | 1 + .../caps_4.0.0.x86_64.xml | 1 + .../caps_4.1.0.x86_64.xml | 1 + .../caps_4.2.0.aarch64.xml| 1 + .../qemucapabilitiesdata/caps_4.2.0.ppc64.xml | 1 + .../qemucapabilitiesdata/caps_4.2.0.s390x.xml | 1 + .../caps_4.2.0.x86_64.xml | 1 + .../caps_5.0.0.aarch64.xml| 1 + .../qemucapabilitiesdata/caps_5.0.0.ppc64.xml | 1 + .../caps_5.0.0.riscv64.xml| 1 + .../caps_5.0.0.x86_64.xml | 1 + .../caps_5.1.0.x86_64.xml | 1 + .../caps_5.2.0.aarch64.xml| 1 + .../qemucapabilitiesdata/caps_5.2.0.ppc64.xml | 1 + .../caps_5.2.0.riscv64.xml| 1 + .../qemucapabilitiesdata/caps_5.2.0.s390x.xml | 1 + .../caps_5.2.0.x86_64.xml | 1 + .../caps_6.0.0.aarch64.xml| 1 + .../qemucapabilitiesdata/caps_6.0.0.s390x.xml | 1 + .../caps_6.0.0.x86_64.xml | 1 + .../caps_6.1.0.x86_64.xml | 1 + .../disk-virtio-queue-size.args | 35 + .../disk-virtio-queue-size.x86_64-latest.args | 35 + .../disk-virtio-queue-size.xml| 38 +++ .../qemuxml2argvdata/disk-virtio-queues.args | 20 ++ .../disk-virtio-queues.x86_64-latest.args | 35 + tests/qemuxml2argvdata/disk-virtio-queues.xml | 5 ++- tests/qemuxml2argvtest.c | 4 +- .../disk-virtio-queue-size.x86_64-latest.xml | 38 +++ .../disk-virtio-queue-size.xml| 35 + .../disk-virtio-queues.x86_64-latest.xml | 1 + tests/qemuxml2xmltest.c | 3 +- 53 files changed, 314 insertions(+), 12 deletions(-) create mode 100644 tests/qemuxml2argvdata/disk-virtio-queue-size.args create mode 100644 tests/qemuxml2argvdata/disk-virtio-queue-size.x86_64-latest.args create mode 100644 tests/qemuxml2argvdata/disk-virtio-queue-size.xml create mode 100644 tests/qemuxml2argvdata/disk-virtio-queues.x86_64-latest.args create mode 100644 tests/qemuxml2xmloutdata/disk-virtio-queue-size.x86_64-latest.xml create mode 100644 tests/qemuxml2xmloutdata/disk-virtio-queue-size.xml create mode 12 tests/qemuxml2xmloutdata/disk-virtio-queues.x86_64-latest.xml -- 2.17.1
[PATCH 3/5] qemu: add queue_size option to disk
The option "queue-size" in virtio-blk was added in qemu-2.12.0, and default
value increased from qemu-5.0.0.
However, increasing this value may lead to drop of random access performance.
To add disk queue_size option, first this commit add interface to accept.
Signed-off-by: Hiroki Narukawa
---
docs/formatdomain.rst | 4 +++-
docs/schemas/domaincommon.rng | 5 +
src/conf/domain_conf.c| 13 +
src/conf/domain_conf.h| 1 +
4 files changed, 22 insertions(+), 1 deletion(-)
diff --git a/docs/formatdomain.rst b/docs/formatdomain.rst
index 479a3acfbb..617a892b38 100644
--- a/docs/formatdomain.rst
+++ b/docs/formatdomain.rst
@@ -2474,7 +2474,7 @@ paravirtualized driver is specified via the ``disk``
element.
-
+
@@ -3085,6 +3085,8 @@ paravirtualized driver is specified via the ``disk``
element.
(QEMU 2.1)`
- The optional ``queues`` attribute specifies the number of virt queues for
virtio-blk. ( :since:`Since 3.9.0` )
+ - The optional ``queue_size`` attribute specifies the size of each virt
+ queue for virtio-blk. ( :since:`Since 7.8.0` )
- For virtio disks, `Virtio-specific options <#elementsVirtio>`__ can also
be set. ( :since:`Since 3.5.0` )
- The optional ``metadata_cache`` subelement controls aspects related to
the
diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
index 11fa24f398..fdc04f90aa 100644
--- a/docs/schemas/domaincommon.rng
+++ b/docs/schemas/domaincommon.rng
@@ -2363,6 +2363,11 @@
+
+
+
+
+
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 6cc1f78ec2..123e9b694a 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -8930,6 +8930,9 @@ virDomainDiskDefDriverParseXML(virDomainDiskDef *def,
if (virXMLPropUInt(cur, "queues", 10, VIR_XML_PROP_NONE, >queues) < 0)
return -1;
+if (virXMLPropUInt(cur, "queue_size", 10, VIR_XML_PROP_NONE,
>queue_size) < 0)
+return -1;
+
return 0;
}
@@ -20773,6 +20776,13 @@ virDomainDiskDefCheckABIStability(virDomainDiskDef
*src,
return false;
}
+if (src->queue_size != dst->queue_size) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("Target disk queue size %u does not match source %u"),
+ dst->queues, src->queues);
+return false;
+}
+
if (!virDomainVirtioOptionsCheckABIStability(src->virtio, dst->virtio))
return false;
@@ -23423,6 +23433,9 @@ virDomainDiskDefFormatDriver(virBuffer *buf,
if (disk->queues)
virBufferAsprintf(, " queues='%u'", disk->queues);
+if (disk->queue_size)
+virBufferAsprintf(, " queue_size='%u'", disk->queue_size);
+
virDomainVirtioOptionsFormat(, disk->virtio);
if (disk->src->metadataCacheMaxSize > 0) {
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index c7e6df7981..688a842660 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -584,6 +584,7 @@ struct _virDomainDiskDef {
virDomainDiskDetectZeroes detect_zeroes;
char *domain_name; /* backend domain name */
unsigned int queues;
+unsigned int queue_size;
virDomainDiskModel model;
virDomainVirtioOptions *virtio;
--
2.17.1
[PATCH 9/9] ch_driver: Handle validation failure correctly
When validation like deviceValidateCallback fails, the vm will not be
set and so the call to virDomainObjListRemove will be passed a NULL
pointer causing a segfault. To prevent this add a check that the vm is
defined before calling out to virDomainObjListRemove.
Signed-off-by: William Douglas
---
src/ch/ch_driver.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/ch/ch_driver.c b/src/ch/ch_driver.c
index c821459fc5..1824d2fd16 100644
--- a/src/ch/ch_driver.c
+++ b/src/ch/ch_driver.c
@@ -262,7 +262,7 @@ chDomainCreateXML(virConnectPtr conn,
virCHDomainObjEndJob(vm);
cleanup:
-if (!dom) {
+if (vm && !dom) {
virDomainObjListRemove(driver->domains, vm);
}
virDomainDefFree(vmdef);
--
2.31.1
[PATCH 3/9] ch_monitor: Update virCHMonitorGet to handle accept a response
The virCHMonitorGet function needed to be able to return data from the
hypervisor. This functionality is needed in order for the driver to
support PTY enablement and getting details about the VM state.
Signed-off-by: William Douglas
---
src/ch/ch_monitor.c | 46 +++--
1 file changed, 44 insertions(+), 2 deletions(-)
diff --git a/src/ch/ch_monitor.c b/src/ch/ch_monitor.c
index b4bc10bfcf..44b99ef07a 100644
--- a/src/ch/ch_monitor.c
+++ b/src/ch/ch_monitor.c
@@ -611,12 +611,36 @@ virCHMonitorPutNoContent(virCHMonitor *mon, const char
*endpoint)
return ret;
}
+struct curl_data {
+char *content;
+size_t size;
+};
+
+static size_t
+curl_callback(void *contents, size_t size, size_t nmemb, void *userp)
+{
+size_t content_size = size * nmemb;
+struct curl_data *data = (struct curl_data *)userp;
+
+if (content_size == 0)
+return content_size;
+
+data->content = g_realloc(data->content, data->size + content_size);
+
+memcpy(&(data->content[data->size]), contents, content_size);
+data->size += content_size;
+
+return content_size;
+}
+
static int
-virCHMonitorGet(virCHMonitor *mon, const char *endpoint)
+virCHMonitorGet(virCHMonitor *mon, const char *endpoint, virJSONValue
**response)
{
g_autofree char *url = NULL;
int responseCode = 0;
int ret = -1;
+struct curl_slist *headers = NULL;
+struct curl_data data = {0};
url = g_strdup_printf("%s/%s", URL_ROOT, endpoint);
@@ -628,12 +652,30 @@ virCHMonitorGet(virCHMonitor *mon, const char *endpoint)
curl_easy_setopt(mon->handle, CURLOPT_UNIX_SOCKET_PATH, mon->socketpath);
curl_easy_setopt(mon->handle, CURLOPT_URL, url);
+if (response) {
+headers = curl_slist_append(headers, "Accept: application/json");
+headers = curl_slist_append(headers, "Content-Type: application/json");
+curl_easy_setopt(mon->handle, CURLOPT_HTTPHEADER, headers);
+curl_easy_setopt(mon->handle, CURLOPT_WRITEFUNCTION, curl_callback);
+curl_easy_setopt(mon->handle, CURLOPT_WRITEDATA, (void *));
+}
+
responseCode = virCHMonitorCurlPerform(mon->handle);
virObjectUnlock(mon);
-if (responseCode == 200 || responseCode == 204)
+if (responseCode == 200 || responseCode == 204) {
ret = 0;
+if (response) {
+data.content = g_realloc(data.content, data.size + 1);
+data.content[data.size] = 0;
+*response = virJSONValueFromString(data.content);
+}
+}
+
+g_free(data.content);
+/* reset the libcurl handle to avoid leaking a stack pointer to data */
+curl_easy_reset(mon->handle);
return ret;
}
--
2.31.1
[PATCH 4/9] ch_monitor: Use virCHMonitorGet to access cloud-hypervisor API
Now that virCHMonitorGet is capable of handling data returned by the
cloud-hypervisor API, make use of this via virCHMonitorGetInfo to call
into the vm.info endpoint.
Signed-off-by: William Douglas
---
src/ch/ch_monitor.c | 15 +++
src/ch/ch_monitor.h | 3 +++
2 files changed, 18 insertions(+)
diff --git a/src/ch/ch_monitor.c b/src/ch/ch_monitor.c
index 44b99ef07a..28c70c7281 100644
--- a/src/ch/ch_monitor.c
+++ b/src/ch/ch_monitor.c
@@ -753,3 +753,18 @@ virCHMonitorResumeVM(virCHMonitor *mon)
{
return virCHMonitorPutNoContent(mon, URL_VM_RESUME);
}
+
+/**
+ * virCHMonitorGetInfo:
+ * @mon: Pointer to the monitor
+ * @info: Get VM info
+ *
+ * Retrive the VM info and store in @info
+ *
+ * Returns 0 on success.
+ */
+int
+virCHMonitorGetInfo(virCHMonitor *mon, virJSONValue **info)
+{
+return virCHMonitorGet(mon, URL_VM_INFO, info);
+}
diff --git a/src/ch/ch_monitor.h b/src/ch/ch_monitor.h
index e717e11cbc..e39b4eb8b2 100644
--- a/src/ch/ch_monitor.h
+++ b/src/ch/ch_monitor.h
@@ -23,6 +23,7 @@
#include
#include "virobject.h"
+#include "virjson.h"
#include "domain_conf.h"
#define URL_ROOT "http://localhost/api/v1;
@@ -34,6 +35,7 @@
#define URL_VM_REBOOT "vm.reboot"
#define URL_VM_Suspend "vm.pause"
#define URL_VM_RESUME "vm.resume"
+#define URL_VM_INFO "vm.info"
typedef struct _virCHMonitor virCHMonitor;
@@ -58,3 +60,4 @@ int virCHMonitorShutdownVM(virCHMonitor *mon);
int virCHMonitorRebootVM(virCHMonitor *mon);
int virCHMonitorSuspendVM(virCHMonitor *mon);
int virCHMonitorResumeVM(virCHMonitor *mon);
+int virCHMonitorGetInfo(virCHMonitor *mon, virJSONValue **info);
--
2.31.1
[PATCH 8/9] ch_driver: Add handler for console API
Enable the handler function to find and open the console character
device that will be used by the console API.
Signed-off-by: William Douglas
---
src/ch/ch_driver.c | 74 ++
1 file changed, 74 insertions(+)
diff --git a/src/ch/ch_driver.c b/src/ch/ch_driver.c
index ac958d73a8..c821459fc5 100644
--- a/src/ch/ch_driver.c
+++ b/src/ch/ch_driver.c
@@ -30,6 +30,7 @@
#include "viraccessapicheck.h"
#include "viralloc.h"
#include "virbuffer.h"
+#include "virchrdev.h"
#include "vircommand.h"
#include "virerror.h"
#include "virfile.h"
@@ -816,6 +817,78 @@ static int chDomainGetInfo(virDomainPtr dom,
return ret;
}
+static int
+chDomainOpenConsole(virDomainPtr dom,
+const char *dev_name,
+virStreamPtr st,
+unsigned int flags)
+{
+ virDomainObj *vm = NULL;
+ int ret = -1;
+ size_t i;
+ virDomainChrDef *chr = NULL;
+ virCHDomainObjPrivate *priv;
+
+ virCheckFlags(VIR_DOMAIN_CONSOLE_SAFE | VIR_DOMAIN_CONSOLE_FORCE, -1);
+
+ if (!(vm = chDomObjFromDomain(dom)))
+ goto cleanup;
+
+ if (virDomainOpenConsoleEnsureACL(dom->conn, vm->def) < 0)
+ goto cleanup;
+
+ if (virDomainObjCheckActive(vm) < 0)
+ goto cleanup;
+
+ priv = vm->privateData;
+
+ if (dev_name) {
+ for (i = 0; !chr && i < vm->def->nconsoles; i++) {
+ if (vm->def->consoles[i]->info.alias &&
+ STREQ(dev_name, vm->def->consoles[i]->info.alias))
+chr = vm->def->consoles[i];
+ }
+ for (i = 0; !chr && i < vm->def->nserials; i++) {
+ if (STREQ(dev_name, vm->def->serials[i]->info.alias))
+chr = vm->def->serials[i];
+ }
+ } else {
+ if (vm->def->nconsoles &&
+ vm->def->consoles[0]->source->type == VIR_DOMAIN_CHR_TYPE_PTY)
+ chr = vm->def->consoles[0];
+ else if (vm->def->nserials &&
+ vm->def->serials[0]->source->type ==
VIR_DOMAIN_CHR_TYPE_PTY)
+ chr = vm->def->serials[0];
+ }
+
+ if (!chr) {
+ virReportError(VIR_ERR_INTERNAL_ERROR, _("cannot find character
device %s"),
+ NULLSTR(dev_name));
+ goto cleanup;
+ }
+
+ if (chr->source->type != VIR_DOMAIN_CHR_TYPE_PTY) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("character device %s is not using a PTY"),
+ dev_name ? dev_name : NULLSTR(chr->info.alias));
+ goto cleanup;
+ }
+
+ /* handle mutually exclusive access to console devices */
+ ret = virChrdevOpen(priv->chrdevs, chr->source, st,
+ (flags & VIR_DOMAIN_CONSOLE_FORCE) != 0);
+
+ if (ret == 1) {
+ virReportError(VIR_ERR_OPERATION_FAILED, "%s",
+ _("Active console session exists for this domain"));
+ ret = -1;
+ }
+
+ cleanup:
+ virDomainObjEndAPI();
+ return ret;
+}
+
static int chStateCleanup(void)
{
if (ch_driver == NULL)
@@ -913,6 +986,7 @@ static virHypervisorDriver chHypervisorDriver = {
.domainGetXMLDesc = chDomainGetXMLDesc, /* 7.5.0 */
.domainGetInfo = chDomainGetInfo, /* 7.5.0 */
.domainIsActive = chDomainIsActive, /* 7.5.0 */
+.domainOpenConsole = chDomainOpenConsole, /* 7.8.0 */
.nodeGetInfo = chNodeGetInfo, /* 7.5.0 */
};
--
2.31.1
[PATCH 6/9] ch_process: Handle enabled console devices
Add functionality to allow libvirt console to connect to the
cloud-hypervisor created PTY associated with a VM by updating the
domain with console path information. This has to be run after the VM
is created by cloud-hypervisor.
Signed-off-by: William Douglas
---
src/ch/ch_process.c | 84 +
1 file changed, 84 insertions(+)
diff --git a/src/ch/ch_process.c b/src/ch/ch_process.c
index 93b1f7f97e..1a01ca9384 100644
--- a/src/ch/ch_process.c
+++ b/src/ch/ch_process.c
@@ -28,6 +28,7 @@
#include "ch_process.h"
#include "viralloc.h"
#include "virerror.h"
+#include "virjson.h"
#include "virlog.h"
#define VIR_FROM_THIS VIR_FROM_CH
@@ -52,6 +53,86 @@ virCHProcessConnectMonitor(virCHDriver *driver,
return monitor;
}
+static void
+virCHProcessUpdateConsoleDevice(virDomainObj *vm,
+virJSONValue *config,
+const char *device)
+{
+const char *path;
+virDomainChrDef *chr = NULL;
+virJSONValue *dev, *file;
+
+if (!config)
+return;
+
+dev = virJSONValueObjectGet(config, device);
+if (!dev) {
+virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("missing '%s' in 'config' from cloud-hypervisor"),
+ device);
+return;
+}
+
+file = virJSONValueObjectGet(dev, "file");
+if (!file) {
+virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("missing 'file' in '%s' from cloud-hypervisor"),
+ device);
+return;
+}
+
+path = virJSONValueGetString(file);
+if (!path) {
+virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("unable to parse contents of 'file' field in '%s'
from cloud-hypervisor"),
+ device);
+return;
+}
+
+if (STREQ(device, "console")) {
+chr = vm->def->consoles[0];
+} else if (STREQ(device, "serial")) {
+chr = vm->def->serials[0];
+}
+
+if (chr && chr->source)
+chr->source->data.file.path = g_strdup(path);
+}
+
+static void
+virCHProcessUpdateConsole(virDomainObj *vm,
+ virJSONValue *info)
+{
+virJSONValue *config;
+
+config = virJSONValueObjectGet(info, "config");
+if (!config) {
+virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("missing 'config' in info query result from
cloud-hypervisor"));
+return;
+}
+
+if (vm->def->nconsoles > 0)
+virCHProcessUpdateConsoleDevice(vm, config, "console");
+if (vm->def->nserials > 0)
+virCHProcessUpdateConsoleDevice(vm, config, "serial");
+}
+
+static int
+virCHProcessUpdateInfo(virDomainObj *vm)
+{
+virJSONValue *info;
+virCHDomainObjPrivate *priv = vm->privateData;
+if (virCHMonitorGetInfo(priv->monitor, ) < 0)
+return -1;
+
+virCHProcessUpdateConsole(vm, info);
+
+virJSONValueFree(info);
+
+return 0;
+}
+
/**
* virCHProcessStart:
* @driver: pointer to driver structure
@@ -92,6 +173,9 @@ int virCHProcessStart(virCHDriver *driver,
vm->pid = priv->monitor->pid;
vm->def->id = vm->pid;
+
+virCHProcessUpdateInfo(vm);
+
virDomainObjSetState(vm, VIR_DOMAIN_RUNNING, reason);
return 0;
--
2.31.1
[PATCH 2/9] ch_monitor: Make unused function static
The virCHMonitorGet function wasn't in use and was declared as
non-static (though not in a header file). This function isn't going to
be used outside of the monitor though so remove the initial
declaration and define the function to be static.
Future work should adjust this function to allow reading VM state
needed for PTY enablement.
Signed-off-by: William Douglas
---
src/ch/ch_monitor.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/src/ch/ch_monitor.c b/src/ch/ch_monitor.c
index 0f8752d1ed..b4bc10bfcf 100644
--- a/src/ch/ch_monitor.c
+++ b/src/ch/ch_monitor.c
@@ -54,7 +54,6 @@ VIR_ONCE_GLOBAL_INIT(virCHMonitor);
int virCHMonitorShutdownVMM(virCHMonitor *mon);
int virCHMonitorPutNoContent(virCHMonitor *mon, const char *endpoint);
-int virCHMonitorGet(virCHMonitor *mon, const char *endpoint);
static int
virCHMonitorBuildCPUJson(virJSONValue *content, virDomainDef *vmdef)
@@ -612,7 +611,7 @@ virCHMonitorPutNoContent(virCHMonitor *mon, const char
*endpoint)
return ret;
}
-int
+static int
virCHMonitorGet(virCHMonitor *mon, const char *endpoint)
{
g_autofree char *url = NULL;
--
2.31.1
[PATCH 1/9] ch_domain: Add virChrdevs for console support
Add and initialize a virChrdevs to the _virCHDomainObjPrivate
structure in order to eventually track the consoles in use by a domain.
Signed-off-by: William Douglas
---
src/ch/ch_domain.c | 7 +++
src/ch/ch_domain.h | 3 +++
2 files changed, 10 insertions(+)
diff --git a/src/ch/ch_domain.c b/src/ch/ch_domain.c
index 780a46ba00..a6b87e28e5 100644
--- a/src/ch/ch_domain.c
+++ b/src/ch/ch_domain.c
@@ -22,6 +22,7 @@
#include "ch_domain.h"
#include "viralloc.h"
+#include "virchrdev.h"
#include "virlog.h"
#include "virtime.h"
@@ -146,6 +147,11 @@ virCHDomainObjPrivateAlloc(void *opaque G_GNUC_UNUSED)
return NULL;
}
+if (!(priv->chrdevs = virChrdevAlloc())) {
+g_free(priv);
+return NULL;
+}
+
return priv;
}
@@ -154,6 +160,7 @@ virCHDomainObjPrivateFree(void *data)
{
virCHDomainObjPrivate *priv = data;
+virChrdevFree(priv->chrdevs);
virCHDomainObjFreeJob(priv);
g_free(priv);
}
diff --git a/src/ch/ch_domain.h b/src/ch/ch_domain.h
index b4e0d4c212..61b34b0467 100644
--- a/src/ch/ch_domain.h
+++ b/src/ch/ch_domain.h
@@ -22,6 +22,7 @@
#include "ch_conf.h"
#include "ch_monitor.h"
+#include "virchrdev.h"
/* Give up waiting for mutex after 30 seconds */
#define CH_JOB_WAIT_TIME (1000ull * 30)
@@ -52,6 +53,8 @@ struct _virCHDomainObjPrivate {
struct virCHDomainJobObj job;
virCHMonitor *monitor;
+
+ virChrdevs *chrdevs;
};
extern virDomainXMLPrivateDataCallbacks virCHDriverPrivateDataCallbacks;
--
2.31.1
[PATCH 7/9] ch_domain: Allow controller and chr devices
With the console and serial device handling fully functional, allow
the required device types to be specified in the domain
configuration.
The configuration only supports a single serial or console device.
Signed-off-by: William Douglas
---
src/ch/ch_domain.c | 33 +++--
1 file changed, 31 insertions(+), 2 deletions(-)
diff --git a/src/ch/ch_domain.c b/src/ch/ch_domain.c
index a6b87e28e5..ed01f637b1 100644
--- a/src/ch/ch_domain.c
+++ b/src/ch/ch_domain.c
@@ -215,6 +215,8 @@ chValidateDomainDeviceDef(const virDomainDeviceDef *dev,
case VIR_DOMAIN_DEVICE_NET:
case VIR_DOMAIN_DEVICE_MEMORY:
case VIR_DOMAIN_DEVICE_VSOCK:
+case VIR_DOMAIN_DEVICE_CONTROLLER:
+case VIR_DOMAIN_DEVICE_CHR:
break;
case VIR_DOMAIN_DEVICE_LEASE:
@@ -224,12 +226,10 @@ chValidateDomainDeviceDef(const virDomainDeviceDef *dev,
case VIR_DOMAIN_DEVICE_VIDEO:
case VIR_DOMAIN_DEVICE_HOSTDEV:
case VIR_DOMAIN_DEVICE_WATCHDOG:
-case VIR_DOMAIN_DEVICE_CONTROLLER:
case VIR_DOMAIN_DEVICE_GRAPHICS:
case VIR_DOMAIN_DEVICE_HUB:
case VIR_DOMAIN_DEVICE_REDIRDEV:
case VIR_DOMAIN_DEVICE_SMARTCARD:
-case VIR_DOMAIN_DEVICE_CHR:
case VIR_DOMAIN_DEVICE_MEMBALLOON:
case VIR_DOMAIN_DEVICE_NVRAM:
case VIR_DOMAIN_DEVICE_RNG:
@@ -254,6 +254,35 @@ chValidateDomainDeviceDef(const virDomainDeviceDef *dev,
return -1;
}
+if ((def->nconsoles &&
+ def->consoles[0]->source->type == VIR_DOMAIN_CHR_TYPE_PTY)
+&& (def->nserials &&
+def->serials[0]->source->type == VIR_DOMAIN_CHR_TYPE_PTY)) {
+virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("Only a single console or serial can be configured
for this domain"));
+return -1;
+} else if (def->nconsoles > 1) {
+virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("Only a single console can be configured for this
domain"));
+return -1;
+} else if (def->nserials > 1) {
+virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("Only a single serial can be configured for this
domain"));
+return -1;
+}
+
+if (def->nconsoles && def->consoles[0]->source->type !=
VIR_DOMAIN_CHR_TYPE_PTY) {
+virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("Console can only be enabled for a PTY"));
+return -1;
+}
+
+if (def->nserials && def->serials[0]->source->type !=
VIR_DOMAIN_CHR_TYPE_PTY) {
+virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("Serial can only be enabled for a PTY"));
+return -1;
+}
+
return 0;
}
--
2.31.1
[PATCH 5/9] ch_monitor: Add pty json builder function
Add function to build the the json structure to configure a PTY in
cloud-hypervisor.
The devices themselves still aren't allowed in configurations yet
though.
Signed-off-by: William Douglas
---
src/ch/ch_monitor.c | 27 +++
1 file changed, 27 insertions(+)
diff --git a/src/ch/ch_monitor.c b/src/ch/ch_monitor.c
index 28c70c7281..a01c031064 100644
--- a/src/ch/ch_monitor.c
+++ b/src/ch/ch_monitor.c
@@ -89,6 +89,30 @@ virCHMonitorBuildCPUJson(virJSONValue *content, virDomainDef
*vmdef)
return -1;
}
+static int
+virCHMonitorBuildPTYJson(virJSONValue *content, virDomainDef *vmdef)
+{
+virJSONValue *ptys = virJSONValueNewObject();
+
+if (vmdef->nconsoles) {
+g_autoptr(virJSONValue) pty = virJSONValueNewObject();
+if (virJSONValueObjectAppendString(pty, "mode", "Pty") < 0)
+return -1;
+if (virJSONValueObjectAppend(content, "console", ) < 0)
+return -1;
+}
+
+if (vmdef->nserials) {
+g_autoptr(virJSONValue) pty = virJSONValueNewObject();
+if (virJSONValueObjectAppendString(ptys, "mode", "Pty") < 0)
+return -1;
+if (virJSONValueObjectAppend(content, "serial", ) < 0)
+return -1;
+}
+
+return 0;
+}
+
static int
virCHMonitorBuildKernelRelatedJson(virJSONValue *content, virDomainDef *vmdef)
{
@@ -370,6 +394,9 @@ virCHMonitorBuildVMJson(virDomainDef *vmdef, char **jsonstr)
goto cleanup;
}
+if (virCHMonitorBuildPTYJson(content, vmdef) < 0)
+goto cleanup;
+
if (virCHMonitorBuildCPUJson(content, vmdef) < 0)
goto cleanup;
--
2.31.1
[PATCHv4 0/9] ch: Add Console support
This series enables console support in the cloud-hypervisor driver. Cloud-hypervisor only supports a single console or serial device at a time, hence the checks to ensure the domain configuration is only passing one or the other. Changes since v3: * Reset ch monitor's curl handle after a get info request * Improved PTY validation logic * Updated deviceValidateCallback to handle PTY validation * Added error reporting for unexpected PTY json data * Dropped the parallels scanning for PTY * Added a fix for handling deviceValidateCallback failures Changes since v2: * Squashed an additional patch. Changes since v1: * Added missing patch to add the virChrdev device * Added handling for multiple curl WRITEFUNCTION call backs * Added missing free for data.content * Removed redundant console configuration check * Improved handling of pty JSON data to make use of g_autoptr * Squashed two patches William Douglas (9): ch_domain: Add virChrdevs for console support ch_monitor: Make unused function static ch_monitor: Update virCHMonitorGet to handle accept a response ch_monitor: Use virCHMonitorGet to access cloud-hypervisor API ch_monitor: Add pty json builder function ch_process: Handle enabled console devices ch_domain: Allow controller and chr devices ch_driver: Add handler for console API ch_driver: Handle validation failure correctly src/ch/ch_domain.c | 40 +++- src/ch/ch_domain.h | 3 ++ src/ch/ch_driver.c | 76 - src/ch/ch_monitor.c | 91 +++-- src/ch/ch_monitor.h | 3 ++ src/ch/ch_process.c | 84 + 6 files changed, 290 insertions(+), 7 deletions(-) -- 2.31.1
Re: [PATCH v2 0/4] Add support for two i386 pm options which control acpi hotplug
Ping again … On Wed, Sep 1, 2021 at 09:03 Ani Sinha wrote: > > > On Thu, 19 Aug 2021, Ani Sinha wrote: > > > Hi: > > > > I added some negative xml2argv tests as well as new xml2xml tests. In > the process, > > I also fixed a bug where I had not added appropriate code to generate > the output > > xml correctly. > > The patch series is sent again as v2. Kindly, please provide inputs and > review them. > > > > [PATCH v2 1/4] pm/i386: add support for two options that controls > > [PATCH v2 2/4] tests: add positive xml2argv tests to exercize pm acpi > > [PATCH v2 3/4] tests: add negative xml2argv tests to exercize pm acpi > > [PATCH v2 4/4] tests: add xml2xml tests to exercize pm acpi hotplug > > Ping ... please provide some review on this patchset. > >
Re: [PATCH] qemu: remove unnecessary strlen for LINE_ENDING
On 8/31/21 11:04 PM, renlei1...@163.com wrote:
From: Ren Lei
the length of LINE_ENDING is static, it's a waste to call
strlen every time.
AFAIK gcc optimizes out calls to strlen of a literal constant string,
making this hand-optimization unnecessary.
Signed-off-by: Ren Lei
---
src/qemu/qemu_agent.c| 5 +++--
src/qemu/qemu_monitor_json.c | 3 ++-
2 files changed, 5 insertions(+), 3 deletions(-)
diff --git a/src/qemu/qemu_agent.c b/src/qemu/qemu_agent.c
index 5f421be6f6..3a453b4c58 100644
--- a/src/qemu/qemu_agent.c
+++ b/src/qemu/qemu_agent.c
@@ -47,6 +47,7 @@
VIR_LOG_INIT("qemu.qemu_agent");
#define LINE_ENDING "\n"
+#define LINE_ENDING_LENGTH 1
#define DEBUG_IO 0
#define DEBUG_RAW_IO 0
@@ -341,11 +342,11 @@ static int qemuAgentIOProcessData(qemuAgent *agent,
if (nl) {
int got = nl - (data + used);
-for (i = 0; i < strlen(LINE_ENDING); i++)
+for (i = 0; i < LINE_ENDING_LENGTH; i++)
data[used + got + i] = '\0';
if (qemuAgentIOProcessLine(agent, data + used, msg) < 0)
return -1;
-used += got + strlen(LINE_ENDING);
+used += got + LINE_ENDING_LENGTH;
} else {
break;
}
diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
index 8d3c4031a6..8b77b3cdaa 100644
--- a/src/qemu/qemu_monitor_json.c
+++ b/src/qemu/qemu_monitor_json.c
@@ -51,6 +51,7 @@ VIR_LOG_INIT("qemu.qemu_monitor_json");
#define QOM_CPU_PATH "/machine/unattached/device[0]"
#define LINE_ENDING "\r\n"
+#define LINE_ENDING_LENGTH 2
VIR_ENUM_IMPL(qemuMonitorJob,
QEMU_MONITOR_JOB_TYPE_LAST,
@@ -271,7 +272,7 @@ int qemuMonitorJSONIOProcess(qemuMonitor *mon,
int got = nl - (data + used);
char *line;
line = g_strndup(data + used, got);
-used += got + strlen(LINE_ENDING);
+used += got + LINE_ENDING_LENGTH;
line[got] = '\0'; /* kill \n */
if (qemuMonitorJSONIOProcessLine(mon, line, msg) < 0) {
VIR_FREE(line);
Re: [PATCH 0/6] qemu: Fix crash when validating a XML without disk's
On 9/7/21 3:22 PM, Peter Krempa wrote: > Apart from fixing the crash the validation code is fixed to do the > correct thing and a test case is added. > > Peter Krempa (6): > qemuDomainDefValidateDiskLunSource: Unbreak error messages > conf: validate: Move qemu-specific LUN disk validation to global > validation > conf: validate: Run global device definition validation before > callbacks > conf: Don't call 'virDomainDiskDefAssignAddress' when disk->dst is > NULL > virDomainDiskDefValidate: Move validation of disk target > qemuxml2argvtest: Add test case for missing disk '' > > src/conf/domain_conf.c| 1 + > src/conf/domain_validate.c| 94 ++- > src/conf/domain_validate.h| 2 + > src/libvirt_private.syms | 1 + > src/qemu/qemu_domain.c| 50 -- > src/qemu/qemu_domain.h| 3 - > src/qemu/qemu_driver.c| 2 +- > src/qemu/qemu_validate.c | 3 - > .../default-video-type-x86_64-caps-test-0.err | 2 +- > .../disk-fdc-incompatible-address.err | 2 +- > .../disk-ide-incompatible-address.err | 2 +- > .../disk-missing-target-invalid.err | 1 + > .../disk-missing-target-invalid.xml | 22 + > .../disk-sata-incompatible-address.err| 2 +- > .../disk-scsi-incompatible-address.err| 2 +- > .../pseries-default-phb-numa-node.err | 2 +- > .../pseries-phb-invalid-target-index-1.err| 2 +- > .../pseries-phb-invalid-target-index-2.err| 2 +- > .../pseries-phb-invalid-target-index-3.err| 2 +- > .../video-invalid-multiple-devices.err| 2 +- > ...splay-device-pci-address.x86_64-latest.err | 2 +- > tests/qemuxml2argvtest.c | 1 + > 22 files changed, 109 insertions(+), 93 deletions(-) > create mode 100644 tests/qemuxml2argvdata/disk-missing-target-invalid.err > create mode 100644 tests/qemuxml2argvdata/disk-missing-target-invalid.xml > Reviewed-by: Michal Privoznik Michal
Re: RFC: revert to external snapshot API
Hi! What do you think of this approach to implementing reverting? вт, 31 авг. 2021 г. в 16:46, Nikolay Shirokovskiy < nshirokovs...@virtuozzo.com>: > Hi, all. > > I want to implement reverting to external snapshot functionality. > I've checked the mailing list history and found some previous attempts > (not sure if this is a complete list of them). > > [1] was done in 2012 by the Redhat team itself. [2] and [3] was done in > 2018. > Looks like it was not clear how the API should look like. > > For example we have disk.qcow2 <- disk.snap1 chain <- disk.snap2 chain > (after > having snap1 and snap2 snapshots). Now we want to revert to snap1 > snapshot. The > snapshot state is held by disk.qcow2 image. We can run reverted domain on > disk.qcow2 itself but then we lose snap1 (held by disk.qcow2) and snap2 > (held by disk.snap1). So we definitely should run on some overlay over > disk.qcow2. But then what name should be given to overlay? We should have > an option for mgmt to specify this name like in case of snapshots itself. > > The [1] has some discussion on adding extra options to reverting > functionality. > Like for example if we revert back to snap2 then having the ability to run > from > state in disk.snap2 rather than disk.snap1. My opinion is there is no need > to > as if one wants to revert to the state of disk2.snap2 it can take a > snapshot (some > snap3). At the same time one needs to be aware that revert operation loses > current state and later one can revert only to the states of snapshots. > This is the way internal snapshots work and the way one expects external > snapshots to work too. > > The [2] takes an approach of reusing current top image as overlay on > revert so > that in the above example disk.snap2 will be overlay over disk.qcow2 on > reverting to snap1 snapshot. IMHO this is a confusing naming scheme. > > The [3] suggests a different scheme for naming images. For example after > taking > snapshot snap1 the chain should be disk.snap1 <- disk.qcow2 which looks > very > appealing to me. With this naming using the [2] approach is quite natural. > Implementing this does not look hard even for a running domain but this is > a big change to API and all mgmt need to be aware of (well it could be done > safely using a new flag). > > Anyway we can go on with current image names. In order to specify overlay > image name let's introduce new API: > > int virDomainRevertToSnapshotXML(virDomainSnapshotPtr snapshot, > char *xmlDesc, > unsigned int flags) > > with XML like: > > > > > > > > > > Having an XML looks like a bit overkill right now but I could not > find a better solution. > > If overlay name is omitted the generated name will be like disk.snap1-1 in > the > above example to be in alignment with creating a snapshot case. So that > disk.snap1* > images hold states derived from snap1 state. We can also support reverting > to > external snapshot thru existing virDomainRevertToSnapshot for those who > rely on > generated names using this naming scheme. > > [1] Re: [PATCHv4 4/4] snapshot: qemu: Implement reverting of external > snapshots > https://listman.redhat.com/archives/libvir-list/2012-November/msg00932.html > > [2] Re: [PATCH 1/5] snapshot: Implement reverting for external disk > snapshots > https://listman.redhat.com/archives/libvir-list/2018-November/msg00338.html > > [3] External disk snapshot paths and the revert operation > https://listman.redhat.com/archives/libvir-list/2018-October/msg01110.html >
Re: [PATCH 3/3] qemu: add virtio-blk queue-size option
On Mon, Sep 06, 2021 at 17:06:15 +0900, Hiroki Narukawa wrote:
Same as previous patches.
> ---
> src/qemu/qemu_command.c | 3 ++
> src/qemu/qemu_validate.c | 6
> .../disk-virtio-queue-size.args | 29 +++
> .../disk-virtio-queue-size.xml| 35 +++
> tests/qemuxml2argvtest.c | 2 ++
> .../disk-virtio-queue-size.xml| 35 +++
> tests/qemuxml2xmltest.c | 1 +
> 7 files changed, 111 insertions(+)
> create mode 100644 tests/qemuxml2argvdata/disk-virtio-queue-size.args
> create mode 100644 tests/qemuxml2argvdata/disk-virtio-queue-size.xml
> create mode 100644 tests/qemuxml2xmloutdata/disk-virtio-queue-size.xml
[...]
> diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c
> index 3b331d5fd4..43532c3890 100644
> --- a/tests/qemuxml2argvtest.c
> +++ b/tests/qemuxml2argvtest.c
> @@ -1341,6 +1341,8 @@ mymain(void)
> DO_TEST("disk-order", QEMU_CAPS_VIRTIO_BLK_SCSI);
> DO_TEST("disk-virtio-queues",
> QEMU_CAPS_VIRTIO_BLK_NUM_QUEUES);
> +DO_TEST("disk-virtio-queue-size",
> +QEMU_CAPS_VIRTIO_BLK_QUEUE_SIZE);
All new test cases should use DO_TEST_CAPS_LATEST instead of fake
capabilities.
If you are striving to match what is done for the 'queues' you should
convert that case first to DO_TEST_CAPS_LATEST (in a separate commit).
> DO_TEST_NOCAPS("disk-boot-disk");
> DO_TEST_NOCAPS("disk-boot-cdrom");
> DO_TEST_NOCAPS("floppy-drive-fat");
[...]
> diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c
> index 6d3526f91f..ee1ee88b85 100644
> --- a/tests/qemuxml2xmltest.c
> +++ b/tests/qemuxml2xmltest.c
> @@ -302,6 +302,7 @@ mymain(void)
> DO_TEST_NOCAPS("disk-virtio");
> DO_TEST_NOCAPS("floppy-drive-fat");
> DO_TEST("disk-virtio-queues", QEMU_CAPS_VIRTIO_BLK_NUM_QUEUES);
> +DO_TEST("disk-virtio-queue-size", QEMU_CAPS_VIRTIO_BLK_QUEUE_SIZE);
Here too.
> DO_TEST_NOCAPS("disk-boot-disk");
> DO_TEST_NOCAPS("disk-boot-cdrom");
> DO_TEST_NOCAPS("disk-error-policy");
> --
> 2.17.1
>
Re: [PATCH 2/3] qemu: add QEMU_CAPS_VIRTIO_BLK_QUEUE_SIZE capability
On Mon, Sep 06, 2021 at 17:06:14 +0900, Hiroki Narukawa wrote: This looks good, but the commit is lacking the commit message and origin certification. > ---
Re: [PATCH 1/3] qemu: accept queue_size XML element
On Mon, Sep 06, 2021 at 17:06:13 +0900, Hiroki Narukawa wrote:
Missing commit message. Please describe what and why you are adding.
You also need provide certification that your patches are in compliance
with the Developer Certificate of Origin:
https://www.libvirt.org/hacking.html#developer-certificate-of-origin
> ---
> docs/schemas/domaincommon.rng | 5 +
> src/conf/domain_conf.c| 6 ++
> src/conf/domain_conf.h| 1 +
> 3 files changed, 12 insertions(+)
Additionally this is also missing documentation for the XML format
(docs/formatdomain.rst).
[...]
> diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
> index c7e6df7981..688a842660 100644
> --- a/src/conf/domain_conf.h
> +++ b/src/conf/domain_conf.h
> @@ -584,6 +584,7 @@ struct _virDomainDiskDef {
> virDomainDiskDetectZeroes detect_zeroes;
> char *domain_name; /* backend domain name */
> unsigned int queues;
> +unsigned int queue_size;
This (along also with 'queues') feels like a guest-OS visible property
and thus must be covered by the ABI stability check [1] unless there are
good technical reasons why not.
'queues' is not currently is not covered, so for bonus points you can
fix that one too (in a separate commit).
[1] See virDomainDiskDefCheckABIStability
Re: [PATCH v2 2/3] conf: support for virtio-blk-pci discard option
On Wed, Sep 08, 2021 at 11:56:02 +0200, Peter Krempa wrote:
> On Tue, Sep 07, 2021 at 15:12:09 +0800, yuxiat...@huawei.com wrote:
> > From: yuxiating
> >
> > DISCARD and WRITE_ZEROES features for machine type >= 4.0 is enabled by
> > default
> > since 5c81161f8041("virtio-blk: add "discard" and "write-zeroes"
> > properties).
> > Virtio_blk kernel driver has a bug that causes memory corruption in
> > virtblk_setup_discard_write_zeroes();
> > af822aa68fbd ("block: virtio_blk: fix handling single range discard
> > request")
> > has fix it.
> > However, some operating systems are not fixed and need to disabled on the
> > QEMU side.
>
> I must say that I'm not persuaded that there's enough value in this
> workaround. Similarly users could use virtio-scsi instead as a
> workaround or fix their OS.
>
> For me this reasoning will not be enough, but other on the list might
> think otherwise.
I want to elaborate a bit. Detecting whether a guest OS has a fix is
hard and additionally the guest can be patched without restart of the VM
where the user would lose discards even when it isn't a problem any
more.
This brings me to another thing I forgot to point out. Using
discard_enable='off' must be made incompatible with discard='unmap'.
e.g.
XML must be rejected by the validator as discards would not be
propagated to the file even on an explicit request of the user.
I'm also slightly unhappy that we are stashing frontend config under
but there's prior art in doing that so that doesn't need to be
changed.
Re: [PATCH v2 3/3] qemu: command: support for virtio-blk-pci discard option
On Tue, Sep 07, 2021 at 15:12:10 +0800, yuxiat...@huawei.com wrote:
> From: yuxiating
>
> This patch introduces a new attribute "discard". An example of
> the XML:
>
>
>
>
> The corresponding QEMU command line:
>
> -device virtio-blk-pci,scsi=off,discard=off,id=virtio-disk0
>
> Signed-off-by: yuxiating
> ---
> src/qemu/qemu_command.c | 5 +++
> .../qemuxml2argvdata/disk-virtio-discard.args | 29 +++
> .../qemuxml2argvdata/disk-virtio-discard.xml | 35 +++
> tests/qemuxml2argvtest.c | 2 ++
> 4 files changed, 71 insertions(+)
> create mode 100644 tests/qemuxml2argvdata/disk-virtio-discard.args
> create mode 100644 tests/qemuxml2argvdata/disk-virtio-discard.xml
[...]
> diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c
> index 3b331d5fd4..4375b06d40 100644
> --- a/tests/qemuxml2argvtest.c
> +++ b/tests/qemuxml2argvtest.c
> @@ -1341,6 +1341,8 @@ mymain(void)
> DO_TEST("disk-order", QEMU_CAPS_VIRTIO_BLK_SCSI);
> DO_TEST("disk-virtio-queues",
> QEMU_CAPS_VIRTIO_BLK_NUM_QUEUES);
> +DO_TEST("disk-virtio-discard",
> +QEMU_CAPS_DEVICE_DISCARD);
Please always use DO_TEST_CAPS_LATEST with new test cases as it covers
real qemu capabilities.
> DO_TEST_NOCAPS("disk-boot-disk");
> DO_TEST_NOCAPS("disk-boot-cdrom");
> DO_TEST_NOCAPS("floppy-drive-fat");
> --
> 2.27.0
>
Re: [PATCH v2 2/3] conf: support for virtio-blk-pci discard option
On Tue, Sep 07, 2021 at 15:12:09 +0800, yuxiat...@huawei.com wrote:
> From: yuxiating
>
> DISCARD and WRITE_ZEROES features for machine type >= 4.0 is enabled by
> default
> since 5c81161f8041("virtio-blk: add "discard" and "write-zeroes" properties).
> Virtio_blk kernel driver has a bug that causes memory corruption in
> virtblk_setup_discard_write_zeroes();
> af822aa68fbd ("block: virtio_blk: fix handling single range discard request")
> has fix it.
> However, some operating systems are not fixed and need to disabled on the
> QEMU side.
I must say that I'm not persuaded that there's enough value in this
workaround. Similarly users could use virtio-scsi instead as a
workaround or fix their OS.
For me this reasoning will not be enough, but other on the list might
think otherwise.
> Signed-off-by: yuxiating
> ---
> docs/formatdomain.rst | 3 +++
> docs/schemas/domaincommon.rng | 8
> src/conf/domain_conf.c| 8
> src/conf/domain_conf.h| 1 +
> src/conf/domain_validate.c| 6 ++
> 5 files changed, 26 insertions(+)
[...]
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 6127513117..304015f42e 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
[...]
> @@ -23416,6 +23420,10 @@ virDomainDiskDefFormatDriver(virBuffer *buf,
> if (disk->queues)
> virBufferAsprintf(, " queues='%u'", disk->queues);
>
> +if (disk->discard_enable)
We tend to use explicit check:
if (disk->discard_enable != VIR_TRISTATE_BOOL_ABSENT)
> +virBufferAsprintf(, " discard_enable='%s'",
> +
> virTristateSwitchTypeToString(disk->discard_enable));
> +
> virDomainVirtioOptionsFormat(, disk->virtio);
>
> if (disk->src->metadataCacheMaxSize > 0) {
This definitely seems like a feature visible to the guest OS and thus
_must_ be covered by the ABI stability check to prevent regressions in
the ABI.
See 'virDomainDiskDefCheckABIStability'.
Re: RFC: revert to external snapshot API
On Tue, Aug 31, 2021 at 16:46:25 +0300, Nikolay Shirokovskiy wrote: > Hi, all. Hi, sorry for the late reply I was on PTO. > I want to implement reverting to external snapshot functionality. > I've checked the mailing list history and found some previous attempts > (not sure if this is a complete list of them). > > [1] was done in 2012 by the Redhat team itself. [2] and [3] was done in > 2018. > Looks like it was not clear how the API should look like. One additional thing is that me and phrdina started discussing this (in person so I can't point you to a discussion) 2 weeks ago. I'll summarize the points we agreed upon. > For example we have disk.qcow2 <- disk.snap1 chain <- disk.snap2 chain > (after > having snap1 and snap2 snapshots). Now we want to revert to snap1 snapshot. There's one implementation snag we currently have which complicates stuff. Let's expand your above scenario with the snapshot states: s s n n a a p p 1 2 p o │ │ r rbase.qcow2│snap1.qcow2 │snap2.qcow2e i ───► │ ►│ ► s g │ │ e i │ │ n n t A rather big set of problems which we will necessarily encounter when implementing this comes from the following: 1) users can modify the VM betwen snapshots or between the last snapshot and the abandoned state and this modification (e.g. changing a disk image) must be considered to prevent data loss when manipulating the images. 2) libvirt doesn't record the XML used to create the snapshot (e.g. snap1.xm) thus we don't actually know what the (historical) snapshot was actually recording. Coupled with the fact that the user could have changed the VM definition between 'snap1' and 'snap2' we can't even infer what the state was. > The > snapshot state is held by disk.qcow2 image. We can run reverted domain on > disk.qcow2 itself but then we lose snap1 (held by disk.qcow2) and snap2 > (held by disk.snap1). So we definitely should run on some overlay over > disk.qcow2. But then what name should be given to overlay? We should have > an option for mgmt to specify this name like in case of snapshots itself. Exactly. Reversion of external snapshots will necessarily require a new API, which will take a new "snapshot" XML describing the new overlays as you describe below. In the simple case such as with local files we can use the same algorithm for creating overlay filenames as we do when creating snapshots but generally we need to give the MGMT the ability to specify the overlay name. > The [1] has some discussion on adding extra options to reverting > functionality. > Like for example if we revert back to snap2 then having the ability to run > from > state in disk.snap2 rather than disk.snap1. My opinion is there is no need > to > as if one wants to revert to the state of disk2.snap2 it can take a > snapshot (some > snap3). It's possible to avoid doing a combined "take snapshot" operation as long as we have the possibility to take a snapshot and destroy the VM as running it after the point you want to return to is undesirable and pointless. One thing that you need to consider here is that when you are reverting to an arbitrary snapshot, the overlay files after the last snapshot (e.g. snap2.qcow2 (in my diagram), which is the state between snap2 and present) are abandoned and will never be used again. At this point we disucssed that we should be removing those as semantically it's the only point where we can do that as we know the state of the VM which will be abandoned. The problem lies in the fact that between 'snap2' and present the user could have exchanged disks and then we don't have enough metadata to create the overlays. One big additional caveat is that if the user exchanged disk images we _must not_ delete the changed image, so the metadata wrangling might be non-trivial in this case. > At the same time one needs to be aware that revert operation loses > current state and later one can revert only to the states of snapshots. > This is the way internal snapshots work and the way one expects external > snapshots to work too. That is an acceptable caveat for the user. As noted above as long as you can take a snapshot & atomicaly destroy the VM it's acceptable. For libvirt it's harder a bit as described above especially if we don't want to keep litering the disk with unused and invalid images. > The
