[PATCH V2 0/3] qemu: Improve opening and verifying save images
by Jim Fehlig
V2 of https://lists.libvirt.org/archives/list/devel@lists.libvirt.org/message/D...
Changes in V2:
* Move unlinking corrupt save images from qemuSaveImageOpen to the only
caller using that functionality
* Add a function to read save image header
* Correctly position file pointer of save image for QEMU
Jim Fehlig (3):
qemu: Move unlinking corrupt save image file to caller
qemu: Decompose qemuSaveImageOpen
qemu: Check for valid save image format when verifying image header
src/qemu/qemu_driver.c | 46 ++++---
src/qemu/qemu_saveimage.c | 268 +++++++++++++++++++++++---------------
src/qemu/qemu_saveimage.h | 21 ++-
src/qemu/qemu_snapshot.c | 9 +-
4 files changed, 207 insertions(+), 137 deletions(-)
--
2.43.0
2 months, 3 weeks
[PATCH 0/2] qemu: Improve opening and verifying save images
by Jim Fehlig
qemuSaveImageOpen does a lot of stuff. When bypass-cache is specified,
it creates a virFileWrapperFd, which is not cleaned up in the subsequent
failure paths. This results in errors from the iohelper, which can
overwrite the actual error. E.g. consider libvirt attempting to restore
an image saved in an unknown format
# virsh restore --bypass-cache /data/test.sav.sparse
error: Failed to restore domain from /data/test.sav.sparse
error: internal error: Child process (LIBVIRT_LOG_OUTPUTS=1:stderr /usr/lib64/libvirt/libvirt_iohelper /data/test.sav.sparse 0) unexpected fatal signal 13
When not using the iohelper, and not creating a virFileWrapperFd, we see
the real error:
# virsh restore /data/test.sav.sparse
error: Failed to restore domain from /data/test.sav.sparse
error: operation failed: Invalid compressed save format 6
Although that error highlights a spot I missed when removing the
"compression" implications around the 'foo_image_format' settings in
qemu.conf with commit bd6d7ebf622 :-). IMO, the error message would be
best fixed by checking for valid values when reading the save image
metadata.
Patch 1 decomposes qemuSaveImageOpen to allow for better error handling
and more flexibility. Patch 2 checks for a valid format when checking the
other header fields.
Jim Fehlig (2):
qemu: Decompose qemuSaveImageOpen
qemu: Check for valid save image format when verifying image header
src/qemu/qemu_driver.c | 37 +++++++--------
src/qemu/qemu_saveimage.c | 95 +++++++++++++++++++++++++--------------
src/qemu/qemu_saveimage.h | 16 ++++---
src/qemu/qemu_snapshot.c | 9 ++--
4 files changed, 95 insertions(+), 62 deletions(-)
--
2.43.0
2 months, 3 weeks
[PATCH 0/2] finish sysusers support
by Daniel P. Berrangé
This fixes another missing sysusers file and changes the
RPM spec to take account of new RPM processing of sysusers
files at install time.
Daniel P. Berrangé (2):
tools: add sysusers file to create 'virtlogin' group
rpm: disable account creation for Fedora >= 42
libvirt.spec.in | 19 +++++++++++++++++++
tools/libvirt-login-shell.sysusers.conf | 1 +
tools/meson.build | 7 +++++++
3 files changed, 27 insertions(+)
create mode 100644 tools/libvirt-login-shell.sysusers.conf
--
2.47.1
2 months, 3 weeks
[RFC PATCH 0/5] qemu: Route hostdevs to multiple nested SMMUs
by Nathan Chen
Hi,
This is a draft solution for supporting multiple vSMMU instances in a qemu VM.
Based on discussions/suggestions received for a previous RFC by Nicolin here[0],
the association of vSMMUs to VFIO devices in VM PCIe topology should be moved
out of qemu into libvirt. In addition, the nested SMMU nodes should be passed
to qemu as pluggable devices.
To address these changes, this patch series introduces a new "nestedSmmuv3"
IOMMU model and "nestedSmmuv3" device type. Upon specifying the nestedSmmuv3
IOMMU model, nestedSmmuv3 devices will be auto-added to the VM definition based
on the available SMMU nodes in the host's sysfs. The nestedSmmuv3 devices will
each be attached to a separate PXB controller, and VFIO devices will be routed
to PXBs based on their association with host SMMU nodes. This will maintain a VM
PCIe topology that allows for multiple nested SMMUs per Nicolin's original qemu
patch series in [0] and Shameer's work in [1] to remove VM topology changes from
qemu and allow the nested SMMUs to be specified as pluggable devices.
For instance, if we specify the nestedSmmuv3 IOMMU model and a hostdev for
passthrough:
<devices>
<hostdev mode='subsystem' type='pci' managed='no'>
<source>
<address domain='0x0009' bus='0x01' slot='0x00' function='0x0'/>
</source>
</hostdev>
<iommu model='nestedSmmuv3'/>
</devices>
Libvirt will scan sysfs and populate the VM definition with controllers and
nestedSmmuv3 devices based on host config. So if
/sys/bus/pci/devices/0009:01:00.0/iommu is a symlink to the host SMMU node
represented by
/sys/devices/platform/arm-smmu-v3.8.auto/iommu/smmu3.0x0000000016000000
and there are 3 host SMMU nodes under /sys/class/iommu/, we'll see three
auto-added nestedSmmuv3 devices, each routed to a pcie-expander-bus controller.
Then the hostdev will be routed to a PXB controller that has a matching host
SMMU node associated with it:
<devices>
...
<controller type='pci' index='1' model='pcie-expander-bus'>
<model name='pxb-pcie'/>
<target busNr='254'/>
<address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x0'/>
</controller>
<controller type='pci' index='2' model='pcie-expander-bus'>
<model name='pxb-pcie'/>
<target busNr='251'/>
<address type='pci' domain='0x0000' bus='0x00' slot='0x02' function='0x0'/>
</controller>
<controller type='pci' index='3' model='pcie-expander-bus'>
<model name='pxb-pcie'/>
<target busNr='249'/>
<address type='pci' domain='0x0000' bus='0x00' slot='0x03' function='0x0'/>
</controller>
<controller type='pci' index='4' model='pcie-root-port'>
<model name='pcie-root-port'/>
<target chassis='7' port='0x8'/>
<address type='pci' domain='0x0000' bus='0x02' slot='0x01' function='0x0'/>
</controller>
<hostdev mode='subsystem' type='pci' managed='no'>
<source>
<address domain='0x0009' bus='0x01' slot='0x00' function='0x0'/>
</source>
<address type='pci' domain='0x0000' bus='0x04' slot='0x00' function='0x0'/>
</hostdev>
<iommu model='nestedSmmuv3'/>
<nestedSmmuv3>
<name>smmu3.0x0000000012000000</name>
<address type='pci' domain='0x0000' bus='0x01' slot='0x00' function='0x0'/>
</nestedSmmuv3>
<nestedSmmuv3>
<name>smmu3.0x0000000016000000</name>
<address type='pci' domain='0x0000' bus='0x02' slot='0x00' function='0x0'/>
</nestedSmmuv3>
<nestedSmmuv3>
<name>smmu3.0x0000000011000000</name>
<address type='pci' domain='0x0000' bus='0x03' slot='0x00' function='0x0'/>
</nestedSmmuv3>
<iommu model='nestedSmmuv3'/>
</devices>
TODO:
- No DMA mapping can found by UEFI when specifying multiple passthrough devices
in the VM definition, and VM boot is subsequently blocked. We need to
investigate this for the next revision, but we don't encounter this issue when
passing through a single device. We'll include iommufd support in the next
revision to narrow down whether the required fix would be outside of libvirt.
- Shameer's qemu branch specifies nestedSmmuv3 bus number with "pci-bus"
instead of "bus", so the libvirt compilation test args and qemu args in
qemuBuildPCINestedSmmuv3DevProps() need to be modified to match this revision
of qemu. It will be reverted to using "bus" in the next qemu revision.
- This patchset decrements PXB busNr based on how many devices are attached
downstream, and the libvirt documentation states we must reserve busNr for the
PXB itself in addition to any devices attached downstream. When I launch a VM
and a PXB has a pcie-root-port and hostdev attached downstream, busNrs 253,
252, and 251 are reserved. But the PXB itself already has a bus number
assigned via the <address/> attribute, and I see 253 and 252 assigned to the
hostdev and pcie-root-port in the VM but not 251. Should we decrement busNr
based on libvirt documentation or do we only need two busNrs 253 and 252 in
the example here?
This series is on Github:
https://github.com/NathanChenNVIDIA/libvirt/tree/nested-smmuv3-12-05-24
Thanks,
Nathan
[0] https://lore.kernel.org/qemu-devel/cover.1719361174.git.nicolinc@nvidia.com/
[1] https://lore.kernel.org/qemu-devel/20241108125242.60136-1-shameerali.kolo...
Signed-off-by: Nathan Chen <nathanc(a)nvidia.com>
Nathan Chen (5):
conf: Add a nestedSmmuv3 IOMMU model
qemu: Implement and auto-add a nestedSmmuv3 device type
qemu: Create PXBs and auto-assign VFIO devs and nested SMMUs
qemu: Update PXB busNr for nestedSmmuv3 controllers
qemu: Add test case for specifying multiple nested SMMUs
docs/formatdomain.rst | 25 ++-
src/ch/ch_domain.c | 1 +
src/conf/domain_addr.c | 26 ++-
src/conf/domain_addr.h | 4 +-
src/conf/domain_conf.c | 188 +++++++++++++++++
src/conf/domain_conf.h | 15 ++
src/conf/domain_postparse.c | 1 +
src/conf/domain_validate.c | 24 +++
src/conf/schemas/domaincommon.rng | 17 ++
src/conf/virconftypes.h | 2 +
src/libvirt_private.syms | 2 +
src/lxc/lxc_driver.c | 6 +
src/qemu/qemu_command.c | 64 +++++-
src/qemu/qemu_command.h | 4 +
src/qemu/qemu_domain.c | 2 +
src/qemu/qemu_domain_address.c | 193 ++++++++++++++++++
src/qemu/qemu_driver.c | 3 +
src/qemu/qemu_hotplug.c | 5 +
src/qemu/qemu_postparse.c | 1 +
src/qemu/qemu_validate.c | 16 ++
src/test/test_driver.c | 4 +
tests/meson.build | 1 +
.../iommu-nestedsmmuv3.aarch64-latest.args | 38 ++++
.../iommu-nestedsmmuv3.aarch64-latest.xml | 61 ++++++
tests/qemuxmlconfdata/iommu-nestedsmmuv3.xml | 29 +++
tests/qemuxmlconftest.c | 4 +-
tests/schemas/device.rng.in | 1 +
tests/virnestedsmmuv3mock.c | 57 ++++++
28 files changed, 788 insertions(+), 6 deletions(-)
create mode 100644 tests/qemuxmlconfdata/iommu-nestedsmmuv3.aarch64-latest.args
create mode 100644 tests/qemuxmlconfdata/iommu-nestedsmmuv3.aarch64-latest.xml
create mode 100644 tests/qemuxmlconfdata/iommu-nestedsmmuv3.xml
create mode 100644 tests/virnestedsmmuv3mock.c
--
2.34.1
2 months, 3 weeks
[PATCH v2 00/15] qemu: Store I/O error messages for disks
by Peter Krempa
v2:
- ACK'd patches were pushed
- the QEMU 'qom-path' regression is no longer mentioned
- it's worker around by prefering lookup via node-name
- the patch is justified without the need to mention it
- 'qom-path' is now properly used for lookup of the disk
- I/O error messages are now stored in virStorageSource
- the messages can be viewed via virDomainGetMessages()
- various refactors to make the addition clean added
- news added
Peter Krempa (15):
qemu: Handle quirks of 'device' field of BLOCK_IO_ERROR event in
monitor code
qemu: Rename 'diskAlias' to 'device' in qemu IO error event handling
qemuProcessHandleIOError: Rename local variables
qemuMonitorJSONHandleIOError: Do not munge 'reason' field of IO error
event
qemuProcessHandleIOError: Prefer lookup by node name
qemuMonitorJSONHandleIOError: Propagate new 'qom-path' field
virStorageSource: Add fields for storing last I/O error message
qemuProcessHandleIOError: Populate I/O error reason to
virStorageSource
qemuProcessHandleIOError: Log IO errors in the VM log file
libxlDomainGetMessages: Add existing flags to 'virCheckFlags'
virDomainObjGetMessages: Refactor using GPtrArray
virDomainGetMessages: Introduce VIR_DOMAIN_MESSAGE_IOERRORS
include: libvirt-domain: Reword documentation for @reason of
VIR_DOMAIN_EVENT_ID_IO_ERROR_REASON
include: libvirt-domain: Add 'hypervisor-message' @reason of
VIR_DOMAIN_EVENT_ID_IO_ERROR_REASON
NEWS: Mention preserving I/O error messages for qemu VMs
NEWS.rst | 6 +++
include/libvirt/libvirt-domain.h | 19 ++++++--
src/conf/domain_conf.c | 80 +++++++++++++++++++++-----------
src/conf/domain_conf.h | 9 +++-
src/conf/storage_source_conf.c | 5 ++
src/conf/storage_source_conf.h | 6 +++
src/libvirt_private.syms | 1 +
src/libxl/libxl_driver.c | 13 +++++-
src/qemu/qemu_driver.c | 24 +++++++++-
src/qemu/qemu_monitor.c | 6 ++-
src/qemu/qemu_monitor.h | 8 +++-
src/qemu/qemu_monitor_json.c | 23 ++++++---
src/qemu/qemu_process.c | 61 +++++++++++++++---------
src/test/test_driver.c | 12 ++++-
14 files changed, 202 insertions(+), 71 deletions(-)
--
2.48.1
2 months, 4 weeks
[PATCH] glibcompat: Updating "backport" 'g_string_replace'
by Adam Julis
Update the vir_g_string_replace with following commits from glib:
c9e48947e gstring: Fix a heap buffer overflow in the new
g_string_replace() code
e8517e777 remove quadratic behavior in g_string_replace
Signed-off-by: Adam Julis <ajulis(a)redhat.com>
---
src/util/glibcompat.c | 125 +++++++++++++++++++++++++++++++++++-------
1 file changed, 105 insertions(+), 20 deletions(-)
diff --git a/src/util/glibcompat.c b/src/util/glibcompat.c
index bcb666992a..47e3edef13 100644
--- a/src/util/glibcompat.c
+++ b/src/util/glibcompat.c
@@ -65,7 +65,7 @@
/**
* Adapted (to pass syntax check) from 'g_string_replace' from
- * glib-2.81.1. Drop once minimum glib is bumped to 2.68.
+ * glib-2.83.3. Drop once minimum glib is bumped to 2.68.
*
* g_string_replace:
* @string: a #GString
@@ -94,35 +94,120 @@ vir_g_string_replace(GString *string,
const gchar *replace,
guint limit)
{
- gsize f_len, r_len, pos;
- gchar *cur, *next;
- guint n = 0;
+ GString *new_string = NULL;
+ gsize f_len, r_len, new_len;
+ gchar *cur, *next, *first, *dst;
+ guint n;
g_return_val_if_fail(string != NULL, 0);
g_return_val_if_fail(find != NULL, 0);
g_return_val_if_fail(replace != NULL, 0);
+ first = strstr(string->str, find);
+
+ if (first == NULL)
+ return 0;
+
+ new_len = string->len;
f_len = strlen(find);
r_len = strlen(replace);
- cur = string->str;
- while ((next = strstr(cur, find)) != NULL) {
- pos = next - string->str;
- g_string_erase(string, pos, f_len);
- g_string_insert(string, pos, replace);
- cur = string->str + pos + r_len;
- n++;
- /* Only match the empty string once at any given position, to
- * avoid infinite loops */
- if (f_len == 0) {
- if (cur[0] == '\0')
- break;
- else
- cur++;
+ /* It removes a lot of branches and possibility for infinite loops if we
+ * handle the case of an empty @find string separately. */
+ if (G_UNLIKELY(f_len == 0)) {
+ size_t i;
+ if (limit == 0 || limit > string->len) {
+ if (string->len > G_MAXSIZE - 1)
+ g_error("inserting in every position in string would overflow");
+
+ limit = string->len + 1;
+ }
+
+ if (r_len > 0 &&
+ (limit > G_MAXSIZE / r_len ||
+ limit * r_len > G_MAXSIZE - string->len))
+ g_error("inserting in every position in string would overflow");
+
+ new_len = string->len + limit * r_len;
+ new_string = g_string_sized_new(new_len);
+ for (i = 0; i < limit; i++) {
+ g_string_append_len(new_string, replace, r_len);
+ if (i < string->len)
+ g_string_append_c(new_string, string->str[i]);
}
- if (n == limit)
- break;
+ if (limit < string->len)
+ g_string_append_len(new_string, string->str + limit, string->len - limit);
+
+ g_free(string->str);
+ string->allocated_len = new_string->allocated_len;
+ string->len = new_string->len;
+ string->str = g_string_free(g_steal_pointer(&new_string), FALSE);
+
+ return limit;
}
+ /* Potentially do two passes: the first to calculate the length of the new string,
+ * new_len, if it’s going to be longer than the original string; and the second to
+ * do the replacements. The first pass is skipped if the new string is going to be
+ * no longer than the original.
+ *
+ * The second pass calls various g_string_insert_len() (and similar) methods
+ * which would normally potentially reallocate string->str, and hence
+ * invalidate the cur/next/first/dst pointers. Because we’ve pre-calculated
+ * the new_len and do all the string manipulations on new_string, that
+ * shouldn’t happen. This means we scan `string` while modifying
+ * `new_string`. */
+ do {
+ dst = first;
+ cur = first;
+ n = 0;
+ while ((next = strstr(cur, find)) != NULL) {
+ n++;
+
+ if (r_len <= f_len) {
+ memmove(dst, cur, next - cur);
+ dst += next - cur;
+ memcpy(dst, replace, r_len);
+ dst += r_len;
+ } else {
+ if (new_string == NULL) {
+ new_len += r_len - f_len;
+ } else {
+ g_string_append_len(new_string, cur, next - cur);
+ g_string_append_len(new_string, replace, r_len);
+ }
+ }
+ cur = next + f_len;
+
+ if (n == limit)
+ break;
+ }
+
+ /* Append the trailing characters from after the final instance of @find
+ * in the input string. */
+ if (r_len <= f_len) {
+ /* First pass skipped. */
+ gchar *end = string->str + string->len;
+ memmove(dst, cur, end - cur);
+ end = dst + (end - cur);
+ *end = 0;
+ string->len = end - string->str;
+ break;
+ } else {
+ if (new_string == NULL) {
+ /* First pass. */
+ new_string = g_string_sized_new(new_len);
+ g_string_append_len(new_string, string->str, first - string->str);
+ } else {
+ /* Second pass. */
+ g_string_append_len(new_string, cur, (string->str + string->len) - cur);
+ g_free(string->str);
+ string->allocated_len = new_string->allocated_len;
+ string->len = new_string->len;
+ string->str = g_string_free(g_steal_pointer(&new_string), FALSE);
+ break;
+ }
+ }
+ } while (1);
return n;
}
--
2.47.1
2 months, 4 weeks
[PATCH] remote: add sysusers file to create 'libvirt' group
by Daniel P. Berrangé
We previously added a sysusers file, but missed the 'libvirt' group.
This group is referenced in the polkit rules, so we should be
registering that too. It must be done in a separate sysusers file,
however, since it is common to all daemons.
Fixes: a2c3e390f7bedf36f4ddc544d09fe3b8772c5c6f
Signed-off-by: Daniel P. Berrangé <berrange(a)redhat.com>
---
libvirt.spec.in | 1 +
src/remote/libvirt.sysusers.conf | 1 +
src/remote/meson.build | 7 +++++++
3 files changed, 9 insertions(+)
create mode 100644 src/remote/libvirt.sysusers.conf
diff --git a/libvirt.spec.in b/libvirt.spec.in
index a82c366334..5c5d36966d 100644
--- a/libvirt.spec.in
+++ b/libvirt.spec.in
@@ -2110,6 +2110,7 @@ exit 0
%{_datadir}/polkit-1/actions/org.libvirt.unix.policy
%{_datadir}/polkit-1/actions/org.libvirt.api.policy
%{_datadir}/polkit-1/rules.d/50-libvirt.rules
+%{_sysusersdir}/libvirt.conf
%dir %attr(0700, root, root) %{_localstatedir}/log/libvirt/
%attr(0755, root, root) %{_libexecdir}/libvirt_iohelper
%attr(0755, root, root) %{_bindir}/virt-ssh-helper
diff --git a/src/remote/libvirt.sysusers.conf b/src/remote/libvirt.sysusers.conf
new file mode 100644
index 0000000000..50c6716cce
--- /dev/null
+++ b/src/remote/libvirt.sysusers.conf
@@ -0,0 +1 @@
+g libvirt -
diff --git a/src/remote/meson.build b/src/remote/meson.build
index ea063ed6cc..e503263266 100644
--- a/src/remote/meson.build
+++ b/src/remote/meson.build
@@ -310,6 +310,13 @@ if conf.has('WITH_REMOTE')
)
endif
+ # Install the sysuser config for the daemon polkit rules
+ install_data(
+ 'libvirt.sysusers.conf',
+ install_dir: sysusersdir,
+ rename: [ 'libvirt.conf' ],
+ )
+
virt_helpers += {
'name': 'virt-ssh-helper',
'sources': [
--
2.47.1
2 months, 4 weeks
Re: [PATCH] Сheck snapshot disk is not NULL when searching it in the VM config
by Peter Krempa
On Mon, May 20, 2024 at 14:48:47 +0000, Efim Shevrin via Devel wrote:
> Hello,
>
> > If vmdisk is NULL, shouldn't this function (qemuSnapshotDeleteValidate()) return an error?
>
> I think this qemuSnapshotDeleteValidate should not return an error.
>
> It seems to me that when vmdisk is NULL, this does not invalidate
> the snapshot itself, but indicates that the config has changed since
> the snapshot was done. And if the VM config has changed, this adds evidence that the snapshot should be deleted,
> because the snapshot does not reflect the real vm config.
>
> Since we do not have an analogue of the --force option for deleting a snapshot, in the case when qemuSnapshotDeleteValidate returns
> an error when vmdisk is NULL, we will never delete a snapshot which has invalid disk.
Snapshot deletion does have something that can be considered force and
that is the '--metadata' option that removes just the snapshot
definition (metadata) and doesn't touch the disk images.
> > Similarly, disk can be NULL too
> Thank you for the comment regarding the disk variable. I`ve reworked patch.
>
> When creating a snapshot of a VM with multiple hard disks,
> the snapshot takes into account the presence of all disks
> in the system. If, over time, one of the disks is deleted,
> the snapshot will continue to store knowledge of the deleted disk.
> This results in the fact that at the moment of deleting the snapshot,
> at the validation stage, a disk from the snapshot will be searched which
> is not in the VM configuration. As a result, vmdisk variable will
> be equal to NULL. Dereferencing a null pointer at the time of calling
> virStorageSourceIsSameLocation(vmdisk->src, disk->src)
> will result in SIGSEGV.
Crashing is obviously not okay ...
> Also, the disk variable can also be equal to NULL and this
> requires to check that disk != NULL before calling the
> virStorageSourceIsSameLocation function to avoid SIGSEGV.
.. but going ahead with the snapshot deletion isn't always okay either.
The disk isn't referenced by the VM so the disk state can't be merged,
while the state would be merged for any other disk.
When reverting back to a previous snapshot, which is still referencing
the older state of the disk which was removed from the VM, the VM would
see that the image state of disks that were present at deletion would
contain the merged state, but only a partial state for the disk which
was later removed.
2 months, 4 weeks
[PATCH] qemuSnapshotDeleteValidate: Fix crash when disk is not found in VM definition
by kaihuan
qemuDomainDiskByName() can return a NULL pointer on failure.
But this returned value in qemuSnapshotDeleteValidate is not checked.It will make libvirtd crash.
Signed-off-by: kaihuan <jungleman759(a)gmail.com>
---
src/qemu/qemu_snapshot.c | 15 +++++++++++++--
1 file changed, 13 insertions(+), 2 deletions(-)
diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c
index 18b2e478f6..bcbd913073 100644
--- a/src/qemu/qemu_snapshot.c
+++ b/src/qemu/qemu_snapshot.c
@@ -4242,8 +4242,19 @@ qemuSnapshotDeleteValidate(virDomainObj *vm,
virDomainDiskDef *vmdisk = NULL;
virDomainDiskDef *disk = NULL;
- vmdisk = qemuDomainDiskByName(vm->def, snapDisk->name);
- disk = qemuDomainDiskByName(snapdef->parent.dom, snapDisk->name);
+ if (!(vmdisk = qemuDomainDiskByName(vm->def, snapDisk->name))) {
+ virReportError(VIR_ERR_OPERATION_FAILED,
+ _("disk '%1$s' referenced by snapshot '%2$s' not found in the current definition"),
+ snapDisk->name, snap->def->name);
+ return -1;
+ }
+
+ if (!(disk = qemuDomainDiskByName(snapdef->parent.dom, snapDisk->name))) {
+ virReportError(VIR_ERR_OPERATION_FAILED,
+ _("disk '%1$s' referenced by snapshot '%2$s' not found in the VM definition of the deleted snapshot"),
+ snapDisk->name, snap->def->name);
+ return -1;
+ }
if (!virStorageSourceIsSameLocation(vmdisk->src, disk->src)) {
virReportError(VIR_ERR_OPERATION_UNSUPPORTED,
--
2.33.1.windows.1
2 months, 4 weeks
[PATCH] Support IDE/SATA disk 'product' parameter
by Adam Julis
Since we supported 'product' parameter for SCSI, just expanded existing
solution makes IDE/SATA parameter works too. QEMU requires parameter 'model'
in case of IDE/SATA (instead of 'product'), so the process of making JSON
object is slightly modified for that.
Resolves: https://gitlab.com/libvirt/libvirt/-/issues/697
Signed-off-by: Adam Julis <ajulis(a)redhat.com>
---
docs/formatdomain.rst | 7 ++--
src/qemu/qemu_command.c | 11 +++++-
src/qemu/qemu_validate.c | 14 ++++++--
...disk-product-build-error.x86_64-latest.err | 1 +
.../disk-scsi-disk-product-build-error.xml | 34 +++++++++++++++++++
...-disk-vendor-build-error.x86_64-latest.err | 1 +
... => disk-scsi-disk-vendor-build-error.xml} | 0
...csi-disk-vpd-build-error.x86_64-latest.err | 1 -
.../disk-scsi-disk-vpd.x86_64-latest.args | 4 +--
.../disk-scsi-disk-vpd.x86_64-latest.xml | 7 ++--
tests/qemuxmlconfdata/disk-scsi-disk-vpd.xml | 4 +--
tests/qemuxmlconftest.c | 3 +-
12 files changed, 71 insertions(+), 16 deletions(-)
create mode 100644 tests/qemuxmlconfdata/disk-scsi-disk-product-build-error.x86_64-latest.err
create mode 100644 tests/qemuxmlconfdata/disk-scsi-disk-product-build-error.xml
create mode 100644 tests/qemuxmlconfdata/disk-scsi-disk-vendor-build-error.x86_64-latest.err
rename tests/qemuxmlconfdata/{disk-scsi-disk-vpd-build-error.xml => disk-scsi-disk-vendor-build-error.xml} (100%)
delete mode 100644 tests/qemuxmlconfdata/disk-scsi-disk-vpd-build-error.x86_64-latest.err
diff --git a/docs/formatdomain.rst b/docs/formatdomain.rst
index 60bee8bd4f..c93a321401 100644
--- a/docs/formatdomain.rst
+++ b/docs/formatdomain.rst
@@ -3551,12 +3551,13 @@ paravirtualized driver is specified via the ``disk`` element.
:since:`Since 0.10.1`
``vendor``
If present, this element specifies the vendor of a virtual hard disk or
- CD-ROM device. It must not be longer than 8 printable characters.
- :since:`Since 1.0.1`
+ CD-ROM device. It must not be longer than 8 printable characters. Only for
+ devices using 'scsi' ``bus``. :since:`Since 1.0.1`
``product``
If present, this element specifies the product of a virtual hard disk or
CD-ROM device. It must not be longer than 16 printable characters.
- :since:`Since 1.0.1`
+ Only for devices using 'scsi' (:since:`Since 1.0.1`), 'sata' or 'ide' ``bus``.
+ :since:`Since 11.0.0`
``address``
If present, the ``address`` element ties the disk to a given slot of a
controller (the actual ``<controller>`` device can often be inferred by
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index dcb9c4934e..5c38858f5d 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -1628,6 +1628,11 @@ qemuBuildDiskDeviceProps(const virDomainDef *def,
else
driver = "ide-hd";
+ if (virJSONValueObjectAdd(&props,
+ "S:model", disk->product,
+ NULL) < 0)
+ return NULL;
+
break;
case VIR_DOMAIN_DISK_BUS_SCSI:
@@ -1654,6 +1659,11 @@ qemuBuildDiskDeviceProps(const virDomainDef *def,
}
}
+ if (virJSONValueObjectAdd(&props,
+ "S:product", disk->product,
+ NULL) < 0)
+ return NULL;
+
break;
case VIR_DOMAIN_DISK_BUS_VIRTIO: {
@@ -1803,7 +1813,6 @@ qemuBuildDiskDeviceProps(const virDomainDef *def,
"A:wwn", &wwn,
"p:rotation_rate", disk->rotation_rate,
"S:vendor", disk->vendor,
- "S:product", disk->product,
"T:removable", removable,
"S:write-cache", qemuOnOffAuto(writeCache),
"p:cyls", disk->geometry.cylinders,
diff --git a/src/qemu/qemu_validate.c b/src/qemu/qemu_validate.c
index aaa056379e..f0be236533 100644
--- a/src/qemu/qemu_validate.c
+++ b/src/qemu/qemu_validate.c
@@ -2947,10 +2947,20 @@ qemuValidateDomainDeviceDefDiskFrontend(const virDomainDiskDef *disk,
}
}
- if (disk->vendor || disk->product) {
+ if (disk->vendor) {
if (disk->bus != VIR_DOMAIN_DISK_BUS_SCSI) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
- _("Only scsi disk supports vendor and product"));
+ _("Only scsi disk supports vendor"));
+ return -1;
+ }
+ }
+
+ if (disk->product) {
+ if ((disk->bus != VIR_DOMAIN_DISK_BUS_IDE) &&
+ (disk->bus != VIR_DOMAIN_DISK_BUS_SATA) &&
+ (disk->bus != VIR_DOMAIN_DISK_BUS_SCSI)) {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+ _("Only ide, sata and scsi disk supports product"));
return -1;
}
}
diff --git a/tests/qemuxmlconfdata/disk-scsi-disk-product-build-error.x86_64-latest.err b/tests/qemuxmlconfdata/disk-scsi-disk-product-build-error.x86_64-latest.err
new file mode 100644
index 0000000000..93dfac0d1e
--- /dev/null
+++ b/tests/qemuxmlconfdata/disk-scsi-disk-product-build-error.x86_64-latest.err
@@ -0,0 +1 @@
+unsupported configuration: Only ide, sata and scsi disk supports product
diff --git a/tests/qemuxmlconfdata/disk-scsi-disk-product-build-error.xml b/tests/qemuxmlconfdata/disk-scsi-disk-product-build-error.xml
new file mode 100644
index 0000000000..da2fc59da3
--- /dev/null
+++ b/tests/qemuxmlconfdata/disk-scsi-disk-product-build-error.xml
@@ -0,0 +1,34 @@
+<domain type='qemu'>
+ <name>QEMUGuest1</name>
+ <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid>
+ <memory unit='KiB'>219136</memory>
+ <currentMemory unit='KiB'>219136</currentMemory>
+ <vcpu placement='static'>1</vcpu>
+ <os>
+ <type arch='x86_64' machine='pc'>hvm</type>
+ <boot dev='hd'/>
+ </os>
+ <clock offset='utc'/>
+ <on_poweroff>destroy</on_poweroff>
+ <on_reboot>restart</on_reboot>
+ <on_crash>destroy</on_crash>
+ <devices>
+ <emulator>/usr/bin/qemu-system-x86_64</emulator>
+ <disk type='block' device='disk'>
+ <source dev='/dev/HostVG/QEMUGuest1'/>
+ <target dev='sda' bus='virtio'/>
+ <product>ST3146707LC</product>
+ </disk>
+ <disk type='block' device='disk'>
+ <source dev='/dev/HostVG/QEMUGuest2'/>
+ <target dev='sdb' bus='scsi'/>
+ <vendor>SEAGATE</vendor>
+ <product>ST3567807GD</product>
+ <address type='drive' controller='0' bus='0' target='0' unit='0'/>
+ </disk>
+ <controller type='usb' index='0'/>
+ <controller type='scsi' index='0' model='virtio-scsi'/>
+ <controller type='scsi' index='1' model='lsilogic'/>
+ <memballoon model='virtio'/>
+ </devices>
+</domain>
diff --git a/tests/qemuxmlconfdata/disk-scsi-disk-vendor-build-error.x86_64-latest.err b/tests/qemuxmlconfdata/disk-scsi-disk-vendor-build-error.x86_64-latest.err
new file mode 100644
index 0000000000..88bd9e5468
--- /dev/null
+++ b/tests/qemuxmlconfdata/disk-scsi-disk-vendor-build-error.x86_64-latest.err
@@ -0,0 +1 @@
+unsupported configuration: Only scsi disk supports vendor
diff --git a/tests/qemuxmlconfdata/disk-scsi-disk-vpd-build-error.xml b/tests/qemuxmlconfdata/disk-scsi-disk-vendor-build-error.xml
similarity index 100%
rename from tests/qemuxmlconfdata/disk-scsi-disk-vpd-build-error.xml
rename to tests/qemuxmlconfdata/disk-scsi-disk-vendor-build-error.xml
diff --git a/tests/qemuxmlconfdata/disk-scsi-disk-vpd-build-error.x86_64-latest.err b/tests/qemuxmlconfdata/disk-scsi-disk-vpd-build-error.x86_64-latest.err
deleted file mode 100644
index f70b7a774f..0000000000
--- a/tests/qemuxmlconfdata/disk-scsi-disk-vpd-build-error.x86_64-latest.err
+++ /dev/null
@@ -1 +0,0 @@
-unsupported configuration: Only scsi disk supports vendor and product
diff --git a/tests/qemuxmlconfdata/disk-scsi-disk-vpd.x86_64-latest.args b/tests/qemuxmlconfdata/disk-scsi-disk-vpd.x86_64-latest.args
index 4234a7e677..1d3aaf3819 100644
--- a/tests/qemuxmlconfdata/disk-scsi-disk-vpd.x86_64-latest.args
+++ b/tests/qemuxmlconfdata/disk-scsi-disk-vpd.x86_64-latest.args
@@ -30,9 +30,9 @@ XDG_CONFIG_HOME=/var/lib/libvirt/qemu/domain--1-QEMUGuest1/.config \
-device '{"driver":"virtio-scsi-pci","id":"scsi0","bus":"pci.0","addr":"0x2"}' \
-device '{"driver":"lsi","id":"scsi1","bus":"pci.0","addr":"0x3"}' \
-blockdev '{"driver":"host_device","filename":"/dev/HostVG/QEMUGuest1","node-name":"libvirt-2-storage","read-only":true}' \
--device '{"driver":"scsi-cd","bus":"scsi0.0","channel":0,"scsi-id":0,"lun":0,"device_id":"drive-scsi0-0-0-0","drive":"libvirt-2-storage","id":"scsi0-0-0-0","vendor":"SEAGATE","product":"ST3146707LC"}' \
+-device '{"model":"ST3146707LC","driver":"ide-cd","bus":"ide.0","unit":0,"drive":"libvirt-2-storage","id":"ide0-0-0"}' \
-blockdev '{"driver":"host_device","filename":"/dev/HostVG/QEMUGuest2","node-name":"libvirt-1-storage","read-only":true}' \
--device '{"driver":"scsi-hd","bus":"scsi1.0","scsi-id":0,"device_id":"drive-scsi1-0-0","drive":"libvirt-1-storage","id":"scsi1-0-0","bootindex":1,"vendor":"SEA GATE","product":"ST67 807GD"}' \
+-device '{"product":"ST67 807GD","driver":"scsi-hd","bus":"scsi1.0","scsi-id":0,"device_id":"drive-scsi1-0-0","drive":"libvirt-1-storage","id":"scsi1-0-0","bootindex":1}' \
-audiodev '{"id":"audio1","driver":"none"}' \
-device '{"driver":"virtio-balloon-pci","id":"balloon0","bus":"pci.0","addr":"0x4"}' \
-sandbox on,obsolete=deny,elevateprivileges=deny,spawn=deny,resourcecontrol=deny \
diff --git a/tests/qemuxmlconfdata/disk-scsi-disk-vpd.x86_64-latest.xml b/tests/qemuxmlconfdata/disk-scsi-disk-vpd.x86_64-latest.xml
index 4b23fbfcfe..39148f6ce7 100644
--- a/tests/qemuxmlconfdata/disk-scsi-disk-vpd.x86_64-latest.xml
+++ b/tests/qemuxmlconfdata/disk-scsi-disk-vpd.x86_64-latest.xml
@@ -20,9 +20,8 @@
<disk type='block' device='cdrom'>
<driver name='qemu' type='raw'/>
<source dev='/dev/HostVG/QEMUGuest1'/>
- <target dev='sda' bus='scsi'/>
+ <target dev='sda' bus='ide'/>
<readonly/>
- <vendor>SEAGATE</vendor>
<product>ST3146707LC</product>
<address type='drive' controller='0' bus='0' target='0' unit='0'/>
</disk>
@@ -31,7 +30,6 @@
<source dev='/dev/HostVG/QEMUGuest2'/>
<target dev='sdb' bus='scsi'/>
<readonly/>
- <vendor>SEA GATE</vendor>
<product>ST67 807GD</product>
<address type='drive' controller='1' bus='0' target='0' unit='0'/>
</disk>
@@ -45,6 +43,9 @@
<address type='pci' domain='0x0000' bus='0x00' slot='0x03' function='0x0'/>
</controller>
<controller type='pci' index='0' model='pci-root'/>
+ <controller type='ide' index='0'>
+ <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x1'/>
+ </controller>
<input type='mouse' bus='ps2'/>
<input type='keyboard' bus='ps2'/>
<audio id='1' type='none'/>
diff --git a/tests/qemuxmlconfdata/disk-scsi-disk-vpd.xml b/tests/qemuxmlconfdata/disk-scsi-disk-vpd.xml
index 36dd2a89ba..e3665d3afa 100644
--- a/tests/qemuxmlconfdata/disk-scsi-disk-vpd.xml
+++ b/tests/qemuxmlconfdata/disk-scsi-disk-vpd.xml
@@ -16,9 +16,8 @@
<emulator>/usr/bin/qemu-system-x86_64</emulator>
<disk type='block' device='cdrom'>
<source dev='/dev/HostVG/QEMUGuest1'/>
- <target dev='sda' bus='scsi'/>
+ <target dev='sda' bus='ide'/>
<readonly/>
- <vendor>SEAGATE</vendor>
<product>ST3146707LC</product>
<address type='drive' controller='0' bus='0' target='0' unit='0'/>
</disk>
@@ -26,7 +25,6 @@
<source dev='/dev/HostVG/QEMUGuest2'/>
<target dev='sdb' bus='scsi'/>
<readonly/>
- <vendor>SEA GATE</vendor>
<product>ST67 807GD</product>
<address type='drive' controller='1' bus='0' target='0' unit='0'/>
</disk>
diff --git a/tests/qemuxmlconftest.c b/tests/qemuxmlconftest.c
index 21b56dc94e..083b0ab7f6 100644
--- a/tests/qemuxmlconftest.c
+++ b/tests/qemuxmlconftest.c
@@ -1620,7 +1620,8 @@ mymain(void)
DO_TEST_CAPS_LATEST("disk-scsi-disk-split");
DO_TEST_CAPS_LATEST("disk-scsi-disk-wwn");
DO_TEST_CAPS_LATEST("disk-scsi-disk-vpd");
- DO_TEST_CAPS_LATEST_PARSE_ERROR("disk-scsi-disk-vpd-build-error");
+ DO_TEST_CAPS_LATEST_PARSE_ERROR("disk-scsi-disk-vendor-build-error");
+ DO_TEST_CAPS_LATEST_PARSE_ERROR("disk-scsi-disk-product-build-error");
DO_TEST_CAPS_LATEST("controller-virtio-scsi");
DO_TEST_CAPS_LATEST("controller-scsi-auto");
DO_TEST_CAPS_LATEST("disk-sata-device");
--
2.47.0
2 months, 4 weeks