[PATCH 0/6] Various fixes and cleanups
A collection of random one-off fixes for issues where the root cause was analyzed by the reporter and cleanups that I've recently accumulated. Peter Krempa (6): storage_file_probe: Use named initializer for 'struct FileTypeInfo' virQEMUCapsFindBinary: Refactor local variables virshPrintJobProgress: Don't rewrite migration status line on non-terminals storage: parthelper: Use if/else instead of ternary operator storage: disk: Properly handle partition numbers separated by 'p' tlscert: Don't force 'keyEncipherment' for ECDSA and ECDH src/qemu/qemu_capabilities.c | 9 +- src/rpc/virnettlscert.c | 33 +++-- src/storage/parthelper.c | 7 +- src/storage/storage_backend_disk.c | 9 +- src/storage_file/storage_file_probe.c | 174 ++++++++++++++++++-------- tools/virsh-domain.c | 13 +- 6 files changed, 165 insertions(+), 80 deletions(-) -- 2.49.0
From: Peter Krempa <pkrempa@redhat.com> Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/storage_file/storage_file_probe.c | 174 ++++++++++++++++++-------- 1 file changed, 119 insertions(+), 55 deletions(-) diff --git a/src/storage_file/storage_file_probe.c b/src/storage_file/storage_file_probe.c index 26f8d63e9a..82cea28b20 100644 --- a/src/storage_file/storage_file_probe.c +++ b/src/storage_file/storage_file_probe.c @@ -242,92 +242,156 @@ static struct FileEncryptionInfo const qcow2EncryptionInfo[] = { }; static struct FileTypeInfo const fileTypeInfo[] = { - [VIR_STORAGE_FILE_NONE] = { 0, NULL, LV_LITTLE_ENDIAN, - -1, 0, {0}, 0, 0, 0, NULL, NULL }, - [VIR_STORAGE_FILE_RAW] = { 0, NULL, LV_LITTLE_ENDIAN, - -1, 0, {0}, 0, 0, 0, - luksEncryptionInfo, - NULL }, - [VIR_STORAGE_FILE_DIR] = { 0, NULL, LV_LITTLE_ENDIAN, - -1, 0, {0}, 0, 0, 0, NULL, NULL }, + [VIR_STORAGE_FILE_NONE] = { + .endian = LV_LITTLE_ENDIAN, + .versionOffset = -1, + }, + [VIR_STORAGE_FILE_RAW] = { + .endian = LV_LITTLE_ENDIAN, + .versionOffset = -1, + .cryptInfo = luksEncryptionInfo, + }, + [VIR_STORAGE_FILE_DIR] = { + .endian = LV_LITTLE_ENDIAN, + .versionOffset = -1, + .cryptInfo = luksEncryptionInfo, + }, [VIR_STORAGE_FILE_BOCHS] = { /*"Bochs Virtual HD Image", */ /* Untested */ - 0, NULL, - LV_LITTLE_ENDIAN, 64, 4, {0x20000}, - 32+16+16+4+4+4+4+4, 8, 1, NULL, NULL + .endian = LV_LITTLE_ENDIAN, + .versionOffset = 64, + .versionSize = 4, + .versionNumbers = {0x20000}, + .sizeOffset = 32 + 16 + 16 + 4 + 4 + 4 + 4 + 4, + .sizeBytes = 8, + .sizeMultiplier = 1, }, + [VIR_STORAGE_FILE_CLOOP] = { /* #!/bin/sh #V2.0 Format modprobe cloop file=$0 && mount -r -t iso9660 /dev/cloop $1 */ /* Untested */ - 0, NULL, - LV_LITTLE_ENDIAN, -1, 0, {0}, - -1, 0, 0, NULL, NULL + .endian = LV_LITTLE_ENDIAN, + .versionOffset = -1, + .sizeOffset = -1, }, [VIR_STORAGE_FILE_DMG] = { /* XXX QEMU says there's no magic for dmg, * /usr/share/misc/magic lists double magic (both offsets * would have to match) but then disables that check. */ - 0, NULL, - 0, -1, 0, {0}, - -1, 0, 0, NULL, NULL + .endian = LV_LITTLE_ENDIAN, + .versionOffset = -1, + .sizeOffset = -1, }, [VIR_STORAGE_FILE_ISO] = { - 32769, "CD001", - LV_LITTLE_ENDIAN, -2, 0, {0}, - -1, 0, 0, NULL, NULL + .magicOffset = 32769, + .magic = "CD001", + .endian = LV_LITTLE_ENDIAN, + .versionOffset = -2, + .sizeOffset = -1, }, [VIR_STORAGE_FILE_VPC] = { - 0, "conectix", - LV_BIG_ENDIAN, 12, 4, {0x10000}, - 8 + 4 + 4 + 8 + 4 + 4 + 2 + 2 + 4, 8, 1, NULL, NULL + .magicOffset = 0, + .magic = "conectix", + .endian = LV_BIG_ENDIAN, + .versionOffset = 12, + .versionSize = 4, + .versionNumbers = {0x10000}, + .sizeOffset = 8 + 4 + 4 + 8 + 4 + 4 + 2 + 2 + 4, + .sizeBytes = 8, + .sizeMultiplier = 1, }, - /* TODO: add getBackingStore function */ [VIR_STORAGE_FILE_VDI] = { - 64, "\x7f\x10\xda\xbe", - LV_LITTLE_ENDIAN, 68, 4, {0x00010001}, - 64 + 5 * 4 + 256 + 7 * 4, 8, 1, NULL, NULL }, - + .magicOffset = 64, + .magic = "\x7f\x10\xda\xbe", + .endian = LV_LITTLE_ENDIAN, + .versionOffset = 68, + .versionSize = 4, + .versionNumbers = {0x00010001}, + .sizeOffset = 64 + 5 * 4 + 256 + 7 * 4, + .sizeBytes = 8, + .sizeMultiplier = 1, + }, /* Not direct file formats, but used for various drivers */ - [VIR_STORAGE_FILE_FAT] = { 0, NULL, LV_LITTLE_ENDIAN, - -1, 0, {0}, 0, 0, 0, NULL, NULL }, - [VIR_STORAGE_FILE_VHD] = { 0, NULL, LV_LITTLE_ENDIAN, - -1, 0, {0}, 0, 0, 0, NULL, NULL }, - [VIR_STORAGE_FILE_PLOOP] = { 0, "WithouFreSpacExt", LV_LITTLE_ENDIAN, - -2, 0, {0}, PLOOP_IMAGE_SIZE_OFFSET, 8, - PLOOP_SIZE_MULTIPLIER, NULL, NULL }, - + [VIR_STORAGE_FILE_FAT] = { + .endian = LV_LITTLE_ENDIAN, + .versionOffset = -1, + }, + [VIR_STORAGE_FILE_VHD] = { + .endian = LV_LITTLE_ENDIAN, + .versionOffset = -1, + }, + [VIR_STORAGE_FILE_PLOOP] = { + .magicOffset = 0, + .magic = "WithouFreSpacExt", + .endian = LV_LITTLE_ENDIAN, + .versionOffset = -2, + .sizeOffset = PLOOP_IMAGE_SIZE_OFFSET, + .sizeBytes = 8, + .sizeMultiplier = PLOOP_SIZE_MULTIPLIER, + }, /* All formats with a backing store probe below here */ [VIR_STORAGE_FILE_COW] = { - 0, "OOOM", - LV_BIG_ENDIAN, 4, 4, {2}, - 4+4+1024+4, 8, 1, NULL, cowGetImageSpecific + .magicOffset = 0, + .magic = "OOOM", + .endian = LV_BIG_ENDIAN, + .versionOffset = 4, + .versionSize = 4, + .versionNumbers = {2}, + .sizeOffset = 4 + 4 + 1024 + 4, + .sizeBytes = 8, + .sizeMultiplier = 1, + .getImageSpecific = cowGetImageSpecific, }, [VIR_STORAGE_FILE_QCOW] = { - 0, "QFI", - LV_BIG_ENDIAN, 4, 4, {1}, - QCOWX_HDR_IMAGE_SIZE, 8, 1, - qcow1EncryptionInfo, - qcowGetImageSpecific + .magicOffset = 0, + .magic = "QFI", + .endian = LV_BIG_ENDIAN, + .versionOffset = 4, + .versionSize = 4, + .versionNumbers = {1}, + .sizeOffset = QCOWX_HDR_IMAGE_SIZE, + .sizeBytes = 8, + .sizeMultiplier = 1, + .cryptInfo = qcow1EncryptionInfo, + .getImageSpecific = qcowGetImageSpecific, }, [VIR_STORAGE_FILE_QCOW2] = { - 0, "QFI", - LV_BIG_ENDIAN, 4, 4, {2, 3}, - QCOWX_HDR_IMAGE_SIZE, 8, 1, - qcow2EncryptionInfo, - qcow2GetImageSpecific + .magicOffset = 0, + .magic = "QFI", + .endian = LV_BIG_ENDIAN, + .versionOffset = 4, + .versionSize = 4, + .versionNumbers = {2, 3}, + .sizeOffset = QCOWX_HDR_IMAGE_SIZE, + .sizeBytes = 8, + .sizeMultiplier = 1, + .cryptInfo = qcow2EncryptionInfo, + .getImageSpecific = qcow2GetImageSpecific, }, [VIR_STORAGE_FILE_QED] = { /* https://wiki.qemu.org/Features/QED */ - 0, "QED", - LV_LITTLE_ENDIAN, -2, 0, {0}, - QED_HDR_IMAGE_SIZE, 8, 1, NULL, qedGetImageSpecific + .magicOffset = 0, + .magic = "QED", + .endian = LV_LITTLE_ENDIAN, + .versionOffset = -2, + .sizeOffset = QED_HDR_IMAGE_SIZE, + .sizeBytes = 8, + .sizeMultiplier = 1, + .getImageSpecific = qedGetImageSpecific, }, [VIR_STORAGE_FILE_VMDK] = { - 0, "KDMV", - LV_LITTLE_ENDIAN, 4, 4, {1, 2, 3}, - 4+4+4, 8, 512, NULL, vmdk4GetImageSpecific + .magicOffset = 0, + .magic = "KDMV", + .endian = LV_LITTLE_ENDIAN, + .versionOffset = 4, + .versionSize = 4, + .versionNumbers = {1, 2, 3}, + .sizeOffset = 4 + 4 + 4, + .sizeBytes = 8, + .sizeMultiplier = 512, + .getImageSpecific = vmdk4GetImageSpecific, }, }; G_STATIC_ASSERT(G_N_ELEMENTS(fileTypeInfo) == VIR_STORAGE_FILE_LAST); -- 2.49.0
From: Peter Krempa <pkrempa@redhat.com> Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_capabilities.c | 9 ++------- 1 file changed, 2 insertions(+), 7 deletions(-) diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index 4251ad2d92..2ba5462bb2 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -972,14 +972,9 @@ static char * virQEMUCapsFindBinary(const char *format, const char *archstr) { - char *ret = NULL; - char *binary = NULL; + g_autofree char *binary = g_strdup_printf(format, archstr); - binary = g_strdup_printf(format, archstr); - - ret = virFindFileInPath(binary); - VIR_FREE(binary); - return ret; + return virFindFileInPath(binary); } char * -- 2.49.0
From: Peter Krempa <pkrempa@redhat.com> On non-terminals print each progress report on a new line. Fix based on suggestion in the issue report. Closes: https://gitlab.com/libvirt/libvirt/-/issues/756 Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- tools/virsh-domain.c | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index 8d615b6e7a..7253596f4a 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -2091,10 +2091,13 @@ cmdBlkiotune(vshControl * ctl, const vshCmd * cmd) static void -virshPrintJobProgress(const char *label, unsigned long long remaining, +virshPrintJobProgress(const char *label, + unsigned long long remaining, unsigned long long total) { double progress = 100.00; + const char *term_start = "\r"; + const char *term_end = ""; /* if remaining == 0 migration has completed */ if (remaining != 0) { @@ -2106,10 +2109,16 @@ virshPrintJobProgress(const char *label, unsigned long long remaining, } } + if (!isatty(STDERR_FILENO)) { + term_start = ""; + term_end = "\n"; + } + /* see comments in vshError about why we must flush */ fflush(stdout); /* avoid auto-round-off of double by keeping only 2 decimals */ - fprintf(stderr, "\r%s: [%5.2f %%]", label, (int)(progress*100)/100.0); + fprintf(stderr, "%s%s: [%5.2f %%]%s", + term_start, label, (int)(progress*100)/100.0, term_end); fflush(stderr); } -- 2.49.0
From: Peter Krempa <pkrempa@redhat.com> Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/storage/parthelper.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/src/storage/parthelper.c b/src/storage/parthelper.c index 1169ebfb64..94aac34f7c 100644 --- a/src/storage/parthelper.c +++ b/src/storage/parthelper.c @@ -95,8 +95,11 @@ int main(int argc, char **argv) if (virFileResolveLink(path, &canonical_path) != 0) return 2; - partsep = *canonical_path && - g_ascii_isdigit(canonical_path[strlen(canonical_path)-1]) ? "p" : ""; + if (*canonical_path && + g_ascii_isdigit(canonical_path[strlen(canonical_path)-1])) + partsep = "p"; + else + partsep = ""; } if ((dev = ped_device_get(path)) == NULL) { -- 2.49.0
From: Peter Krempa <pkrempa@redhat.com> The 'p' separator for partitions is now common also for NVMe devices. Fix the algorithm to extract the partition number to always consider it. The fix is based on suggestion in the issue mentioned below. Closes: https://gitlab.com/libvirt/libvirt/-/issues/239 Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/storage/storage_backend_disk.c | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/src/storage/storage_backend_disk.c b/src/storage/storage_backend_disk.c index 996395de4a..871226ee22 100644 --- a/src/storage/storage_backend_disk.c +++ b/src/storage/storage_backend_disk.c @@ -805,7 +805,6 @@ virStorageBackendDiskDeleteVol(virStoragePoolObj *pool, virStoragePoolDef *def = virStoragePoolObjGetDef(pool); char *src_path = def->source.devices[0].path; g_autofree char *srcname = g_path_get_basename(src_path); - bool isDevMapperDevice; g_autofree char *devpath = NULL; g_autoptr(virCommand) cmd = NULL; @@ -822,8 +821,7 @@ virStorageBackendDiskDeleteVol(virStoragePoolObj *pool, * (parthelper.c) that is used to generate the target.path name * for use by libvirt. Changes to either, need to be reflected * in both places */ - isDevMapperDevice = virIsDevMapperDevice(vol->target.path); - if (isDevMapperDevice) { + if (virIsDevMapperDevice(vol->target.path)) { dev_name = g_path_get_basename(vol->target.path); } else { if (virFileResolveLink(vol->target.path, &devpath) < 0) { @@ -846,9 +844,8 @@ virStorageBackendDiskDeleteVol(virStoragePoolObj *pool, part_num = dev_name + strlen(srcname); - /* For device mapper and we have a partition character 'p' as the - * current character, let's move beyond that before checking part_num */ - if (isDevMapperDevice && *part_num == 'p') + /* Check if partition character 'p' is present and move beyond it */ + if (*part_num == 'p' && g_ascii_isdigit(*(part_num + 1))) part_num++; if (*part_num == 0) { -- 2.49.0
From: Peter Krempa <pkrempa@redhat.com> Per RFC8813 [1] which amends RFC5580 [2] ECDSA, ECDH, and ECMQV algorithms must not have 'keyEncipherment' present, but our code did check it. Add exemption for known algorithms which don't use it. [1] https://datatracker.ietf.org/doc/rfc8813/ [2] https://datatracker.ietf.org/doc/rfc5480 Closes: https://gitlab.com/libvirt/libvirt/-/issues/691 Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/rpc/virnettlscert.c | 33 +++++++++++++++++++++++++-------- 1 file changed, 25 insertions(+), 8 deletions(-) diff --git a/src/rpc/virnettlscert.c b/src/rpc/virnettlscert.c index 1befbe06bc..f197995633 100644 --- a/src/rpc/virnettlscert.c +++ b/src/rpc/virnettlscert.c @@ -163,14 +163,31 @@ static int virNetTLSCertCheckKeyUsage(gnutls_x509_crt_t cert, } } if (!(usage & GNUTLS_KEY_KEY_ENCIPHERMENT)) { - if (critical) { - virReportError(VIR_ERR_SYSTEM_ERROR, - _("Certificate %1$s usage does not permit key encipherment"), - certFile); - return -1; - } else { - VIR_WARN("Certificate %s usage does not permit key encipherment", - certFile); + int alg = gnutls_x509_crt_get_pk_algorithm(cert, NULL); + + /* Per RFC8813 [1] which amends RFC5580 [2] ECDSA, ECDH, and ECMQV + * algorithms must not have 'keyEncipherment' present. + * + * [1] https://datatracker.ietf.org/doc/rfc8813/ + * [2] https://datatracker.ietf.org/doc/rfc5480 + */ + + switch (alg) { + case GNUTLS_PK_ECDSA: + case GNUTLS_PK_ECDH_X25519: + case GNUTLS_PK_ECDH_X448: + break; + + default: + if (critical) { + virReportError(VIR_ERR_SYSTEM_ERROR, + _("Certificate %1$s usage does not permit key encipherment"), + certFile); + return -1; + } else { + VIR_WARN("Certificate %s usage does not permit key encipherment", + certFile); + } } } } -- 2.49.0
On Tue, Jun 17, 2025 at 03:43:59PM +0200, Peter Krempa via Devel wrote:
From: Peter Krempa <pkrempa@redhat.com>
Per RFC8813 [1] which amends RFC5580 [2] ECDSA, ECDH, and ECMQV algorithms must not have 'keyEncipherment' present, but our code did check it. Add exemption for known algorithms which don't use it.
[1] https://datatracker.ietf.org/doc/rfc8813/ [2] https://datatracker.ietf.org/doc/rfc5480
Closes: https://gitlab.com/libvirt/libvirt/-/issues/691 Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/rpc/virnettlscert.c | 33 +++++++++++++++++++++++++-------- 1 file changed, 25 insertions(+), 8 deletions(-)
Surprised we didn't need a test update, but it seems we don't test any EC certs, so that's why.
diff --git a/src/rpc/virnettlscert.c b/src/rpc/virnettlscert.c index 1befbe06bc..f197995633 100644 --- a/src/rpc/virnettlscert.c +++ b/src/rpc/virnettlscert.c @@ -163,14 +163,31 @@ static int virNetTLSCertCheckKeyUsage(gnutls_x509_crt_t cert, } } if (!(usage & GNUTLS_KEY_KEY_ENCIPHERMENT)) { - if (critical) { - virReportError(VIR_ERR_SYSTEM_ERROR, - _("Certificate %1$s usage does not permit key encipherment"), - certFile); - return -1; - } else { - VIR_WARN("Certificate %s usage does not permit key encipherment", - certFile); + int alg = gnutls_x509_crt_get_pk_algorithm(cert, NULL); + + /* Per RFC8813 [1] which amends RFC5580 [2] ECDSA, ECDH, and ECMQV + * algorithms must not have 'keyEncipherment' present. + * + * [1] https://datatracker.ietf.org/doc/rfc8813/ + * [2] https://datatracker.ietf.org/doc/rfc5480 + */ + + switch (alg) { + case GNUTLS_PK_ECDSA: + case GNUTLS_PK_ECDH_X25519: + case GNUTLS_PK_ECDH_X448: + break; + + default: + if (critical) { + virReportError(VIR_ERR_SYSTEM_ERROR, + _("Certificate %1$s usage does not permit key encipherment"), + certFile); + return -1; + } else { + VIR_WARN("Certificate %s usage does not permit key encipherment", + certFile); + } } } }
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> With regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On 6/17/25 15:43, Peter Krempa via Devel wrote:
A collection of random one-off fixes for issues where the root cause was analyzed by the reporter and cleanups that I've recently accumulated.
Peter Krempa (6): storage_file_probe: Use named initializer for 'struct FileTypeInfo' virQEMUCapsFindBinary: Refactor local variables virshPrintJobProgress: Don't rewrite migration status line on non-terminals storage: parthelper: Use if/else instead of ternary operator storage: disk: Properly handle partition numbers separated by 'p' tlscert: Don't force 'keyEncipherment' for ECDSA and ECDH
src/qemu/qemu_capabilities.c | 9 +- src/rpc/virnettlscert.c | 33 +++-- src/storage/parthelper.c | 7 +- src/storage/storage_backend_disk.c | 9 +- src/storage_file/storage_file_probe.c | 174 ++++++++++++++++++-------- tools/virsh-domain.c | 13 +- 6 files changed, 165 insertions(+), 80 deletions(-)
Reviewed-by: Michal Privoznik <mprivozn@redhat.com> Michal
On a Tuesday in 2025, Peter Krempa via Devel wrote:
A collection of random one-off fixes for issues where the root cause was analyzed by the reporter and cleanups that I've recently accumulated.
Peter Krempa (6): storage_file_probe: Use named initializer for 'struct FileTypeInfo' virQEMUCapsFindBinary: Refactor local variables virshPrintJobProgress: Don't rewrite migration status line on non-terminals storage: parthelper: Use if/else instead of ternary operator storage: disk: Properly handle partition numbers separated by 'p' tlscert: Don't force 'keyEncipherment' for ECDSA and ECDH
src/qemu/qemu_capabilities.c | 9 +- src/rpc/virnettlscert.c | 33 +++-- src/storage/parthelper.c | 7 +- src/storage/storage_backend_disk.c | 9 +- src/storage_file/storage_file_probe.c | 174 ++++++++++++++++++-------- tools/virsh-domain.c | 13 +- 6 files changed, 165 insertions(+), 80 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
participants (4)
-
Daniel P. Berrangé -
Ján Tomko -
Michal Prívozník -
Peter Krempa