October 2021 - Devel - libvirt List Archives

[PATCH] qemu_block: Refactor qemuBlockExportAddNBD()
by Kristina Hanicova 26 Oct '21

26 Oct '21

I think this patch improves readability of the function by removing unnecessary 'else' branches after 'return' and reworking the code a bit. Signed-off-by: Kristina Hanicova <khanicov(a)redhat.com> --- src/qemu/qemu_block.c | 33 +++++++++++++-------------------- 1 file changed, 13 insertions(+), 20 deletions(-) diff --git a/src/qemu/qemu_block.c b/src/qemu/qemu_block.c index 34fdec2c4b..4e532d29c0 100644 --- a/src/qemu/qemu_block.c +++ b/src/qemu/qemu_block.c @@ -3592,28 +3592,21 @@ qemuBlockExportAddNBD(virDomainObj *vm, const char *bitmap) { qemuDomainObjPrivate *priv = vm->privateData; + g_autoptr(virJSONValue) nbdprops = NULL; + const char *bitmaps[2] = { bitmap, NULL }; - if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_BLOCKDEV)) { - if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_BLOCK_EXPORT_ADD)) { - g_autoptr(virJSONValue) nbdprops = NULL; - const char *bitmaps[2] = { bitmap, NULL }; + /* older qemu versions didn't support configuring the exportname and + * took the 'drivealias' as the export name */ + if (!virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_BLOCKDEV)) + return qemuMonitorNBDServerAdd(priv->mon, drivealias, NULL, writable, NULL); - if (!(nbdprops = qemuBlockExportGetNBDProps(src->nodeformat, - exportname, - writable, - bitmaps))) - return -1; + if (!virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_BLOCK_EXPORT_ADD)) + return qemuMonitorNBDServerAdd(priv->mon, src->nodeformat, + exportname, writable, bitmap); - return qemuMonitorBlockExportAdd(priv->mon, &nbdprops); - } else { - return qemuMonitorNBDServerAdd(priv->mon, src->nodeformat, - exportname, writable, bitmap); - } - } else { - /* older qemu versions didn't support configuring the exportname and - * took the 'drivealias' as the export name */ - return qemuMonitorNBDServerAdd(priv->mon, drivealias, NULL, writable, NULL); - } + if ((nbdprops = qemuBlockExportGetNBDProps(src->nodeformat, exportname, + writable, bitmaps))) + return qemuMonitorBlockExportAdd(priv->mon, &nbdprops); - return 0; + return -1; } -- 2.31.1

2 1

[libvirt PATCH 0/1] Fix some typos
by Tim Wiederhake 26 Oct '21

26 Oct '21

Found by "codespell", see proposed script at https://listman.redhat.com/archives/libvir-list/2021-October/msg00015.html Cheers, Tim Tim Wiederhake (1): Fix some typos NEWS.rst | 2 +- src/conf/domain_conf.c | 2 +- src/qemu/qemu_command.c | 4 ++-- 3 files changed, 4 insertions(+), 4 deletions(-) -- 2.31.1

2 2

Re: [PATCH 0/5] trace: inroduce qmp: trace namespace
by Markus Armbruster 26 Oct '21

26 Oct '21

