Re: [PATCH 0/5] trace: inroduce qmp: trace namespace
by Markus Armbruster
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 years
[PATCH 0/2] domainMemoryDeviceSizeChange: Two simple improvements
by Michal Privoznik
*** 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
3 years
[libvirt PATCH] util: Drop pointless NUL_TERMINATE macro
by Jiri Denemark
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
3 years
[libvirt PATCH] storage_file: Compute QCOW2 cluster size as ULL
by Jiri Denemark
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
3 years
[libvirt PATCH] node_device: Fix memory leak in udevProcessMediatedDevice
by Jiri Denemark
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
3 years
[PATCH] cosmetic: remove unused function return value
by Ani Sinha
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
3 years
[PATCH] i440fx/hotplug: Fix error message format to conform to spec
by Ani Sinha
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
3 years
[PATCH] docs: fix docs output path with meson 0.60.0
by Daniel P. Berrangé
The meson 0.60.0 release introduced a bug with the '/' operator when
using an empty path component. '/foo' / '' will now result in '/foo'
not '/foo/'
https://github.com/mesonbuild/meson/issues/9450
This breaks libvirt because xsltproc requires the trailing '/' on the
output directory path. Fortunately the explicit 'join_paths' function
is not affected by the regression
Signed-off-by: Daniel P. Berrangé <berrange(a)redhat.com>
---
Pushed as a CI / build fix
docs/meson.build | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/docs/meson.build b/docs/meson.build
index cbc138fa1f..fb6e0029d0 100644
--- a/docs/meson.build
+++ b/docs/meson.build
@@ -1,7 +1,9 @@
docs_html_dir = docdir / 'html'
# xsltproc requires that the -o path ends with '/'
-docs_builddir = meson.current_build_dir() / ''
+# Not using '/' operator due to bug in meson 0.60.0
+# https://github.com/mesonbuild/meson/issues/9450
+docs_builddir = join_paths(meson.current_build_dir(), '')
docs_assets = [
'android-chrome-192x192.png',
--
2.31.1
3 years
[PATCH] qemublocktest: Don't leak 'disk' in testQemuImageCreateLoadDiskXML
by Peter Krempa
The function returns only the source portion but forgot to free the disk
wrapper.
Fixes: 9696427ad67196
Signed-off-by: Peter Krempa <pkrempa(a)redhat.com>
---
Pushed as a trivial CI fix.
tests/qemublocktest.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/tests/qemublocktest.c b/tests/qemublocktest.c
index 0176fbd3f4..9d3c2fe6b0 100644
--- a/tests/qemublocktest.c
+++ b/tests/qemublocktest.c
@@ -473,7 +473,7 @@ testQemuImageCreateLoadDiskXML(const char *name,
virDomainXMLOption *xmlopt)
{
- virDomainDiskDef *disk = NULL;
+ g_autoptr(virDomainDiskDef) disk = NULL;
g_autofree char *xmlpath = NULL;
g_autofree char *xmlstr = NULL;
@@ -490,7 +490,7 @@ testQemuImageCreateLoadDiskXML(const char *name,
if (qemuDomainDeviceDiskDefPostParse(disk, 0) < 0)
return NULL;
- return disk->src;
+ return g_steal_pointer(&disk->src);
}
--
2.31.1
3 years, 1 month
