5:20 a.m.
Whilst preparing for libvirt-php release, I want to ditch its imagick
dependency which is needed to convert PPM to PNG. But I've learned that
QEMU is able to return PNG already which looks more versatile than PPM
anyways.
Michal Prívozník (5):
qemu_caps: Introduce QEMU_CAPS_SCREENSHOT_FORMAT_PNG
qemu_monitor: Debug print all arguments in qemuMonitorScreendump()
qemu_monitor: Extend qemuMonitorScreendump() for @format
qemu: Prefer PNG for domain screenshots
NEWS: Document change of screenshot format for QEMU
NEWS.rst | 4 ++++
src/qemu/qemu_capabilities.c | 2 ++
src/qemu/qemu_capabilities.h | 1 +
src/qemu/qemu_driver.c | 13 +++++++++++--
src/qemu/qemu_monitor.c | 6 ++++--
src/qemu/qemu_monitor.h | 1 +
src/qemu/qemu_monitor_json.c | 2 ++
src/qemu/qemu_monitor_json.h | 1 +
tests/qemucapabilitiesdata/caps_7.1.0.ppc64.xml | 1 +
tests/qemucapabilitiesdata/caps_7.1.0.x86_64.xml | 1 +
tests/qemucapabilitiesdata/caps_7.2.0.x86_64.xml | 1 +
tests/qemumonitorjsontest.c | 2 +-
12 files changed, 30 insertions(+), 5 deletions(-)
--
2.37.4
5:20 a.m.
New subject: [PATCH 1/5] qemu_caps: Introduce QEMU_CAPS_SCREENSHOT_FORMAT_PNG
In its v7.1.0-rc0~125^2~6 commit, QEMU gained support for taking
screenshots in PNG format. Track this capability.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/qemu/qemu_capabilities.c | 2 ++
src/qemu/qemu_capabilities.h | 1 +
tests/qemucapabilitiesdata/caps_7.1.0.ppc64.xml | 1 +
tests/qemucapabilitiesdata/caps_7.1.0.x86_64.xml | 1 +
tests/qemucapabilitiesdata/caps_7.2.0.x86_64.xml | 1 +
5 files changed, 6 insertions(+)
diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
index 965af45cb2..2553b5b3ad 100644
--- a/src/qemu/qemu_capabilities.c
+++ b/src/qemu/qemu_capabilities.c
@@ -679,6 +679,7 @@ VIR_ENUM_IMPL(virQEMUCaps,
"query-stats-schemas", /* QEMU_CAPS_QUERY_STATS_SCHEMAS */
"sgx-epc", /* QEMU_CAPS_SGX_EPC */
"thread-context", /* QEMU_CAPS_THREAD_CONTEXT */
+ "screenshot-format-png", /* QEMU_CAPS_SCREENSHOT_FORMAT_PNG */
);
@@ -1562,6 +1563,7 @@ static struct virQEMUCapsStringFlags virQEMUCapsQMPSchemaQueries[] =
{
{ "query-display-options/ret-type/+dbus", QEMU_CAPS_DISPLAY_DBUS },
{ "object-add/arg-type/+iothread/thread-pool-max",
QEMU_CAPS_IOTHREAD_THREAD_POOL_MAX },
{ "query-migrate/ret-type/blocked-reasons",
QEMU_CAPS_MIGRATION_BLOCKED_REASONS },
+ { "screendump/arg-type/format/^png", QEMU_CAPS_SCREENSHOT_FORMAT_PNG },
};
typedef struct _virQEMUCapsObjectTypeProps virQEMUCapsObjectTypeProps;
diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h
index b70c02c05b..cc8b3759ea 100644
--- a/src/qemu/qemu_capabilities.h
+++ b/src/qemu/qemu_capabilities.h
@@ -658,6 +658,7 @@ typedef enum { /* virQEMUCapsFlags grouping marker for syntax-check
*/
QEMU_CAPS_QUERY_STATS_SCHEMAS, /* accepts query-stats-schemas */
QEMU_CAPS_SGX_EPC, /* -object sgx-epc,... */
QEMU_CAPS_THREAD_CONTEXT, /* -object thread-context */
+ QEMU_CAPS_SCREENSHOT_FORMAT_PNG, /* screendump command supports png format */
QEMU_CAPS_LAST /* this must always be the last item */
} virQEMUCapsFlags;
diff --git a/tests/qemucapabilitiesdata/caps_7.1.0.ppc64.xml
b/tests/qemucapabilitiesdata/caps_7.1.0.ppc64.xml
index 2cd47a7770..1f43612703 100644
--- a/tests/qemucapabilitiesdata/caps_7.1.0.ppc64.xml
+++ b/tests/qemucapabilitiesdata/caps_7.1.0.ppc64.xml
@@ -166,6 +166,7 @@
<flag name='migration.blocked-reasons'/>
<flag name='query-stats'/>
<flag name='query-stats-schemas'/>
+ <flag name='screenshot-format-png'/>
<version>7001000</version>
<kvmVersion>0</kvmVersion>
<microcodeVersion>42900244</microcodeVersion>
diff --git a/tests/qemucapabilitiesdata/caps_7.1.0.x86_64.xml
b/tests/qemucapabilitiesdata/caps_7.1.0.x86_64.xml
index e9210dfd44..8a2ed2236a 100644
--- a/tests/qemucapabilitiesdata/caps_7.1.0.x86_64.xml
+++ b/tests/qemucapabilitiesdata/caps_7.1.0.x86_64.xml
@@ -198,6 +198,7 @@
<flag name='migration.blocked-reasons'/>
<flag name='query-stats'/>
<flag name='query-stats-schemas'/>
+ <flag name='screenshot-format-png'/>
<version>7001000</version>
<kvmVersion>0</kvmVersion>
<microcodeVersion>43100244</microcodeVersion>
diff --git a/tests/qemucapabilitiesdata/caps_7.2.0.x86_64.xml
b/tests/qemucapabilitiesdata/caps_7.2.0.x86_64.xml
index 0fa042a339..6bc739065f 100644
--- a/tests/qemucapabilitiesdata/caps_7.2.0.x86_64.xml
+++ b/tests/qemucapabilitiesdata/caps_7.2.0.x86_64.xml
@@ -200,6 +200,7 @@
<flag name='query-stats'/>
<flag name='query-stats-schemas'/>
<flag name='thread-context'/>
+ <flag name='screenshot-format-png'/>
<version>7001091</version>
<kvmVersion>0</kvmVersion>
<microcodeVersion>43100245</microcodeVersion>
--
2.37.4
5:20 a.m.
New subject: [PATCH 2/5] qemu_monitor: Debug print all arguments in qemuMonitorScreendump()
For some reason, only @file argument is printed into debug logs.
The rest of arguments was left out. Include all arguments.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/qemu/qemu_monitor.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c
index 80f262cec7..e697ef2518 100644
--- a/src/qemu/qemu_monitor.c
+++ b/src/qemu/qemu_monitor.c
@@ -2855,7 +2855,8 @@ qemuMonitorScreendump(qemuMonitor *mon,
unsigned int head,
const char *file)
{
- VIR_DEBUG("file=%s", file);
+ VIR_DEBUG("device=%s head=%u file=%s",
+ device, head, file);
QEMU_CHECK_MONITOR(mon);
--
2.37.4
5:20 a.m.
New subject: [PATCH 3/5] qemu_monitor: Extend qemuMonitorScreendump() for @format
The 'screendump' command has new argument 'format'. Let's expose
this on our QMP level so that callers can specify the format, if
they wish so.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/qemu/qemu_driver.c | 2 +-
src/qemu/qemu_monitor.c | 7 ++++---
src/qemu/qemu_monitor.h | 1 +
src/qemu/qemu_monitor_json.c | 2 ++
src/qemu/qemu_monitor_json.h | 1 +
tests/qemumonitorjsontest.c | 2 +-
6 files changed, 10 insertions(+), 5 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 25a1f6e0fd..f6683cbb0a 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -3350,7 +3350,7 @@ qemuDomainScreenshot(virDomainPtr dom,
qemuSecurityDomainSetPathLabel(driver, vm, tmp, false);
qemuDomainObjEnterMonitor(vm);
- if (qemuMonitorScreendump(priv->mon, videoAlias, screen, tmp) < 0) {
+ if (qemuMonitorScreendump(priv->mon, videoAlias, screen, NULL, tmp) < 0) {
qemuDomainObjExitMonitor(vm);
goto endjob;
}
diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c
index e697ef2518..734364e070 100644
--- a/src/qemu/qemu_monitor.c
+++ b/src/qemu/qemu_monitor.c
@@ -2853,14 +2853,15 @@ int
qemuMonitorScreendump(qemuMonitor *mon,
const char *device,
unsigned int head,
+ const char *format,
const char *file)
{
- VIR_DEBUG("device=%s head=%u file=%s",
- device, head, file);
+ VIR_DEBUG("device=%s head=%u format=%s file=%s",
+ device, head, NULLSTR(format), file);
QEMU_CHECK_MONITOR(mon);
- return qemuMonitorJSONScreendump(mon, device, head, file);
+ return qemuMonitorJSONScreendump(mon, device, head, format, file);
}
diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h
index c690fc3655..906a919f52 100644
--- a/src/qemu/qemu_monitor.h
+++ b/src/qemu/qemu_monitor.h
@@ -987,6 +987,7 @@ int qemuMonitorInjectNMI(qemuMonitor *mon);
int qemuMonitorScreendump(qemuMonitor *mon,
const char *device,
unsigned int head,
+ const char *format,
const char *file);
int qemuMonitorSendKey(qemuMonitor *mon,
diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
index 39f313c2af..9822097bd7 100644
--- a/src/qemu/qemu_monitor_json.c
+++ b/src/qemu/qemu_monitor_json.c
@@ -4151,6 +4151,7 @@ int qemuMonitorJSONSendKey(qemuMonitor *mon,
int qemuMonitorJSONScreendump(qemuMonitor *mon,
const char *device,
unsigned int head,
+ const char *format,
const char *file)
{
g_autoptr(virJSONValue) cmd = NULL;
@@ -4160,6 +4161,7 @@ int qemuMonitorJSONScreendump(qemuMonitor *mon,
"s:filename", file,
"S:device", device,
"p:head", head,
+ "S:format", format,
NULL);
if (!cmd)
diff --git a/src/qemu/qemu_monitor_json.h b/src/qemu/qemu_monitor_json.h
index 93789480c5..484cb09830 100644
--- a/src/qemu/qemu_monitor_json.h
+++ b/src/qemu/qemu_monitor_json.h
@@ -337,6 +337,7 @@ int
qemuMonitorJSONScreendump(qemuMonitor *mon,
const char *device,
unsigned int head,
+ const char *format,
const char *file);
int
diff --git a/tests/qemumonitorjsontest.c b/tests/qemumonitorjsontest.c
index 238c6c1813..59f7322711 100644
--- a/tests/qemumonitorjsontest.c
+++ b/tests/qemumonitorjsontest.c
@@ -1212,7 +1212,7 @@ GEN_TEST_FUNC(qemuMonitorJSONDelDevice, "ide0")
GEN_TEST_FUNC(qemuMonitorJSONBlockdevMirror, "jobname", true, "vdb",
"targetnode", 1024, 1234, 31234, true, true)
GEN_TEST_FUNC(qemuMonitorJSONBlockStream, "vdb", "jobname",
"backingnode", "backingfilename", 1024)
GEN_TEST_FUNC(qemuMonitorJSONBlockCommit, "vdb", "jobname",
"topnode", "basenode", "backingfilename", 1024)
-GEN_TEST_FUNC(qemuMonitorJSONScreendump, "devicename", 1,
"/foo/bar")
+GEN_TEST_FUNC(qemuMonitorJSONScreendump, "devicename", 1, NULL,
"/foo/bar")
GEN_TEST_FUNC(qemuMonitorJSONOpenGraphics, "spice", "spicefd",
false)
GEN_TEST_FUNC(qemuMonitorJSONNBDServerAdd, "vda", "export", true,
"bitmap")
GEN_TEST_FUNC(qemuMonitorJSONDetachCharDev, "serial1")
--
2.37.4
5:20 a.m.
New subject: [PATCH 4/5] qemu: Prefer PNG for domain screenshots
Historically, QEMU took screenshots in PPM. While this might use
to be popular format, as of v7.1.0-rc0~125^2~6 it is possible to
take screenshots in PNG. This is more popular and renders almost
everywhere, which is not the case for PPM (for instance, modern
browsers do not render it).
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/qemu/qemu_driver.c | 13 +++++++++++--
1 file changed, 11 insertions(+), 2 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index f6683cbb0a..d509582719 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -3291,6 +3291,7 @@ qemuDomainScreenshot(virDomainPtr dom,
const char *videoAlias = NULL;
char *ret = NULL;
bool unlink_tmp = false;
+ const char *format = NULL;
virCheckFlags(0, NULL);
@@ -3339,6 +3340,10 @@ qemuDomainScreenshot(virDomainPtr dom,
}
}
+ if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_SCREENSHOT_FORMAT_PNG)) {
+ format = "png";
+ }
+
tmp = g_strdup_printf("%s/qemu.screendump.XXXXXX", priv->libDir);
if ((tmp_fd = g_mkstemp_full(tmp, O_RDWR | O_CLOEXEC, S_IRUSR | S_IWUSR)) == -1) {
@@ -3350,7 +3355,7 @@ qemuDomainScreenshot(virDomainPtr dom,
qemuSecurityDomainSetPathLabel(driver, vm, tmp, false);
qemuDomainObjEnterMonitor(vm);
- if (qemuMonitorScreendump(priv->mon, videoAlias, screen, NULL, tmp) < 0) {
+ if (qemuMonitorScreendump(priv->mon, videoAlias, screen, format, tmp) < 0) {
qemuDomainObjExitMonitor(vm);
goto endjob;
}
@@ -3367,7 +3372,11 @@ qemuDomainScreenshot(virDomainPtr dom,
goto endjob;
}
- ret = g_strdup("image/x-portable-pixmap");
+ if (STREQ_NULLABLE(format, "png")) {
+ ret = g_strdup("image/png");
+ } else {
+ ret = g_strdup("image/x-portable-pixmap");
+ }
endjob:
VIR_FORCE_CLOSE(tmp_fd);
--
2.37.4
5:48 a.m.
New subject: [PATCH 4/5] qemu: Prefer PNG for domain screenshots
On Wed, Dec 07, 2022 at 12:20:52PM +0100, Michal Privoznik wrote:
Historically, QEMU took screenshots in PPM. While this might use
to be popular format, as of v7.1.0-rc0~125^2~6 it is possible to
take screenshots in PNG. This is more popular and renders almost
everywhere, which is not the case for PPM (for instance, modern
browsers do not render it).
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/qemu/qemu_driver.c | 13 +++++++++++--
1 file changed, 11 insertions(+), 2 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index f6683cbb0a..d509582719 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -3291,6 +3291,7 @@ qemuDomainScreenshot(virDomainPtr dom,
const char *videoAlias = NULL;
char *ret = NULL;
bool unlink_tmp = false;
+ const char *format = NULL;
virCheckFlags(0, NULL);
@@ -3339,6 +3340,10 @@ qemuDomainScreenshot(virDomainPtr dom,
}
}
+ if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_SCREENSHOT_FORMAT_PNG)) {
+ format = "png";
+ }
+
Curly brackets are discouraged for one-line condition bodies IIRC ;)
5:20 a.m.
New subject: [PATCH 5/5] NEWS: Document change of screenshot format for QEMU
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
NEWS.rst | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/NEWS.rst b/NEWS.rst
index ffffbc61b9..39f508a6ce 100644
--- a/NEWS.rst
+++ b/NEWS.rst
@@ -19,6 +19,10 @@ v9.0.0 (unreleased)
* **Improvements**
+ * qemu: Prefer PNG for domain screenshots
+
+ With sufficiently new QEMU (v7.1.0) screenshots change format from PPM to PNG.
+
* **Bug fixes**
--
2.37.4
5:48 a.m.
On Wed, Dec 07, 2022 at 12:20:48PM +0100, Michal Privoznik wrote:
Whilst preparing for libvirt-php release, I want to ditch its imagick
dependency which is needed to convert PPM to PNG. But I've learned that
QEMU is able to return PNG already which looks more versatile than PPM
anyways.
Michal Prívozník (5):
qemu_caps: Introduce QEMU_CAPS_SCREENSHOT_FORMAT_PNG
qemu_monitor: Debug print all arguments in qemuMonitorScreendump()
qemu_monitor: Extend qemuMonitorScreendump() for @format
qemu: Prefer PNG for domain screenshots
NEWS: Document change of screenshot format for QEMU
Reviewed-by: Martin Kletzander <mkletzan(a)redhat.com>
NEWS.rst | 4 ++++
src/qemu/qemu_capabilities.c | 2 ++
src/qemu/qemu_capabilities.h | 1 +
src/qemu/qemu_driver.c | 13 +++++++++++--
src/qemu/qemu_monitor.c | 6 ++++--
src/qemu/qemu_monitor.h | 1 +
src/qemu/qemu_monitor_json.c | 2 ++
src/qemu/qemu_monitor_json.h | 1 +
tests/qemucapabilitiesdata/caps_7.1.0.ppc64.xml | 1 +
tests/qemucapabilitiesdata/caps_7.1.0.x86_64.xml | 1 +
tests/qemucapabilitiesdata/caps_7.2.0.x86_64.xml | 1 +
tests/qemumonitorjsontest.c | 2 +-
12 files changed, 30 insertions(+), 5 deletions(-)
--
2.37.4
5:50 a.m.
On a Wednesday in 2022, Michal Privoznik wrote:
Whilst preparing for libvirt-php release, I want to ditch its imagick
dependency which is needed to convert PPM to PNG. But I've learned that
QEMU is able to return PNG already which looks more versatile than PPM
anyways.
Michal Prívozník (5):
qemu_caps: Introduce QEMU_CAPS_SCREENSHOT_FORMAT_PNG
qemu_monitor: Debug print all arguments in qemuMonitorScreendump()
qemu_monitor: Extend qemuMonitorScreendump() for @format
qemu: Prefer PNG for domain screenshots
NEWS: Document change of screenshot format for QEMU
NEWS.rst | 4 ++++
src/qemu/qemu_capabilities.c | 2 ++
src/qemu/qemu_capabilities.h | 1 +
src/qemu/qemu_driver.c | 13 +++++++++++--
src/qemu/qemu_monitor.c | 6 ++++--
src/qemu/qemu_monitor.h | 1 +
src/qemu/qemu_monitor_json.c | 2 ++
src/qemu/qemu_monitor_json.h | 1 +
tests/qemucapabilitiesdata/caps_7.1.0.ppc64.xml | 1 +
tests/qemucapabilitiesdata/caps_7.1.0.x86_64.xml | 1 +
tests/qemucapabilitiesdata/caps_7.2.0.x86_64.xml | 1 +
tests/qemumonitorjsontest.c | 2 +-
12 files changed, 30 insertions(+), 5 deletions(-)
Reviewed-by: Ján Tomko <jtomko(a)redhat.com>
Jano
715
days inactive
715
days old
8 comments
3 participants
participants (3)
-
Ján Tomko -
Martin Kletzander -
Michal Privoznik