Vladimir Sementsov-Ogievskiy <vsementsov(a)virtuozzo.com> writes: > Hi all! > > We have handle_qmp_command and qmp_command_repond trace points to trace > qmp commands. They are very useful to debug problems involving > management tools like libvirt. > > But tracing all qmp commands is too much. > > Here I suggest a kind of tracing namespace. Formally this series adds a > trace points called qmp:<some-command> for every command, which may be > enabled in separate like > > --trace qmp:drive-backup > > or by pattern like > > --trace qmp:block-job-* > > or similarly with help of qmp command trace-event-set-state. > > This also allows to enable tracing of some qmp commands permanently > (by downstream patch or in libvirt xml). For example, I'm going to > enable tracing of block job comamnds and blockdev-* commands in > Virtuozzo. Qemu logs are often too empty (for example, in comparison > with Libvirt), logging block jobs is not too much but will be very > helpful. What exactly is traced? Peeking at PATCH 5... looks like it's input that makes it to qmp_dispatch() and command responses, but not events. Fine print on "input that makes it to qmp_dispatch()": * You trace right before we execute the command, not when we receive, parse and enqueue input. * Corollary: input with certain errors is not traced. * You don't trace the input text, you trace the unparsed parse tree. All fine, I presume. Existing tracepoints in monitor/qmp.c, and what information they send (inessential bits omitted for clarity): * handle_qmp_command Handling a QMP command: unparsed parse tree Fine print, safe to ignore: - Out-of-band commands will be executed right away, in-band commands will be queued. Tracepoints monitor_qmp_in_band_enqueue and monitor_qmp_in_band_dequeue provide insight into that. - This also receives and queues parse errors, without tracing them. Tracepoint monitor_qmp_err_in_band traces them as they are dequeued. * monitor_qmp_cmd_in_band About to execute in-band command: command ID, if any * monitor_qmp_cmd_out_of_band About to execute out-of-band command: command ID, if any * monitor_qmp_respond About to send command response or event: QObject For input, --trace qmp:* is like --trace handle_qmp_command, except it traces late rather than early. For output, --trace qmp:* is like --trace monitor_qmp_respond less events. The main improvement over existing tracepoints seems to be the ability to filter on command names. To get that, you overload the @name argument of QMP command trace-event-set-state. In addition to the documented meaning "Event name pattern", it also has an alternate, undocumented meaning "QMP command name pattern". The "undocumented" part is easy enough to fix. However, QMP heavily frowns on argument values that need to be parsed. But before we discuss this in depth, we should decide whether we want the filtering feature. Management applications can enable and disable tracing as needed, but doing it all in QEMU might be more convenient or robust. Libvirt logs all QMP traffic. I doubt it'll make use of your filtering feature. Cc'ing libvir-list just in case. Another way to log all traffic is to route it through socat -x or similar. Opinions?

3 3

[PATCH 0/2] domainMemoryDeviceSizeChange: Two simple improvements
by Michal Privoznik 26 Oct '21

26 Oct '21

*** BLURB HERE *** Michal Prívozník (2): qemu_monitor: Make domainMemoryDeviceSizeChange cb return void qemuProcessHandleMemoryDeviceSizeChange: Use qemuProcessEventSubmit() src/qemu/qemu_monitor.h | 10 +++++----- src/qemu/qemu_process.c | 14 ++------------ 2 files changed, 7 insertions(+), 17 deletions(-) -- 2.32.0

2 3

The exact meaning of DEVICE_REMOVED event
by Han Han 26 Oct '21

26 Oct '21

Hi, developers I has a question about the event VIR_DOMAIN_EVENT_ID_DEVICE_REMOVED. When will it occur? Detach device from an active VM? Or an inactive VM? Or both? I get a code comment from the callback of this event https://gitlab.com/libvirt/libvirt/-/blob/master/include/libvirt/libvirt-do… "This callback occurs when a device is removed from the domain" But it is not clear the domain could be active or inactive or both.

2 1

[libvirt PATCH] util: Drop pointless NUL_TERMINATE macro
by Jiri Denemark 26 Oct '21

26 Oct '21

It's only used once and open coding it is at least as clear as using the macro. Signed-off-by: Jiri Denemark <jdenemar(a)redhat.com> --- src/internal.h | 2 -- src/util/virutil.c | 2 +- 2 files changed, 1 insertion(+), 3 deletions(-) diff --git a/src/internal.h b/src/internal.h index e1250a59fe..d3809bf057 100644 --- a/src/internal.h +++ b/src/internal.h @@ -91,8 +91,6 @@ #define STREQ_NULLABLE(a, b) (g_strcmp0(a, b) == 0) #define STRNEQ_NULLABLE(a, b) (g_strcmp0(a, b) != 0) -#define NUL_TERMINATE(buf) do { (buf)[sizeof(buf)-1] = '\0'; } while (0) - #ifdef WIN32 # ifndef O_CLOEXEC # define O_CLOEXEC _O_NOINHERIT diff --git a/src/util/virutil.c b/src/util/virutil.c index c9de043c40..e04f1343d8 100644 --- a/src/util/virutil.c +++ b/src/util/virutil.c @@ -485,7 +485,7 @@ virGetHostnameImpl(bool quiet) "%s", _("failed to determine host name")); return NULL; } - NUL_TERMINATE(hostname); + hostname[sizeof(hostname) - 1] = '\0'; if (STRPREFIX(hostname, "localhost") || strchr(hostname, '.')) { /* in this case, gethostname returned localhost (meaning we can't -- 2.33.1

2 1

[libvirt PATCH] storage_file: Compute QCOW2 cluster size as ULL
by Jiri Denemark 26 Oct '21

26 Oct '21

While the QCOW2 cluster size is represented in only 4 bits in the QCOW2 header and thus 1 << cluster_size cannot overflow int, qcow2GetClusterSize is supposed to return unsigned long long so we can just compute the result as ULL rather than computing it as int and promoting to unsigned long long. Signed-off-by: Jiri Denemark <jdenemar(a)redhat.com> --- src/storage_file/storage_file_probe.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/storage_file/storage_file_probe.c b/src/storage_file/storage_file_probe.c index f5bb8b7dbb..dc438a5e5d 100644 --- a/src/storage_file/storage_file_probe.c +++ b/src/storage_file/storage_file_probe.c @@ -487,7 +487,7 @@ qcow2GetClusterSize(const char *buf, clusterBits = virReadBufInt32BE(buf + QCOWX_HDR_CLUSTER_BITS_OFFSET); if (clusterBits > 0) - return 1 << clusterBits; + return 1ULL << clusterBits; return 0; } -- 2.33.1

2 1

[libvirt PATCH] node_device: Fix memory leak in udevProcessMediatedDevice
by Jiri Denemark 26 Oct '21

26 Oct '21

One of the paths returned -1 directly without going through the cleanup section. Signed-off-by: Jiri Denemark <jdenemar(a)redhat.com> --- src/node_device/node_device_udev.c | 17 ++++++----------- 1 file changed, 6 insertions(+), 11 deletions(-) diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c index 7c3bb762b3..cd1722f934 100644 --- a/src/node_device/node_device_udev.c +++ b/src/node_device/node_device_udev.c @@ -1020,10 +1020,9 @@ static int udevProcessMediatedDevice(struct udev_device *dev, virNodeDeviceDef *def) { - int ret = -1; int iommugrp = -1; - char *linkpath = NULL; - char *canonicalpath = NULL; + g_autofree char *linkpath = NULL; + g_autofree char *canonicalpath = NULL; virNodeDevCapMdev *data = &def->caps->data.mdev; struct udev_device *parent_device = NULL; @@ -1039,19 +1038,19 @@ udevProcessMediatedDevice(struct udev_device *dev, virReportSystemError(errno, _("failed to wait for file '%s' to appear"), linkpath); - goto cleanup; + return -1; } if (virFileResolveLink(linkpath, &canonicalpath) < 0) { virReportSystemError(errno, _("failed to resolve '%s'"), linkpath); - goto cleanup; + return -1; } data->type = g_path_get_basename(canonicalpath); data->uuid = g_strdup(udev_device_get_sysname(dev)); if ((iommugrp = virMediatedDeviceGetIOMMUGroupNum(data->uuid)) < 0) - goto cleanup; + return -1; /* lookup the address of parent device */ parent_device = udev_device_get_parent(dev); @@ -1072,11 +1071,7 @@ udevProcessMediatedDevice(struct udev_device *dev, data->iommuGroupNumber = iommugrp; - ret = 0; - cleanup: - VIR_FREE(linkpath); - VIR_FREE(canonicalpath); - return ret; + return 0; } -- 2.33.1

2 1

[PATCH] cosmetic: remove unused function return value
by Ani Sinha 26 Oct '21

26 Oct '21

qemuBuildPMPCIRootHotplugCommandLine() returns 0 unconditionally. There is no failure scenario at present. So clean up the code by removing integer return from the function and also remove the failure check conditional from the function call. Also fix indentation for the above function call while at it. Signed-off-by: Ani Sinha <ani(a)anisinha.ca> --- src/qemu/qemu_command.c | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index d6df50ec73..093d8ae62c 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -3177,9 +3177,9 @@ qemuBuildSkipController(const virDomainControllerDef *controller, return false; } -static int +static void qemuBuildPMPCIRootHotplugCommandLine(virCommand *cmd, - const virDomainControllerDef *controller) + const virDomainControllerDef *controller) { if (controller->type == VIR_DOMAIN_CONTROLLER_TYPE_PCI && controller->model == VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT && @@ -3189,7 +3189,7 @@ qemuBuildPMPCIRootHotplugCommandLine(virCommand *cmd, virCommandAddArgFormat(cmd, "PIIX4_PM.acpi-root-pci-hotplug=%s", virTristateSwitchTypeToString(controller->opts.pciopts.hotplug)); } - return 0; + return; } static int @@ -3207,8 +3207,7 @@ qemuBuildControllersByTypeCommandLine(virCommand *cmd, if (cont->type != type) continue; - if (qemuBuildPMPCIRootHotplugCommandLine(cmd, cont)) - continue; + qemuBuildPMPCIRootHotplugCommandLine(cmd, cont); if (qemuBuildSkipController(cont, def)) continue; -- 2.25.1

2 1

[PATCH] i440fx/hotplug: Fix error message format to conform to spec
by Ani Sinha 26 Oct '21

26 Oct '21

Error messages must conform to spec as specified here: https://www.libvirt.org/coding-style.html#error-message-format This change makes some error messages conform to the spec above. Fixes: 8eadf82fb5 ("conf: introduce option to enable/disable pci hotplug on pci-root controller") Signed-off-by: Ani Sinha <ani(a)anisinha.ca> --- src/qemu/qemu_validate.c | 6 +++--- .../pc-i440fx-acpi-root-hotplug-disable.x86_64-5.1.0.err | 2 +- .../pc-i440fx-acpi-root-hotplug-enable.x86_64-5.1.0.err | 2 +- 3 files changed, 5 insertions(+), 5 deletions(-) diff --git a/src/qemu/qemu_validate.c b/src/qemu/qemu_validate.c index 3045e4b64b..f93b697265 100644 --- a/src/qemu/qemu_validate.c +++ b/src/qemu/qemu_validate.c @@ -3947,7 +3947,7 @@ qemuValidateDomainDeviceDefControllerPCI(const virDomainControllerDef *cont, case VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT: if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_PIIX4_ACPI_ROOT_PCI_HOTPLUG)) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("setting the %s property on a '%s' device is not supported by this QEMU binary"), + _("setting the '%s' property on a '%s' device is not supported by this QEMU binary"), "hotplug", "pci-root"); return -1; } @@ -3956,8 +3956,8 @@ qemuValidateDomainDeviceDefControllerPCI(const virDomainControllerDef *cont, case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_SWITCH_DOWNSTREAM_PORT: if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_PCIE_ROOT_PORT_HOTPLUG)) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("setting the hotplug property on a '%s' device is not supported by this QEMU binary"), - modelName); + _("setting the '%s' property on a '%s' device is not supported by this QEMU binary"), + "hotplug", modelName); return -1; } break; diff --git a/tests/qemuxml2argvdata/pc-i440fx-acpi-root-hotplug-disable.x86_64-5.1.0.err b/tests/qemuxml2argvdata/pc-i440fx-acpi-root-hotplug-disable.x86_64-5.1.0.err index b507f1f8bc..55ec41c476 100644 --- a/tests/qemuxml2argvdata/pc-i440fx-acpi-root-hotplug-disable.x86_64-5.1.0.err +++ b/tests/qemuxml2argvdata/pc-i440fx-acpi-root-hotplug-disable.x86_64-5.1.0.err @@ -1 +1 @@ -unsupported configuration: setting the hotplug property on a 'pci-root' device is not supported by this QEMU binary +unsupported configuration: setting the 'hotplug' property on a 'pci-root' device is not supported by this QEMU binary diff --git a/tests/qemuxml2argvdata/pc-i440fx-acpi-root-hotplug-enable.x86_64-5.1.0.err b/tests/qemuxml2argvdata/pc-i440fx-acpi-root-hotplug-enable.x86_64-5.1.0.err index b507f1f8bc..55ec41c476 100644 --- a/tests/qemuxml2argvdata/pc-i440fx-acpi-root-hotplug-enable.x86_64-5.1.0.err +++ b/tests/qemuxml2argvdata/pc-i440fx-acpi-root-hotplug-enable.x86_64-5.1.0.err @@ -1 +1 @@ -unsupported configuration: setting the hotplug property on a 'pci-root' device is not supported by this QEMU binary +unsupported configuration: setting the 'hotplug' property on a 'pci-root' device is not supported by this QEMU binary -- 2.25.1

2 1