Re: [Qemu-devel] [PATCH 2/4] virtio-gpu: remove unused config_size
Reviewed-by: Christophe Fergeau
On Thu, Feb 21, 2019 at 12:43:28PM +0100, Marc-André Lureau wrote:
> Signed-off-by: Marc-André Lureau
> ---
> include/hw/virtio/virtio-gpu.h | 2 --
> hw/display/virtio-gpu.c| 3 +--
> 2 files changed, 1 insertion(+), 4 deletions(-)
>
> diff --git a/include/hw/virtio/virtio-gpu.h b/include/hw/virtio/virtio-gpu.h
> index c7cb821ae3..a1cecd1df8 100644
> --- a/include/hw/virtio/virtio-gpu.h
> +++ b/include/hw/virtio/virtio-gpu.h
> @@ -96,8 +96,6 @@ typedef struct VirtIOGPU {
>
> int enable;
>
> -int config_size;
> -
> QTAILQ_HEAD(, virtio_gpu_simple_resource) reslist;
> QTAILQ_HEAD(, virtio_gpu_ctrl_command) cmdq;
> QTAILQ_HEAD(, virtio_gpu_ctrl_command) fenceq;
> diff --git a/hw/display/virtio-gpu.c b/hw/display/virtio-gpu.c
> index a52c2aed0e..8f4351420b 100644
> --- a/hw/display/virtio-gpu.c
> +++ b/hw/display/virtio-gpu.c
> @@ -1238,10 +1238,9 @@ static void virtio_gpu_device_realize(DeviceState
> *qdev, Error **errp)
> }
> }
>
> -g->config_size = sizeof(struct virtio_gpu_config);
> g->virtio_config.num_scanouts = cpu_to_le32(g->conf.max_outputs);
> virtio_init(VIRTIO_DEVICE(g), "virtio-gpu", VIRTIO_ID_GPU,
> -g->config_size);
> +sizeof(struct virtio_gpu_config));
>
> g->req_state[0].width = g->conf.xres;
> g->req_state[0].height = g->conf.yres;
> --
> 2.21.0.rc1
>
>
signature.asc
Description: PGP signature
Re: [Qemu-devel] [PATCH 3/4] virtio-gpu: block both 2d and 3d rendering
I came up with the same fix while looking at that bug before seeing
Marc-André's patch.
Then I tested Marc-André's patch.
So for this patch,
Reviewed-by: Christophe Fergeau
Tested-by: Christophe Fergeau
On Thu, Feb 21, 2019 at 12:43:29PM +0100, Marc-André Lureau wrote:
> Now that 2d commands are translated to 3d rendering, qemu must stop
> sending 3d updates (from 2d) to Spice as well.
>
> Fixes:
> https://bugzilla.redhat.com/show_bug.cgi?id=1674324
>
> Cc: cferg...@redhat.com
> Signed-off-by: Marc-André Lureau
> ---
> include/hw/virtio/virtio-gpu.h | 1 -
> hw/display/virtio-gpu-3d.c | 21 -
> hw/display/virtio-gpu.c| 27 ++-
> 3 files changed, 22 insertions(+), 27 deletions(-)
>
> diff --git a/include/hw/virtio/virtio-gpu.h b/include/hw/virtio/virtio-gpu.h
> index a1cecd1df8..f8cd8ee96f 100644
> --- a/include/hw/virtio/virtio-gpu.h
> +++ b/include/hw/virtio/virtio-gpu.h
> @@ -169,7 +169,6 @@ void virtio_gpu_virgl_process_cmd(VirtIOGPU *g,
>struct virtio_gpu_ctrl_command *cmd);
> void virtio_gpu_virgl_fence_poll(VirtIOGPU *g);
> void virtio_gpu_virgl_reset(VirtIOGPU *g);
> -void virtio_gpu_gl_block(void *opaque, bool block);
> int virtio_gpu_virgl_init(VirtIOGPU *g);
> int virtio_gpu_virgl_get_num_capsets(VirtIOGPU *g);
> #endif
> diff --git a/hw/display/virtio-gpu-3d.c b/hw/display/virtio-gpu-3d.c
> index bc6e99c943..cb83479ed2 100644
> --- a/hw/display/virtio-gpu-3d.c
> +++ b/hw/display/virtio-gpu-3d.c
> @@ -404,11 +404,6 @@ void virtio_gpu_virgl_process_cmd(VirtIOGPU *g,
> {
> VIRTIO_GPU_FILL_CMD(cmd->cmd_hdr);
>
> -cmd->waiting = g->renderer_blocked;
> -if (cmd->waiting) {
> -return;
> -}
> -
> virgl_renderer_force_ctx_0();
> switch (cmd->cmd_hdr.type) {
> case VIRTIO_GPU_CMD_CTX_CREATE:
> @@ -604,22 +599,6 @@ void virtio_gpu_virgl_reset(VirtIOGPU *g)
> }
> }
>
> -void virtio_gpu_gl_block(void *opaque, bool block)
> -{
> -VirtIOGPU *g = opaque;
> -
> -if (block) {
> -g->renderer_blocked++;
> -} else {
> -g->renderer_blocked--;
> -}
> -assert(g->renderer_blocked >= 0);
> -
> -if (g->renderer_blocked == 0) {
> -virtio_gpu_process_cmdq(g);
> -}
> -}
> -
> int virtio_gpu_virgl_init(VirtIOGPU *g)
> {
> int ret;
> diff --git a/hw/display/virtio-gpu.c b/hw/display/virtio-gpu.c
> index 8f4351420b..7ada4b83ac 100644
> --- a/hw/display/virtio-gpu.c
> +++ b/hw/display/virtio-gpu.c
> @@ -889,12 +889,15 @@ void virtio_gpu_process_cmdq(VirtIOGPU *g)
> while (!QTAILQ_EMPTY(>cmdq)) {
> cmd = QTAILQ_FIRST(>cmdq);
>
> -/* process command */
> -VIRGL(g, virtio_gpu_virgl_process_cmd, virtio_gpu_simple_process_cmd,
> - g, cmd);
> +cmd->waiting = g->renderer_blocked;
> if (cmd->waiting) {
> break;
> }
> +
> +/* process command */
> +VIRGL(g, virtio_gpu_virgl_process_cmd, virtio_gpu_simple_process_cmd,
> + g, cmd);
> +
> QTAILQ_REMOVE(>cmdq, cmd, next);
> if (virtio_gpu_stats_enabled(g->conf)) {
> g->stats.requests++;
> @@ -1030,14 +1033,28 @@ static int virtio_gpu_ui_info(void *opaque, uint32_t
> idx, QemuUIInfo *info)
> return 0;
> }
>
> +static void virtio_gpu_gl_block(void *opaque, bool block)
> +{
> +VirtIOGPU *g = opaque;
> +
> +if (block) {
> +g->renderer_blocked++;
> +} else {
> +g->renderer_blocked--;
> +}
> +assert(g->renderer_blocked >= 0);
> +
> +if (g->renderer_blocked == 0) {
> +virtio_gpu_process_cmdq(g);
> +}
> +}
> +
> const GraphicHwOps virtio_gpu_ops = {
> .invalidate = virtio_gpu_invalidate_display,
> .gfx_update = virtio_gpu_update_display,
> .text_update = virtio_gpu_text_update,
> .ui_info = virtio_gpu_ui_info,
> -#ifdef CONFIG_VIRGL
> .gl_block = virtio_gpu_gl_block,
> -#endif
> };
>
> static const VMStateDescription vmstate_virtio_gpu_scanout = {
> --
> 2.21.0.rc1
>
>
signature.asc
Description: PGP signature
Re: [Qemu-devel] [PATCH 1/4] virtio-gpu: remove unused qdev
Reviewed-by: Christophe Fergeau
On Thu, Feb 21, 2019 at 12:43:27PM +0100, Marc-André Lureau wrote:
> Signed-off-by: Marc-André Lureau
> ---
> include/hw/virtio/virtio-gpu.h | 1 -
> hw/display/virtio-gpu.c| 1 -
> 2 files changed, 2 deletions(-)
>
> diff --git a/include/hw/virtio/virtio-gpu.h b/include/hw/virtio/virtio-gpu.h
> index c8c599f1b9..c7cb821ae3 100644
> --- a/include/hw/virtio/virtio-gpu.h
> +++ b/include/hw/virtio/virtio-gpu.h
> @@ -97,7 +97,6 @@ typedef struct VirtIOGPU {
> int enable;
>
> int config_size;
> -DeviceState *qdev;
>
> QTAILQ_HEAD(, virtio_gpu_simple_resource) reslist;
> QTAILQ_HEAD(, virtio_gpu_ctrl_command) cmdq;
> diff --git a/hw/display/virtio-gpu.c b/hw/display/virtio-gpu.c
> index c6fab56f9b..a52c2aed0e 100644
> --- a/hw/display/virtio-gpu.c
> +++ b/hw/display/virtio-gpu.c
> @@ -1268,7 +1268,6 @@ static void virtio_gpu_device_realize(DeviceState
> *qdev, Error **errp)
> QTAILQ_INIT(>fenceq);
>
> g->enabled_output_bitmask = 1;
> -g->qdev = qdev;
>
> for (i = 0; i < g->conf.max_outputs; i++) {
> g->scanout[i].con =
> --
> 2.21.0.rc1
>
>
signature.asc
Description: PGP signature
Re: [Qemu-devel] [PATCH 4/4] virtio-gpu: remove useless 'waiting' field
Reviewed-by: Christophe Fergeau
On Thu, Feb 21, 2019 at 12:43:30PM +0100, Marc-André Lureau wrote:
> Let's check renderer_blocked instead directly.
>
> Signed-off-by: Marc-André Lureau
> ---
> include/hw/virtio/virtio-gpu.h | 1 -
> hw/display/virtio-gpu.c| 4 +---
> 2 files changed, 1 insertion(+), 4 deletions(-)
>
> diff --git a/include/hw/virtio/virtio-gpu.h b/include/hw/virtio/virtio-gpu.h
> index f8cd8ee96f..26a6698266 100644
> --- a/include/hw/virtio/virtio-gpu.h
> +++ b/include/hw/virtio/virtio-gpu.h
> @@ -81,7 +81,6 @@ struct virtio_gpu_ctrl_command {
> VirtQueue *vq;
> struct virtio_gpu_ctrl_hdr cmd_hdr;
> uint32_t error;
> -bool waiting;
> bool finished;
> QTAILQ_ENTRY(virtio_gpu_ctrl_command) next;
> };
> diff --git a/hw/display/virtio-gpu.c b/hw/display/virtio-gpu.c
> index 7ada4b83ac..0baa9ac0ad 100644
> --- a/hw/display/virtio-gpu.c
> +++ b/hw/display/virtio-gpu.c
> @@ -889,8 +889,7 @@ void virtio_gpu_process_cmdq(VirtIOGPU *g)
> while (!QTAILQ_EMPTY(>cmdq)) {
> cmd = QTAILQ_FIRST(>cmdq);
>
> -cmd->waiting = g->renderer_blocked;
> -if (cmd->waiting) {
> +if (g->renderer_blocked) {
> break;
> }
>
> @@ -939,7 +938,6 @@ static void virtio_gpu_handle_ctrl(VirtIODevice *vdev,
> VirtQueue *vq)
> cmd->vq = vq;
> cmd->error = 0;
> cmd->finished = false;
> -cmd->waiting = false;
> QTAILQ_INSERT_TAIL(>cmdq, cmd, next);
> cmd = virtqueue_pop(vq, sizeof(struct virtio_gpu_ctrl_command));
> }
> --
> 2.21.0.rc1
>
>
signature.asc
Description: PGP signature
[Qemu-devel] [PATCH v7 0/2] log: Make glib logging go through QEMU
The main goal of this patch series is to make logs output by libspice-server.so go through qemu logging infrastructure so that their format is the same as the rest of QEMU messages (in particular, timestamps). This is done in the second patch, the first one is a preparation patch. Christophe Fergeau (2): qemu-io: Use error_[gs]et_progname() log: Make glib logging go through QEMU bsd-user/main.c | 2 ++ include/qemu/error-report.h | 3 +- linux-user/main.c | 2 ++ qemu-img.c | 2 +- qemu-io.c | 14 -- qemu-nbd.c | 2 +- scsi/qemu-pr-helper.c | 1 + util/qemu-error.c | 55 - vl.c| 2 +- 9 files changed, 70 insertions(+), 13 deletions(-) -- 2.20.1
[Qemu-devel] [PATCH v7 2/2] log: Make glib logging go through QEMU
This commit adds a error_init() helper which calls
g_log_set_default_handler() so that glib logs (g_log, g_warning, ...)
are handled similarly to other QEMU logs. This means they will get a
timestamp if timestamps are enabled, and they will go through the
HMP monitor if one is configured.
This commit also adds a call to error_init() to the binaries
installed by QEMU. Since error_init() also calls error_set_progname(),
this means that *-linux-user, *-bsd-user and qemu-pr-helper messages
output with error_report, info_report, ... will slightly change: they
will be prefixed by the binary name.
glib debug messages are enabled through G_MESSAGES_DEBUG similarly to
the glib default log handler.
At the moment, this change will mostly impact SPICE logging if your
spice version is >= 0.14.1. With older spice versions, this is not going
to work as expected, but will not have any ill effect, so this call is
not conditional on the SPICE version.
Signed-off-by: Christophe Fergeau
Reviewed-by: Stefan Hajnoczi
---
Changes since v6:
- expanded on the bsd-user/linux-user/qemu-pr-helper changes in the
commit log
- fix obsolete mention of qemu_init_logging() in commit log
- improved output when 'log_domain' is NULL
- removed unneeded "fall through" comment in switch/case
- fixed multi-line comments
bsd-user/main.c | 2 ++
include/qemu/error-report.h | 3 +-
linux-user/main.c | 2 ++
qemu-img.c | 2 +-
qemu-io.c | 2 +-
qemu-nbd.c | 2 +-
scsi/qemu-pr-helper.c | 1 +
util/qemu-error.c | 55 -
vl.c| 2 +-
9 files changed, 65 insertions(+), 6 deletions(-)
diff --git a/bsd-user/main.c b/bsd-user/main.c
index 0d3156974c..8fd8ae4127 100644
--- a/bsd-user/main.c
+++ b/bsd-user/main.c
@@ -24,6 +24,7 @@
#include "qapi/error.h"
#include "qemu.h"
#include "qemu/config-file.h"
+#include "qemu/error-report.h"
#include "qemu/path.h"
#include "qemu/help_option.h"
#include "cpu.h"
@@ -743,6 +744,7 @@ int main(int argc, char **argv)
if (argc <= 1)
usage();
+error_init(argv[0]);
module_call_init(MODULE_INIT_TRACE);
qemu_init_cpu_list();
module_call_init(MODULE_INIT_QOM);
diff --git a/include/qemu/error-report.h b/include/qemu/error-report.h
index 0a8d9cc9ea..ce43c02314 100644
--- a/include/qemu/error-report.h
+++ b/include/qemu/error-report.h
@@ -34,7 +34,6 @@ void error_vprintf(const char *fmt, va_list ap)
GCC_FMT_ATTR(1, 0);
void error_printf(const char *fmt, ...) GCC_FMT_ATTR(1, 2);
void error_vprintf_unless_qmp(const char *fmt, va_list ap) GCC_FMT_ATTR(1, 0);
void error_printf_unless_qmp(const char *fmt, ...) GCC_FMT_ATTR(1, 2);
-void error_set_progname(const char *argv0);
void error_vreport(const char *fmt, va_list ap) GCC_FMT_ATTR(1, 0);
void warn_vreport(const char *fmt, va_list ap) GCC_FMT_ATTR(1, 0);
@@ -49,6 +48,8 @@ bool error_report_once_cond(bool *printed, const char *fmt,
...)
bool warn_report_once_cond(bool *printed, const char *fmt, ...)
GCC_FMT_ATTR(2, 3);
+void error_init(const char *argv0);
+
/*
* Similar to error_report(), except it prints the message just once.
* Return true when it prints, false otherwise.
diff --git a/linux-user/main.c b/linux-user/main.c
index a0aba9cb1e..f9efe9ff6e 100644
--- a/linux-user/main.c
+++ b/linux-user/main.c
@@ -27,6 +27,7 @@
#include "qemu/path.h"
#include "qemu/config-file.h"
#include "qemu/cutils.h"
+#include "qemu/error-report.h"
#include "qemu/help_option.h"
#include "cpu.h"
#include "exec/exec-all.h"
@@ -600,6 +601,7 @@ int main(int argc, char **argv, char **envp)
int ret;
int execfd;
+error_init(argv[0]);
module_call_init(MODULE_INIT_TRACE);
qemu_init_cpu_list();
module_call_init(MODULE_INIT_QOM);
diff --git a/qemu-img.c b/qemu-img.c
index ad04f59565..0af9cac244 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -4912,8 +4912,8 @@ int main(int argc, char **argv)
signal(SIGPIPE, SIG_IGN);
#endif
+error_init(argv[0]);
module_call_init(MODULE_INIT_TRACE);
-error_set_progname(argv[0]);
qemu_init_exec_dir(argv[0]);
if (qemu_init_main_loop(_error)) {
diff --git a/qemu-io.c b/qemu-io.c
index 2c52ac0ade..8d5d5911cb 100644
--- a/qemu-io.c
+++ b/qemu-io.c
@@ -522,8 +522,8 @@ int main(int argc, char **argv)
signal(SIGPIPE, SIG_IGN);
#endif
+error_init(argv[0]);
module_call_init(MODULE_INIT_TRACE);
-error_set_progname(argv[0]);
qemu_init_exec_dir(argv[0]);
qcrypto_init(_fatal);
diff --git a/qemu-nbd.c b/qemu-nbd.c
index 1f7b2a03f5..39849f1692 100644
--- a/qemu-nbd.c
+++ b/qemu-nbd.c
@@ -685,8 +685,8 @@ int main(int argc, char **argv)
signal(SIGPIPE, SIG_IGN);
#endif
+error_init(argv[0]);
module_ca
[Qemu-devel] [PATCH v7 1/2] qemu-io: Use error_[gs]et_progname()
qemu-io reimplements itself what
error_get_progname()/error_set_progname() already does.
This commit switches it to use this API from qemu-error.h
Signed-off-by: Christophe Fergeau
Reviewed-by: Eric Blake
Reviewed-by: Stefan Hajnoczi
---
No changes since v6
qemu-io.c | 14 ++
1 file changed, 6 insertions(+), 8 deletions(-)
diff --git a/qemu-io.c b/qemu-io.c
index 6df7731af4..2c52ac0ade 100644
--- a/qemu-io.c
+++ b/qemu-io.c
@@ -34,8 +34,6 @@
#define CMD_NOFILE_OK 0x01
-static char *progname;
-
static BlockBackend *qemuio_blk;
static bool quit_qemu_io;
@@ -312,7 +310,7 @@ static char *get_prompt(void)
static char prompt[FILENAME_MAX + 2 /*"> "*/ + 1 /*"\0"*/ ];
if (!prompt[0]) {
-snprintf(prompt, sizeof(prompt), "%s> ", progname);
+snprintf(prompt, sizeof(prompt), "%s> ", error_get_progname());
}
return prompt;
@@ -525,7 +523,7 @@ int main(int argc, char **argv)
#endif
module_call_init(MODULE_INIT_TRACE);
-progname = g_path_get_basename(argv[0]);
+error_set_progname(argv[0]);
qemu_init_exec_dir(argv[0]);
qcrypto_init(_fatal);
@@ -580,10 +578,10 @@ int main(int argc, char **argv)
break;
case 'V':
printf("%s version " QEMU_FULL_VERSION "\n"
- QEMU_COPYRIGHT "\n", progname);
+ QEMU_COPYRIGHT "\n", error_get_progname());
exit(0);
case 'h':
-usage(progname);
+usage(error_get_progname());
exit(0);
case 'U':
force_share = true;
@@ -600,13 +598,13 @@ int main(int argc, char **argv)
imageOpts = true;
break;
default:
-usage(progname);
+usage(error_get_progname());
exit(1);
}
}
if ((argc - optind) > 1) {
-usage(progname);
+usage(error_get_progname());
exit(1);
}
--
2.20.1
[Qemu-devel] [PATCH v6 1/2] qemu-io: Use error_[gs]et_progname()
qemu-io reimplements itself what
error_get_progname()/error_set_progname() already does.
This commit switches it to use this API from qemu-error.h
Signed-off-by: Christophe Fergeau
---
qemu-io.c | 14 ++
1 file changed, 6 insertions(+), 8 deletions(-)
diff --git a/qemu-io.c b/qemu-io.c
index 6df7731af4..2c52ac0ade 100644
--- a/qemu-io.c
+++ b/qemu-io.c
@@ -34,8 +34,6 @@
#define CMD_NOFILE_OK 0x01
-static char *progname;
-
static BlockBackend *qemuio_blk;
static bool quit_qemu_io;
@@ -312,7 +310,7 @@ static char *get_prompt(void)
static char prompt[FILENAME_MAX + 2 /*"> "*/ + 1 /*"\0"*/ ];
if (!prompt[0]) {
-snprintf(prompt, sizeof(prompt), "%s> ", progname);
+snprintf(prompt, sizeof(prompt), "%s> ", error_get_progname());
}
return prompt;
@@ -525,7 +523,7 @@ int main(int argc, char **argv)
#endif
module_call_init(MODULE_INIT_TRACE);
-progname = g_path_get_basename(argv[0]);
+error_set_progname(argv[0]);
qemu_init_exec_dir(argv[0]);
qcrypto_init(_fatal);
@@ -580,10 +578,10 @@ int main(int argc, char **argv)
break;
case 'V':
printf("%s version " QEMU_FULL_VERSION "\n"
- QEMU_COPYRIGHT "\n", progname);
+ QEMU_COPYRIGHT "\n", error_get_progname());
exit(0);
case 'h':
-usage(progname);
+usage(error_get_progname());
exit(0);
case 'U':
force_share = true;
@@ -600,13 +598,13 @@ int main(int argc, char **argv)
imageOpts = true;
break;
default:
-usage(progname);
+usage(error_get_progname());
exit(1);
}
}
if ((argc - optind) > 1) {
-usage(progname);
+usage(error_get_progname());
exit(1);
}
--
2.20.1
[Qemu-devel] [PATCH v6 2/2] log: Make glib logging go through QEMU
This commit adds a qemu_init_logging() helper which calls
g_log_set_default_handler() so that glib logs (g_log, g_warning, ...)
are handled similarly to other QEMU logs. This means they will get a
timestamp if timestamps are enabled, and they will go through the
HMP monitor if one is configured.
This commit also adds a call to qemu_init_logging() to the binaries
installed by QEMU.
glib debug messages are enabled through G_MESSAGES_DEBUG similarly to
glib default log handler.
At the moment, this change will mostly impact SPICE logging if your
spice version is >= 0.14.1. With older spice versions, this is not going
to work as expected, but will not have any ill effect, so this call is
not conditional on the SPICE version.
Signed-off-by: Christophe Fergeau
---
bsd-user/main.c | 2 ++
include/qemu/error-report.h | 3 ++-
linux-user/main.c | 2 ++
qemu-img.c | 2 +-
qemu-io.c | 2 +-
qemu-nbd.c | 2 +-
scsi/qemu-pr-helper.c | 1 +
util/qemu-error.c | 48 -
vl.c| 2 +-
9 files changed, 58 insertions(+), 6 deletions(-)
diff --git a/bsd-user/main.c b/bsd-user/main.c
index 0d3156974c..8fd8ae4127 100644
--- a/bsd-user/main.c
+++ b/bsd-user/main.c
@@ -24,6 +24,7 @@
#include "qapi/error.h"
#include "qemu.h"
#include "qemu/config-file.h"
+#include "qemu/error-report.h"
#include "qemu/path.h"
#include "qemu/help_option.h"
#include "cpu.h"
@@ -743,6 +744,7 @@ int main(int argc, char **argv)
if (argc <= 1)
usage();
+error_init(argv[0]);
module_call_init(MODULE_INIT_TRACE);
qemu_init_cpu_list();
module_call_init(MODULE_INIT_QOM);
diff --git a/include/qemu/error-report.h b/include/qemu/error-report.h
index 0a8d9cc9ea..ce43c02314 100644
--- a/include/qemu/error-report.h
+++ b/include/qemu/error-report.h
@@ -34,7 +34,6 @@ void error_vprintf(const char *fmt, va_list ap)
GCC_FMT_ATTR(1, 0);
void error_printf(const char *fmt, ...) GCC_FMT_ATTR(1, 2);
void error_vprintf_unless_qmp(const char *fmt, va_list ap) GCC_FMT_ATTR(1, 0);
void error_printf_unless_qmp(const char *fmt, ...) GCC_FMT_ATTR(1, 2);
-void error_set_progname(const char *argv0);
void error_vreport(const char *fmt, va_list ap) GCC_FMT_ATTR(1, 0);
void warn_vreport(const char *fmt, va_list ap) GCC_FMT_ATTR(1, 0);
@@ -49,6 +48,8 @@ bool error_report_once_cond(bool *printed, const char *fmt,
...)
bool warn_report_once_cond(bool *printed, const char *fmt, ...)
GCC_FMT_ATTR(2, 3);
+void error_init(const char *argv0);
+
/*
* Similar to error_report(), except it prints the message just once.
* Return true when it prints, false otherwise.
diff --git a/linux-user/main.c b/linux-user/main.c
index a0aba9cb1e..f9efe9ff6e 100644
--- a/linux-user/main.c
+++ b/linux-user/main.c
@@ -27,6 +27,7 @@
#include "qemu/path.h"
#include "qemu/config-file.h"
#include "qemu/cutils.h"
+#include "qemu/error-report.h"
#include "qemu/help_option.h"
#include "cpu.h"
#include "exec/exec-all.h"
@@ -600,6 +601,7 @@ int main(int argc, char **argv, char **envp)
int ret;
int execfd;
+error_init(argv[0]);
module_call_init(MODULE_INIT_TRACE);
qemu_init_cpu_list();
module_call_init(MODULE_INIT_QOM);
diff --git a/qemu-img.c b/qemu-img.c
index ad04f59565..0af9cac244 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -4912,8 +4912,8 @@ int main(int argc, char **argv)
signal(SIGPIPE, SIG_IGN);
#endif
+error_init(argv[0]);
module_call_init(MODULE_INIT_TRACE);
-error_set_progname(argv[0]);
qemu_init_exec_dir(argv[0]);
if (qemu_init_main_loop(_error)) {
diff --git a/qemu-io.c b/qemu-io.c
index 2c52ac0ade..8d5d5911cb 100644
--- a/qemu-io.c
+++ b/qemu-io.c
@@ -522,8 +522,8 @@ int main(int argc, char **argv)
signal(SIGPIPE, SIG_IGN);
#endif
+error_init(argv[0]);
module_call_init(MODULE_INIT_TRACE);
-error_set_progname(argv[0]);
qemu_init_exec_dir(argv[0]);
qcrypto_init(_fatal);
diff --git a/qemu-nbd.c b/qemu-nbd.c
index 1f7b2a03f5..39849f1692 100644
--- a/qemu-nbd.c
+++ b/qemu-nbd.c
@@ -685,8 +685,8 @@ int main(int argc, char **argv)
signal(SIGPIPE, SIG_IGN);
#endif
+error_init(argv[0]);
module_call_init(MODULE_INIT_TRACE);
-error_set_progname(argv[0]);
qcrypto_init(_fatal);
module_call_init(MODULE_INIT_QOM);
diff --git a/scsi/qemu-pr-helper.c b/scsi/qemu-pr-helper.c
index e7af637232..2541fbbd1b 100644
--- a/scsi/qemu-pr-helper.c
+++ b/scsi/qemu-pr-helper.c
@@ -895,6 +895,7 @@ int main(int argc, char **argv)
signal(SIGPIPE, SIG_IGN);
+error_init(argv[0]);
module_call_init(MODULE_INIT_TRACE);
module_call_init(MODULE_INIT_QOM);
qemu_add_opts(_trace_opts);
diff --git a/util/qemu-error.c b/uti
Re: [Qemu-devel] [PATCH v5] log: Make glib logging go through QEMU
On Thu, Jan 24, 2019 at 05:10:28PM +0100, Markus Armbruster wrote:
> Christophe Fergeau writes:
>
> > This commit adds a qemu_init_logging() helper which calls
> > g_log_set_default_handler() so that glib logs (g_log, g_warning, ...)
> > are handled similarly to other QEMU logs. This means they will get a
> > timestamp if timestamps are enabled, and they will go through the
> > monitor if one is configured.
>
> s/monitor/HMP monitor/
>
> I see why one would like to extend the timestamp feature to GLib log
> messages. Routing them through the HMP monitor is perhaps debatable.
> Cc: Dave in case he has an opinion.
Yep, I don't mind whether this goes through HMP or not, this definitely
was not one of my goals when writing this patch. I mention it in the
commit log because this is what the current code is going to do.
> > [snip]
> > diff --git a/util/qemu-error.c b/util/qemu-error.c
> > index fcbe8a1f74..1118ed4695 100644
> > --- a/util/qemu-error.c
> > +++ b/util/qemu-error.c
> > @@ -345,3 +345,50 @@ bool warn_report_once_cond(bool *printed, const char
> > *fmt, ...)
> > va_end(ap);
> > return true;
> > }
> > +
> > +static char *qemu_glog_domains;
> > +
> > +static void qemu_log_func(const gchar *log_domain,
> > + GLogLevelFlags log_level,
> > + const gchar *message,
> > + gpointer user_data)
> > +{
> > +switch (log_level & G_LOG_LEVEL_MASK) {
> > +case G_LOG_LEVEL_DEBUG:
> > +/* Use same G_MESSAGES_DEBUG logic as glib to enable/disable debug
> > + * messages
> > + */
>
> Wing both ends of the comment, please.
Fixed.
>
> > +if (qemu_glog_domains == NULL) {
> > +break;
> > +}
> > +if (strcmp(qemu_glog_domains, "all") != 0 &&
> > + (log_domain == NULL || !strstr(qemu_glog_domains, log_domain))) {
> > +break;
> > +}
> > +/* Fall through */
> > +case G_LOG_LEVEL_INFO:
> > +/* Fall through */
>
> g_log_default_handler() applies G_MESSAGES_DEBUG suppression logic to
> G_LOG_LEVEL_INFO messages, too. Do you deviate intentionally?
Nope, I thought INFO and MESSAGE were equivalent. Fixed now.
>
> > +case G_LOG_LEVEL_MESSAGE:
> > +info_report("%s: %s", log_domain, message);
>
> @log_domain can be null. You even check for that above.
Fixed.
>
> > +break;
> > +case G_LOG_LEVEL_WARNING:
> > +warn_report("%s: %s", log_domain, message);
> > +break;
> > +case G_LOG_LEVEL_CRITICAL:
> > +/* Fall through */
> > +case G_LOG_LEVEL_ERROR:
> > +error_report("%s: %s", log_domain, message);
> > +break;
>
> Sure we don't need to check for G_LOG_FLAG_RECURSION?
> g_log_default_handler() has a conditional for that...
>
> Not sure it has anything for G_LOG_FLAG_FATAL; it's code is surprisingly
> complex.
Both are filtered out from the switch() through G_LOG_LEVEL_MASK:
switch (log_level & G_LOG_LEVEL_MASK) {
and
https://developer.gnome.org/glib/2.58/glib-Message-Logging.html#G-LOG-FLAG-RECURSION:CAPS
documents G_LOG_FLAG_FATAL and G_LOG_FLAG_RECURSION as fatal.
However, g_log_set_handler() then mentions handling them anyways:
https://developer.gnome.org/glib/2.58/glib-Message-Logging.html#g-log-set-handler
"Sets the log handler for a domain and a set of log levels. To handle
fatal and recursive messages the log_levels parameter must be combined
with the G_LOG_FLAG_FATAL and G_LOG_FLAG_RECURSION bit flags."
My understanding of glib's code is if any of these 2 flags is set, glib
is going to abort anyways, so handling or not handling these flags is
not going to make a big difference.
>
> > +}
> > +}
> > +
> > +/*
> > + * Init QEMU logging subsystem. This sets up glib logging so libraries
> > using it
> > + * also print their logs through {info,warn,error}_report.
> > + */
>
> Format like the other function comments:
>
>/*
> * Init QEMU logging subsystem.
> * This sets up glib logging so libraries using it also print their logs
> * through error_report(), warn_report(), info_report().
> */
>
> Braces expanded for better grepability.
>
> > +void qemu_init_logging(void)
>
> Naming is hard... Yes, this "initializes logging" in a sense: it
> installs a GLib default log handler that routes GLib log messages
> through this module. But that's detail; the callers don't care what
&
Re: [Qemu-devel] [PATCH v5] log: Make glib logging go through QEMU
Hey, On Thu, Jan 24, 2019 at 10:38:37AM +0100, Markus Armbruster wrote: > Christophe Fergeau writes: > > > This commit adds a qemu_init_logging() helper which calls > > g_log_set_default_handler() so that glib logs (g_log, g_warning, ...) > > are handled similarly to other QEMU logs. This means they will get a > > timestamp if timestamps are enabled, and they will go through the > > monitor if one is configured. > > This commit also adds a call to qemu_init_logging() to the binaries > > installed by QEMU. > > glib debug messages are enabled through G_MESSAGES_DEBUG similarly to > > glib default log handler. > > > > At the moment, this change will mostly impact SPICE logging if your > > spice version is >= 0.14.1. With older spice versions, this is not going > > to work as expected, but will not have any ill effect, so this call is > > not conditional on the SPICE version. > > > > Signed-off-by: Christophe Fergeau > > Reviewed-by: Daniel P. Berrangé > > Reviewed-by: Stefan Hajnoczi > > Do you expect this to go through my tree? To be honest, I don't know through whose tree this should go. > Hint: if you do, cc'ing me tends to help ;) > > scripts/get_maintainer.pl can be your friend. I ran it, but it returned a few too many names given that it touches files in various subsystems, I was not sure I should cc: everyone. Now that I look again, at least "(supporter:Error reporting)" would have made sense. Thanks for noticing this patch without the cc: :) Christophe signature.asc Description: PGP signature
Re: [Qemu-devel] [PATCH] json: Fix % handling when not interpolating
Hey,
On Thu, Jan 24, 2019 at 10:35:52AM +0100, Markus Armbruster wrote:
> Markus Armbruster writes:
>
> > Eric Blake writes:
> >
> >> On 1/2/19 12:01 PM, Christophe Fergeau wrote:
> >>> Adding Markus to cc: list, I forgot to do it when sending the patch.
> >>
> >> Also worth backporting via qemu-stable, now in cc.
> >>
> >>>
> >>> Christophe
> >>>
> >>> On Wed, Jan 02, 2019 at 03:05:35PM +0100, Christophe Fergeau wrote:
> >>>> commit 8bca4613 added support for %% in json strings when interpolating,
> >>>> but in doing so, this broke handling of % when not interpolating as the
> >>>> '%' is skipped in both cases.
> >>>> This commit ensures we only try to handle %% when interpolating.
> >
> > Impact?
> >
> > If you're unable to assess, could you give us at least a reproducer?
> >
> >>>> Signed-off-by: Christophe Fergeau
> >>>> ---
> >>>> qobject/json-parser.c | 10 ++
> >>>> tests/check-qjson.c | 5 +
> >>>> 2 files changed, 11 insertions(+), 4 deletions(-)
> >>>>
> >>
> >> Reviewed-by: Eric Blake
> >
> > Patch looks good to me, but I'd like us to improve the commit message.
>
> Let me try:
>
> json: Fix % handling when not interpolating
>
> Commit 8bca4613 added support for %% in json strings when interpolating,
> but in doing so broke handling of % when not interpolating.
>
> When parse_string() is fed a string token containing '%', it skips the
> '%' regardless of ctxt->ap, i.e. even it's not interpolating. If the
> '%' is the string's last character, it fails an assertion. Else, it
> "merely" swallows the '%'.
>
> Fix parse_string() to handle '%' specially only when interpolating.
>
> To gauge the bug's impact, let's review non-interpolating users of this
> parser, i.e. code passing NULL context to json_message_parser_init():
>
> * tests/check-qjson.c, tests/test-qobject-input-visitor.c,
> tests/test-visitor-serialization.c
>
> Plenty of tests, but we still failed to cover the buggy case.
>
> * monitor.c: QMP input
>
> * qga/main.c: QGA input
>
> * qobject_from_json():
>
> - qobject-input-visitor.c: JSON command line option arguments of
> -display and -blockdev
>
> Reproducer: -blockdev '{"%"}'
>
> - block.c: JSON pseudo-filenames starting with "json:"
>
> Reproducer: https://bugzilla.redhat.com/show_bug.cgi?id=1668244#c3
>
> - block/rbd.c: JSON key pairs
>
> Pseudo-filenames starting with "rbd:".
>
> Command line, QMP and QGA input are trusted.
>
> Filenames are trusted when they come from command line, QMP or HMP.
> They are untrusted when they come from from image file headers.
> Example: QCOW2 backing file name. Note that this is *not* the security
> boundary between host and guest. It's the boundary between host and an
> image file from an untrusted source.
>
> Neither failing an assertion nor skipping a character in a filename of
> your choice looks exploitable. Note that we don't support compiling
> with NDEBUG.
>
> Fixes: 8bca4613e6cddd948895b8db3def05950463495b
> Cc: qemu-sta...@nongnu.org
>
> Comments?
This looks good to me, thanks for the expanded log!
Christophe
signature.asc
Description: PGP signature
[Qemu-devel] [PATCH v5] log: Make glib logging go through QEMU
This commit adds a qemu_init_logging() helper which calls g_log_set_default_handler() so that glib logs (g_log, g_warning, ...) are handled similarly to other QEMU logs. This means they will get a timestamp if timestamps are enabled, and they will go through the monitor if one is configured. This commit also adds a call to qemu_init_logging() to the binaries installed by QEMU. glib debug messages are enabled through G_MESSAGES_DEBUG similarly to glib default log handler. At the moment, this change will mostly impact SPICE logging if your spice version is >= 0.14.1. With older spice versions, this is not going to work as expected, but will not have any ill effect, so this call is not conditional on the SPICE version. Signed-off-by: Christophe Fergeau Reviewed-by: Daniel P. Berrangé Reviewed-by: Stefan Hajnoczi --- One more iteration of the patch as it hit CI failures (https://patchew.org/QEMU/20181214105642.673-1-cferg...@redhat.com/ ) Only difference from v4 is the addition of #include "qemu/error-report.h" in bsd-user and linux-user. bsd-user/main.c | 2 ++ include/qemu/error-report.h | 2 ++ linux-user/main.c | 2 ++ qemu-img.c | 1 + qemu-io.c | 1 + qemu-nbd.c | 1 + scsi/qemu-pr-helper.c | 1 + util/qemu-error.c | 47 + vl.c| 1 + 9 files changed, 58 insertions(+) diff --git a/bsd-user/main.c b/bsd-user/main.c index 0d3156974c..0df5c853d3 100644 --- a/bsd-user/main.c +++ b/bsd-user/main.c @@ -24,6 +24,7 @@ #include "qapi/error.h" #include "qemu.h" #include "qemu/config-file.h" +#include "qemu/error-report.h" #include "qemu/path.h" #include "qemu/help_option.h" #include "cpu.h" @@ -743,6 +744,7 @@ int main(int argc, char **argv) if (argc <= 1) usage(); +qemu_init_logging(); module_call_init(MODULE_INIT_TRACE); qemu_init_cpu_list(); module_call_init(MODULE_INIT_QOM); diff --git a/include/qemu/error-report.h b/include/qemu/error-report.h index 0a8d9cc9ea..2852e9df2a 100644 --- a/include/qemu/error-report.h +++ b/include/qemu/error-report.h @@ -49,6 +49,8 @@ bool error_report_once_cond(bool *printed, const char *fmt, ...) bool warn_report_once_cond(bool *printed, const char *fmt, ...) GCC_FMT_ATTR(2, 3); +void qemu_init_logging(void); + /* * Similar to error_report(), except it prints the message just once. * Return true when it prints, false otherwise. diff --git a/linux-user/main.c b/linux-user/main.c index a0aba9cb1e..d9b3ffd1f4 100644 --- a/linux-user/main.c +++ b/linux-user/main.c @@ -27,6 +27,7 @@ #include "qemu/path.h" #include "qemu/config-file.h" #include "qemu/cutils.h" +#include "qemu/error-report.h" #include "qemu/help_option.h" #include "cpu.h" #include "exec/exec-all.h" @@ -600,6 +601,7 @@ int main(int argc, char **argv, char **envp) int ret; int execfd; +qemu_init_logging(); module_call_init(MODULE_INIT_TRACE); qemu_init_cpu_list(); module_call_init(MODULE_INIT_QOM); diff --git a/qemu-img.c b/qemu-img.c index ad04f59565..9214392565 100644 --- a/qemu-img.c +++ b/qemu-img.c @@ -4912,6 +4912,7 @@ int main(int argc, char **argv) signal(SIGPIPE, SIG_IGN); #endif +qemu_init_logging(); module_call_init(MODULE_INIT_TRACE); error_set_progname(argv[0]); qemu_init_exec_dir(argv[0]); diff --git a/qemu-io.c b/qemu-io.c index 6df7731af4..ad38d12e68 100644 --- a/qemu-io.c +++ b/qemu-io.c @@ -524,6 +524,7 @@ int main(int argc, char **argv) signal(SIGPIPE, SIG_IGN); #endif +qemu_init_logging(); module_call_init(MODULE_INIT_TRACE); progname = g_path_get_basename(argv[0]); qemu_init_exec_dir(argv[0]); diff --git a/qemu-nbd.c b/qemu-nbd.c index 51b55f2e06..274b22d445 100644 --- a/qemu-nbd.c +++ b/qemu-nbd.c @@ -581,6 +581,7 @@ int main(int argc, char **argv) signal(SIGPIPE, SIG_IGN); #endif +qemu_init_logging(); module_call_init(MODULE_INIT_TRACE); error_set_progname(argv[0]); qcrypto_init(_fatal); diff --git a/scsi/qemu-pr-helper.c b/scsi/qemu-pr-helper.c index e7af637232..523f8b237c 100644 --- a/scsi/qemu-pr-helper.c +++ b/scsi/qemu-pr-helper.c @@ -895,6 +895,7 @@ int main(int argc, char **argv) signal(SIGPIPE, SIG_IGN); +qemu_init_logging(); module_call_init(MODULE_INIT_TRACE); module_call_init(MODULE_INIT_QOM); qemu_add_opts(_trace_opts); diff --git a/util/qemu-error.c b/util/qemu-error.c index fcbe8a1f74..1118ed4695 100644 --- a/util/qemu-error.c +++ b/util/qemu-error.c @@ -345,3 +345,50 @@ bool warn_report_once_cond(bool *printed, const char *fmt, ...) va_end(ap); return true; } + +static char *qemu_glog_domains; + +static void qemu_log_func(const gchar *log_domain, +
Re: [Qemu-devel] [PATCH v4] log: Make glib logging go through QEMU
On Tue, Jan 15, 2019 at 02:09:12PM +, Stefan Hajnoczi wrote:
> > In these codepaths, I'm confident up to
> > void monitor_vfprintf(FILE *stream, const char *fmt, va_list ap)
> > {
> > if (cur_mon && !monitor_cur_is_qmp()) {
> > monitor_vprintf(cur_mon, fmt, ap);
> > } else {
> > vfprintf(stream, fmt, ap);
> > }
> > }
> >
> > From there, it was not obvious to me whether the monitor_vprintf
> > codepath can be triggered from info/warn/error_report or not. If they
> > can, I'd feel less confident that monitor_vprintf will never indirectly
> > call glib log functions.
>
> I think we're okay because otherwise monitor_vprintf() ->
> info/warn/error_report() would already be an infinite loop today.
Ah good point :)
Christophe
signature.asc
Description: PGP signature
Re: [Qemu-devel] [PATCH v4] log: Make glib logging go through QEMU
On Fri, Jan 04, 2019 at 03:40:34PM +, Stefan Hajnoczi wrote:
> On Thu, Jan 03, 2019 at 03:14:55PM +0100, Christophe Fergeau wrote:
> > Hey,
> >
> > On Thu, Jan 03, 2019 at 10:54:25AM +, Stefan Hajnoczi wrote:
> > > On Fri, Dec 14, 2018 at 11:56:42AM +0100, Christophe Fergeau wrote:
> > > > +static void qemu_log_func(const gchar *log_domain,
> > > > + GLogLevelFlags log_level,
> > > > + const gchar *message,
> > > > + gpointer user_data)
> > > > +{
> > > > +switch (log_level & G_LOG_LEVEL_MASK) {
> > > > +case G_LOG_LEVEL_DEBUG:
> > > > +/* Use same G_MESSAGES_DEBUG logic as glib to enable/disable
> > > > debug
> > > > + * messages
> > > > + */
> > > > +if (qemu_glog_domains == NULL) {
> > > > +break;
> > > > +}
> > > > +if (strcmp(qemu_glog_domains, "all") != 0 &&
> > > > + (log_domain == NULL || !strstr(qemu_glog_domains,
> > > > log_domain))) {
> > > > +break;
> > > > +}
> > > > +/* Fall through */
> > > > +case G_LOG_LEVEL_INFO:
> > > > +/* Fall through */
> > > > +case G_LOG_LEVEL_MESSAGE:
> > > > +info_report("%s: %s", log_domain, message);
> > >
> > > QEMU itself uses glib, so what happens if *_report() emit more log
> > > messages? Can this result in an infinite loop?
> >
> > If *_report try to output messages through the g_log API, glib will
> > catch it and will abort, so not an infinite loop, but not a
> > desirable outcome either.
> >
> > Some low-level logging functions which are going to dump the message
> > without glib (or using g_log_default_handler()) are needed.
> > The timestamping/prepending of 'error:', 'warning:' to the error message
> > is done right below *_report (in vreport) so this looked like a good
> > place for that.
>
> If you're confident that QEMU will never (indirectly) emit a glib log
> message from the *_report() code path, then I'm happy.
In these codepaths, I'm confident up to
void monitor_vfprintf(FILE *stream, const char *fmt, va_list ap)
{
if (cur_mon && !monitor_cur_is_qmp()) {
monitor_vprintf(cur_mon, fmt, ap);
} else {
vfprintf(stream, fmt, ap);
}
}
From there, it was not obvious to me whether the monitor_vprintf
codepath can be triggered from info/warn/error_report or not. If they
can, I'd feel less confident that monitor_vprintf will never indirectly
call glib log functions.
Christophe
>
> Stefan
signature.asc
Description: PGP signature
Re: [Qemu-devel] [PATCH] json: Fix % handling when not interpolating
On Mon, Jan 07, 2019 at 04:47:44PM +0100, Markus Armbruster wrote: > Eric Blake writes: > > > On 1/2/19 12:01 PM, Christophe Fergeau wrote: > >> Adding Markus to cc: list, I forgot to do it when sending the patch. > > > > Also worth backporting via qemu-stable, now in cc. > > > >> > >> Christophe > >> > >> On Wed, Jan 02, 2019 at 03:05:35PM +0100, Christophe Fergeau wrote: > >>> commit 8bca4613 added support for %% in json strings when interpolating, > >>> but in doing so, this broke handling of % when not interpolating as the > >>> '%' is skipped in both cases. > >>> This commit ensures we only try to handle %% when interpolating. > > Impact? > > If you're unable to assess, could you give us at least a reproducer? This all came from https://lists.freedesktop.org/archives/spice-devel/2018-December/046644.html Setting up a VM with libvirt with fails to start with: qemu-system-x86_64: qobject/json-parser.c:146: parse_string: Assertion `*ptr' failed. If you use 'password%%' as the password instead, when trying to connect to the VM, you type 'password%' as the password instead of 'password%%' as configured in the domain XML. Christophe signature.asc Description: PGP signature
Re: [Qemu-devel] [Spice-devel] Always get Invalid password while trying to connect to spice server
Hey,
On Thu, Jan 03, 2019 at 04:25:00PM -0600, Eric Blake wrote:
> On 12/27/18 8:51 AM, Niccolò Belli wrote:
> > On mercoledì 26 dicembre 2018 13:38:28 CET, Frediano Ziglio wrote:
> >> Yes, this looks like a format string error in the upper (not into
> >> spice) layer.
> >>
> >> This potentially is a security problem.
> >
> > Considering the spice server is exposed to the internet this is
> > definitely worth investigating.
> >
> >> The specific '%' character could be the issue, can you try others
> >> ('!', '@' and
> >> so on) ?
> >
> > I tried several other special characters and they all seems to work,
> > expect for "Password&&" which gets converted to "Password" (if
> > I type "Password" it works).
>
> Could it be related to this patch where our JSON code mishandles %?
> https://lists.gnu.org/archive/html/qemu-devel/2019-01/msg00108.html
Yes definitely, this is where the patch came from.
Mentioning this spice issue is yet another thing I should have added in the
commit log, but which I only thought about *after* having sent the patch :)
Christophe
signature.asc
Description: PGP signature
Re: [Qemu-devel] [PATCH v4] log: Make glib logging go through QEMU
Hey,
On Thu, Jan 03, 2019 at 10:54:25AM +, Stefan Hajnoczi wrote:
> On Fri, Dec 14, 2018 at 11:56:42AM +0100, Christophe Fergeau wrote:
> > +static void qemu_log_func(const gchar *log_domain,
> > + GLogLevelFlags log_level,
> > + const gchar *message,
> > + gpointer user_data)
> > +{
> > +switch (log_level & G_LOG_LEVEL_MASK) {
> > +case G_LOG_LEVEL_DEBUG:
> > +/* Use same G_MESSAGES_DEBUG logic as glib to enable/disable debug
> > + * messages
> > + */
> > +if (qemu_glog_domains == NULL) {
> > +break;
> > +}
> > +if (strcmp(qemu_glog_domains, "all") != 0 &&
> > + (log_domain == NULL || !strstr(qemu_glog_domains, log_domain))) {
> > +break;
> > +}
> > +/* Fall through */
> > +case G_LOG_LEVEL_INFO:
> > +/* Fall through */
> > +case G_LOG_LEVEL_MESSAGE:
> > +info_report("%s: %s", log_domain, message);
>
> QEMU itself uses glib, so what happens if *_report() emit more log
> messages? Can this result in an infinite loop?
If *_report try to output messages through the g_log API, glib will
catch it and will abort, so not an infinite loop, but not a
desirable outcome either.
Some low-level logging functions which are going to dump the message
without glib (or using g_log_default_handler()) are needed.
The timestamping/prepending of 'error:', 'warning:' to the error message
is done right below *_report (in vreport) so this looked like a good
place for that.
Christophe
signature.asc
Description: PGP signature
Re: [Qemu-devel] [PATCH] json: Fix % handling when not interpolating
Adding Markus to cc: list, I forgot to do it when sending the patch.
Christophe
On Wed, Jan 02, 2019 at 03:05:35PM +0100, Christophe Fergeau wrote:
> commit 8bca4613 added support for %% in json strings when interpolating,
> but in doing so, this broke handling of % when not interpolating as the
> '%' is skipped in both cases.
> This commit ensures we only try to handle %% when interpolating.
>
> Signed-off-by: Christophe Fergeau
> ---
> qobject/json-parser.c | 10 ++
> tests/check-qjson.c | 5 +
> 2 files changed, 11 insertions(+), 4 deletions(-)
>
> diff --git a/qobject/json-parser.c b/qobject/json-parser.c
> index 7a7ae9e8d1..d8eb210c0c 100644
> --- a/qobject/json-parser.c
> +++ b/qobject/json-parser.c
> @@ -208,11 +208,13 @@ static QString *parse_string(JSONParserContext *ctxt,
> JSONToken *token)
> }
> break;
> case '%':
> -if (ctxt->ap && ptr[1] != '%') {
> -parse_error(ctxt, token, "can't interpolate into string");
> -goto out;
> +if (ctxt->ap) {
> +if (ptr[1] != '%') {
> +parse_error(ctxt, token, "can't interpolate into
> string");
> +goto out;
> +}
> +ptr++;
> }
> -ptr++;
> /* fall through */
> default:
> cp = mod_utf8_codepoint(ptr, 6, );
> diff --git a/tests/check-qjson.c b/tests/check-qjson.c
> index d876a7a96e..fa2afccb0a 100644
> --- a/tests/check-qjson.c
> +++ b/tests/check-qjson.c
> @@ -175,6 +175,11 @@ static void utf8_string(void)
> "\xCE\xBA\xE1\xBD\xB9\xCF\x83\xCE\xBC\xCE\xB5",
> "\xCE\xBA\xE1\xBD\xB9\xCF\x83\xCE\xBC\xCE\xB5",
> "\\u03BA\\u1F79\\u03C3\\u03BC\\u03B5",
> +},
> +/* '%' character when not interpolating */
> +{
> +"100%",
> +"100%",
> },
> /* 2 Boundary condition test cases */
> /* 2.1 First possible sequence of a certain length */
> --
> 2.20.1
>
>
signature.asc
Description: PGP signature
[Qemu-devel] [PATCH] json: Fix % handling when not interpolating
commit 8bca4613 added support for %% in json strings when interpolating,
but in doing so, this broke handling of % when not interpolating as the
'%' is skipped in both cases.
This commit ensures we only try to handle %% when interpolating.
Signed-off-by: Christophe Fergeau
---
qobject/json-parser.c | 10 ++
tests/check-qjson.c | 5 +
2 files changed, 11 insertions(+), 4 deletions(-)
diff --git a/qobject/json-parser.c b/qobject/json-parser.c
index 7a7ae9e8d1..d8eb210c0c 100644
--- a/qobject/json-parser.c
+++ b/qobject/json-parser.c
@@ -208,11 +208,13 @@ static QString *parse_string(JSONParserContext *ctxt,
JSONToken *token)
}
break;
case '%':
-if (ctxt->ap && ptr[1] != '%') {
-parse_error(ctxt, token, "can't interpolate into string");
-goto out;
+if (ctxt->ap) {
+if (ptr[1] != '%') {
+parse_error(ctxt, token, "can't interpolate into string");
+goto out;
+}
+ptr++;
}
-ptr++;
/* fall through */
default:
cp = mod_utf8_codepoint(ptr, 6, );
diff --git a/tests/check-qjson.c b/tests/check-qjson.c
index d876a7a96e..fa2afccb0a 100644
--- a/tests/check-qjson.c
+++ b/tests/check-qjson.c
@@ -175,6 +175,11 @@ static void utf8_string(void)
"\xCE\xBA\xE1\xBD\xB9\xCF\x83\xCE\xBC\xCE\xB5",
"\xCE\xBA\xE1\xBD\xB9\xCF\x83\xCE\xBC\xCE\xB5",
"\\u03BA\\u1F79\\u03C3\\u03BC\\u03B5",
+},
+/* '%' character when not interpolating */
+{
+"100%",
+"100%",
},
/* 2 Boundary condition test cases */
/* 2.1 First possible sequence of a certain length */
--
2.20.1
[Qemu-devel] [PATCH v4] log: Make glib logging go through QEMU
This commit adds a qemu_init_logging() helper which calls
g_log_set_default_handler() so that glib logs (g_log, g_warning, ...)
are handled similarly to other QEMU logs. This means they will get a
timestamp if timestamps are enabled, and they will go through the
monitor if one is configured.
This commit also adds a call to qemu_init_logging() to the binaries
installed by QEMU.
glib debug messages are enabled through G_MESSAGES_DEBUG similarly to
glib default log handler.
At the moment, this change will mostly impact SPICE logging if your
spice version is >= 0.14.1. With older spice versions, this is not going
to work as expected, but will not have any ill effect, so this call is
not conditional on the SPICE version.
Signed-off-by: Christophe Fergeau
Reviewed-by: Daniel P. Berrangé
---
Changes since v3:
- Fixed style issue: removed 2 extra spaces before parens
(reported by checkpatch)
bsd-user/main.c | 1 +
include/qemu/error-report.h | 2 ++
linux-user/main.c | 1 +
qemu-img.c | 1 +
qemu-io.c | 1 +
qemu-nbd.c | 1 +
scsi/qemu-pr-helper.c | 1 +
util/qemu-error.c | 47 +
vl.c| 1 +
9 files changed, 56 insertions(+)
diff --git a/bsd-user/main.c b/bsd-user/main.c
index 0d3156974c..96787b27ef 100644
--- a/bsd-user/main.c
+++ b/bsd-user/main.c
@@ -743,6 +743,7 @@ int main(int argc, char **argv)
if (argc <= 1)
usage();
+qemu_init_logging();
module_call_init(MODULE_INIT_TRACE);
qemu_init_cpu_list();
module_call_init(MODULE_INIT_QOM);
diff --git a/include/qemu/error-report.h b/include/qemu/error-report.h
index 0a8d9cc9ea..2852e9df2a 100644
--- a/include/qemu/error-report.h
+++ b/include/qemu/error-report.h
@@ -49,6 +49,8 @@ bool error_report_once_cond(bool *printed, const char *fmt,
...)
bool warn_report_once_cond(bool *printed, const char *fmt, ...)
GCC_FMT_ATTR(2, 3);
+void qemu_init_logging(void);
+
/*
* Similar to error_report(), except it prints the message just once.
* Return true when it prints, false otherwise.
diff --git a/linux-user/main.c b/linux-user/main.c
index 923cbb753a..de9ff462e9 100644
--- a/linux-user/main.c
+++ b/linux-user/main.c
@@ -600,6 +600,7 @@ int main(int argc, char **argv, char **envp)
int ret;
int execfd;
+qemu_init_logging();
module_call_init(MODULE_INIT_TRACE);
qemu_init_cpu_list();
module_call_init(MODULE_INIT_QOM);
diff --git a/qemu-img.c b/qemu-img.c
index ad04f59565..9214392565 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -4912,6 +4912,7 @@ int main(int argc, char **argv)
signal(SIGPIPE, SIG_IGN);
#endif
+qemu_init_logging();
module_call_init(MODULE_INIT_TRACE);
error_set_progname(argv[0]);
qemu_init_exec_dir(argv[0]);
diff --git a/qemu-io.c b/qemu-io.c
index 6df7731af4..ad38d12e68 100644
--- a/qemu-io.c
+++ b/qemu-io.c
@@ -524,6 +524,7 @@ int main(int argc, char **argv)
signal(SIGPIPE, SIG_IGN);
#endif
+qemu_init_logging();
module_call_init(MODULE_INIT_TRACE);
progname = g_path_get_basename(argv[0]);
qemu_init_exec_dir(argv[0]);
diff --git a/qemu-nbd.c b/qemu-nbd.c
index ca7109652e..2cac038230 100644
--- a/qemu-nbd.c
+++ b/qemu-nbd.c
@@ -570,6 +570,7 @@ int main(int argc, char **argv)
signal(SIGPIPE, SIG_IGN);
#endif
+qemu_init_logging();
module_call_init(MODULE_INIT_TRACE);
qcrypto_init(_fatal);
diff --git a/scsi/qemu-pr-helper.c b/scsi/qemu-pr-helper.c
index e7af637232..523f8b237c 100644
--- a/scsi/qemu-pr-helper.c
+++ b/scsi/qemu-pr-helper.c
@@ -895,6 +895,7 @@ int main(int argc, char **argv)
signal(SIGPIPE, SIG_IGN);
+qemu_init_logging();
module_call_init(MODULE_INIT_TRACE);
module_call_init(MODULE_INIT_QOM);
qemu_add_opts(_trace_opts);
diff --git a/util/qemu-error.c b/util/qemu-error.c
index fcbe8a1f74..1118ed4695 100644
--- a/util/qemu-error.c
+++ b/util/qemu-error.c
@@ -345,3 +345,50 @@ bool warn_report_once_cond(bool *printed, const char *fmt,
...)
va_end(ap);
return true;
}
+
+static char *qemu_glog_domains;
+
+static void qemu_log_func(const gchar *log_domain,
+ GLogLevelFlags log_level,
+ const gchar *message,
+ gpointer user_data)
+{
+switch (log_level & G_LOG_LEVEL_MASK) {
+case G_LOG_LEVEL_DEBUG:
+/* Use same G_MESSAGES_DEBUG logic as glib to enable/disable debug
+ * messages
+ */
+if (qemu_glog_domains == NULL) {
+break;
+}
+if (strcmp(qemu_glog_domains, "all") != 0 &&
+ (log_domain == NULL || !strstr(qemu_glog_domains, log_domain))) {
+break;
+}
+/* Fall through */
+case G_LOG_LEVEL_INFO:
+/* Fall through */
+case G_LOG_LEVEL_MESSAGE:
+info_report("%s: %s"
[Qemu-devel] [PATCH v3] log: Make glib logging go through QEMU
This commit adds a qemu_init_logging() helper which calls
g_log_set_default_handler() so that glib logs (g_log, g_warning, ...)
are handled similarly to other QEMU logs. This means they will get a
timestamp if timestamps are enabled, and they will go through the
monitor if one is configured.
This commit also adds a call to qemu_init_logging() to the binaries
installed by QEMU.
glib debug messages are enabled through G_MESSAGES_DEBUG similarly to
glib default log handler.
At the moment, this change will mostly impact SPICE logging if your
spice version is >= 0.14.1. With older spice versions, this is not going
to work as expected, but will not have any ill effect, so this call is
not conditional on the SPICE version.
Signed-off-by: Christophe Fergeau
---
Changes since v2:
- report G_LOG_LEVEL_CRITICAL using error_report() instead of
warn_report()
- print G_LOG_LEVEL_DEBUG messages when G_MESSAGES_DEBUG is set
- prepend glib log domain to messages
Changes since v1:
- introduced a qemu_init_logging() helper, and call that in more
binaries than just vl.c
bsd-user/main.c | 1 +
include/qemu/error-report.h | 2 ++
linux-user/main.c | 1 +
qemu-img.c | 1 +
qemu-io.c | 1 +
qemu-nbd.c | 1 +
scsi/qemu-pr-helper.c | 1 +
util/qemu-error.c | 47 +
vl.c| 1 +
9 files changed, 56 insertions(+)
diff --git a/bsd-user/main.c b/bsd-user/main.c
index 0d3156974c..96787b27ef 100644
--- a/bsd-user/main.c
+++ b/bsd-user/main.c
@@ -743,6 +743,7 @@ int main(int argc, char **argv)
if (argc <= 1)
usage();
+qemu_init_logging();
module_call_init(MODULE_INIT_TRACE);
qemu_init_cpu_list();
module_call_init(MODULE_INIT_QOM);
diff --git a/include/qemu/error-report.h b/include/qemu/error-report.h
index 0a8d9cc9ea..2852e9df2a 100644
--- a/include/qemu/error-report.h
+++ b/include/qemu/error-report.h
@@ -49,6 +49,8 @@ bool error_report_once_cond(bool *printed, const char *fmt,
...)
bool warn_report_once_cond(bool *printed, const char *fmt, ...)
GCC_FMT_ATTR(2, 3);
+void qemu_init_logging(void);
+
/*
* Similar to error_report(), except it prints the message just once.
* Return true when it prints, false otherwise.
diff --git a/linux-user/main.c b/linux-user/main.c
index 923cbb753a..de9ff462e9 100644
--- a/linux-user/main.c
+++ b/linux-user/main.c
@@ -600,6 +600,7 @@ int main(int argc, char **argv, char **envp)
int ret;
int execfd;
+qemu_init_logging();
module_call_init(MODULE_INIT_TRACE);
qemu_init_cpu_list();
module_call_init(MODULE_INIT_QOM);
diff --git a/qemu-img.c b/qemu-img.c
index ad04f59565..9214392565 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -4912,6 +4912,7 @@ int main(int argc, char **argv)
signal(SIGPIPE, SIG_IGN);
#endif
+qemu_init_logging();
module_call_init(MODULE_INIT_TRACE);
error_set_progname(argv[0]);
qemu_init_exec_dir(argv[0]);
diff --git a/qemu-io.c b/qemu-io.c
index 6df7731af4..ad38d12e68 100644
--- a/qemu-io.c
+++ b/qemu-io.c
@@ -524,6 +524,7 @@ int main(int argc, char **argv)
signal(SIGPIPE, SIG_IGN);
#endif
+qemu_init_logging();
module_call_init(MODULE_INIT_TRACE);
progname = g_path_get_basename(argv[0]);
qemu_init_exec_dir(argv[0]);
diff --git a/qemu-nbd.c b/qemu-nbd.c
index ca7109652e..2cac038230 100644
--- a/qemu-nbd.c
+++ b/qemu-nbd.c
@@ -570,6 +570,7 @@ int main(int argc, char **argv)
signal(SIGPIPE, SIG_IGN);
#endif
+qemu_init_logging();
module_call_init(MODULE_INIT_TRACE);
qcrypto_init(_fatal);
diff --git a/scsi/qemu-pr-helper.c b/scsi/qemu-pr-helper.c
index e7af637232..523f8b237c 100644
--- a/scsi/qemu-pr-helper.c
+++ b/scsi/qemu-pr-helper.c
@@ -895,6 +895,7 @@ int main(int argc, char **argv)
signal(SIGPIPE, SIG_IGN);
+qemu_init_logging();
module_call_init(MODULE_INIT_TRACE);
module_call_init(MODULE_INIT_QOM);
qemu_add_opts(_trace_opts);
diff --git a/util/qemu-error.c b/util/qemu-error.c
index fcbe8a1f74..38a1d79d33 100644
--- a/util/qemu-error.c
+++ b/util/qemu-error.c
@@ -345,3 +345,50 @@ bool warn_report_once_cond(bool *printed, const char *fmt,
...)
va_end(ap);
return true;
}
+
+static char *qemu_glog_domains;
+
+static void qemu_log_func(const gchar *log_domain,
+ GLogLevelFlags log_level,
+ const gchar *message,
+ gpointer user_data)
+{
+switch (log_level & G_LOG_LEVEL_MASK) {
+case G_LOG_LEVEL_DEBUG:
+/* Use same G_MESSAGES_DEBUG logic as glib to enable/disable debug
+ * messages
+ */
+if (qemu_glog_domains == NULL) {
+break;
+}
+if (strcmp (qemu_glog_domains, "all") != 0 &&
+ (log_domain == NULL || !strstr (qemu_glog_domains, log_do
Re: [Qemu-devel] [PATCH v2] log: Make glib logging go through QEMU
On Thu, Dec 13, 2018 at 11:52:12AM +, Daniel P. Berrangé wrote:
> On Thu, Dec 13, 2018 at 12:26:12PM +0100, Christophe Fergeau wrote:
> > This commit adds a qemu_init_logging() helper which calls
> > g_log_set_default_handler() so that glib logs (g_log, g_warning, ...)
> > are handled similarly to other QEMU logs. This means they will get a
> > timestamp if timestamps are enabled, and they will go through the
> > monitor if one is configured.
> > This commit also adds a call to qemu_init_logging() to the binaries
> > installed by QEMU.
> >
> > At the moment, this change will mostly impact SPICE logging if your
> > spice version is >= 0.14.1. With older spice versions, this is not going
> > to work as expected, but will not have any ill effect, so this call is
> > not conditional on the SPICE version.
> >
> > Signed-off-by: Christophe Fergeau
> > ---
> > Changes since v1:
> > - introduced a qemu_init_logging() helper, and call that in more
> > binaries than just vl.c
>
> > +static void qemu_log_func(const gchar *log_domain,
> > + GLogLevelFlags log_level,
> > + const gchar *message,
> > + gpointer user_data)
> > +{
> > +switch (log_level & G_LOG_LEVEL_MASK) {
> > +case G_LOG_LEVEL_DEBUG:
> > +break;
> > +case G_LOG_LEVEL_INFO:
> > +/* Fall through */
> > +case G_LOG_LEVEL_MESSAGE:
> > +info_report("%s", message);
> > +break;
> > +case G_LOG_LEVEL_WARNING:
> > +/* Fall through */
> > +case G_LOG_LEVEL_CRITICAL:
> > +warn_report("%s", message);
> > +break;
>
> This didn't adress my previous comment that _CRITICAL should do an
> error_report, not warn_reoprt.
My bad, sorry for missing it, v3 incoming.
Christophe
signature.asc
Description: PGP signature
[Qemu-devel] [PATCH v2] log: Make glib logging go through QEMU
This commit adds a qemu_init_logging() helper which calls
g_log_set_default_handler() so that glib logs (g_log, g_warning, ...)
are handled similarly to other QEMU logs. This means they will get a
timestamp if timestamps are enabled, and they will go through the
monitor if one is configured.
This commit also adds a call to qemu_init_logging() to the binaries
installed by QEMU.
At the moment, this change will mostly impact SPICE logging if your
spice version is >= 0.14.1. With older spice versions, this is not going
to work as expected, but will not have any ill effect, so this call is
not conditional on the SPICE version.
Signed-off-by: Christophe Fergeau
---
Changes since v1:
- introduced a qemu_init_logging() helper, and call that in more
binaries than just vl.c
bsd-user/main.c | 1 +
include/qemu/error-report.h | 2 ++
linux-user/main.c | 1 +
qemu-img.c | 1 +
qemu-io.c | 1 +
qemu-nbd.c | 1 +
scsi/qemu-pr-helper.c | 1 +
util/qemu-error.c | 33 +
vl.c| 1 +
9 files changed, 42 insertions(+)
diff --git a/bsd-user/main.c b/bsd-user/main.c
index 0d3156974c..96787b27ef 100644
--- a/bsd-user/main.c
+++ b/bsd-user/main.c
@@ -743,6 +743,7 @@ int main(int argc, char **argv)
if (argc <= 1)
usage();
+qemu_init_logging();
module_call_init(MODULE_INIT_TRACE);
qemu_init_cpu_list();
module_call_init(MODULE_INIT_QOM);
diff --git a/include/qemu/error-report.h b/include/qemu/error-report.h
index 0a8d9cc9ea..2852e9df2a 100644
--- a/include/qemu/error-report.h
+++ b/include/qemu/error-report.h
@@ -49,6 +49,8 @@ bool error_report_once_cond(bool *printed, const char *fmt,
...)
bool warn_report_once_cond(bool *printed, const char *fmt, ...)
GCC_FMT_ATTR(2, 3);
+void qemu_init_logging(void);
+
/*
* Similar to error_report(), except it prints the message just once.
* Return true when it prints, false otherwise.
diff --git a/linux-user/main.c b/linux-user/main.c
index 923cbb753a..de9ff462e9 100644
--- a/linux-user/main.c
+++ b/linux-user/main.c
@@ -600,6 +600,7 @@ int main(int argc, char **argv, char **envp)
int ret;
int execfd;
+qemu_init_logging();
module_call_init(MODULE_INIT_TRACE);
qemu_init_cpu_list();
module_call_init(MODULE_INIT_QOM);
diff --git a/qemu-img.c b/qemu-img.c
index ad04f59565..9214392565 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -4912,6 +4912,7 @@ int main(int argc, char **argv)
signal(SIGPIPE, SIG_IGN);
#endif
+qemu_init_logging();
module_call_init(MODULE_INIT_TRACE);
error_set_progname(argv[0]);
qemu_init_exec_dir(argv[0]);
diff --git a/qemu-io.c b/qemu-io.c
index 6df7731af4..ad38d12e68 100644
--- a/qemu-io.c
+++ b/qemu-io.c
@@ -524,6 +524,7 @@ int main(int argc, char **argv)
signal(SIGPIPE, SIG_IGN);
#endif
+qemu_init_logging();
module_call_init(MODULE_INIT_TRACE);
progname = g_path_get_basename(argv[0]);
qemu_init_exec_dir(argv[0]);
diff --git a/qemu-nbd.c b/qemu-nbd.c
index ca7109652e..2cac038230 100644
--- a/qemu-nbd.c
+++ b/qemu-nbd.c
@@ -570,6 +570,7 @@ int main(int argc, char **argv)
signal(SIGPIPE, SIG_IGN);
#endif
+qemu_init_logging();
module_call_init(MODULE_INIT_TRACE);
qcrypto_init(_fatal);
diff --git a/scsi/qemu-pr-helper.c b/scsi/qemu-pr-helper.c
index e7af637232..523f8b237c 100644
--- a/scsi/qemu-pr-helper.c
+++ b/scsi/qemu-pr-helper.c
@@ -895,6 +895,7 @@ int main(int argc, char **argv)
signal(SIGPIPE, SIG_IGN);
+qemu_init_logging();
module_call_init(MODULE_INIT_TRACE);
module_call_init(MODULE_INIT_QOM);
qemu_add_opts(_trace_opts);
diff --git a/util/qemu-error.c b/util/qemu-error.c
index fcbe8a1f74..db838e9f20 100644
--- a/util/qemu-error.c
+++ b/util/qemu-error.c
@@ -345,3 +345,36 @@ bool warn_report_once_cond(bool *printed, const char *fmt,
...)
va_end(ap);
return true;
}
+
+static void qemu_log_func(const gchar *log_domain,
+ GLogLevelFlags log_level,
+ const gchar *message,
+ gpointer user_data)
+{
+switch (log_level & G_LOG_LEVEL_MASK) {
+case G_LOG_LEVEL_DEBUG:
+break;
+case G_LOG_LEVEL_INFO:
+/* Fall through */
+case G_LOG_LEVEL_MESSAGE:
+info_report("%s", message);
+break;
+case G_LOG_LEVEL_WARNING:
+/* Fall through */
+case G_LOG_LEVEL_CRITICAL:
+warn_report("%s", message);
+break;
+case G_LOG_LEVEL_ERROR:
+error_report("%s", message);
+break;
+}
+}
+
+/*
+ * Init QEMU logging subsystem. This sets up glib logging so libraries using it
+ * also print their logs through {info,warn,error}_report.
+ */
+void qemu_init_logging(void)
+{
+g_log_set_default_handler(qemu_log_func, NULL);
+}
diff --git a/vl.c b/
[Qemu-devel] [PATCH] spice: Make logging printing go through QEMU
Since spice 0.14.1, it's possible to use g_log_set_default_handler() to
use a custom function to print spice-server's logs, which gives more
consistent log output.
With older spice versions, this is not going to work as expected, but
will not have any ill effect, so this call is not conditional on spice
version.
Since this added g_log_set_default_handler() will bridge glib logging
and QEMU logging, the call might fit better in a more generic place.
Signed-off-by: Christophe Fergeau
---
ui/spice-core.c | 25 +
1 file changed, 25 insertions(+)
diff --git a/ui/spice-core.c b/ui/spice-core.c
index ebaae24643..443a2b3d32 100644
--- a/ui/spice-core.c
+++ b/ui/spice-core.c
@@ -632,6 +632,30 @@ static void vm_change_state_handler(void *opaque, int
running,
}
}
+static void qemu_log_func(const gchar *log_domain,
+ GLogLevelFlags log_level,
+ const gchar *message,
+ gpointer user_data)
+{
+switch (log_level & G_LOG_LEVEL_MASK) {
+case G_LOG_LEVEL_DEBUG:
+break;
+case G_LOG_LEVEL_INFO:
+/* Fall through */
+case G_LOG_LEVEL_MESSAGE:
+info_report("%s", message);
+break;
+case G_LOG_LEVEL_WARNING:
+/* Fall through */
+case G_LOG_LEVEL_CRITICAL:
+warn_report("%s", message);
+break;
+case G_LOG_LEVEL_ERROR:
+error_report("%s", message);
+break;
+}
+}
+
void qemu_spice_init(void)
{
QemuOpts *opts = QTAILQ_FIRST(_spice_opts.head);
@@ -647,6 +671,7 @@ void qemu_spice_init(void)
spice_wan_compression_t wan_compr;
bool seamless_migration;
+g_log_set_default_handler(qemu_log_func, NULL);
qemu_thread_get_self();
if (!opts) {
--
2.19.2
Re: [Qemu-devel] [Spice-devel] [PATCH] spice: Use new SpiceImageCompression definition
hey,
On Mon, Nov 26, 2018 at 03:30:36PM +, Frediano Ziglio wrote:
> Definitions were updated by spice-server in patch de66161 included
> in 0.12.6 released on 12th June 2015.
QEMU's configure only checks for spice-server 0.12.0:
$pkg_config --atleast-version=0.12.0 spice-server
if $pkg_config --atleast-version=0.12.0 spice-server && \
$pkg_config --atleast-version=0.12.3 spice-protocol && \
compile_prog "$spice_cflags" "$spice_libs" ; then
spice="yes"
libs_softmmu="$libs_softmmu $spice_libs"
QEMU_CFLAGS="$QEMU_CFLAGS $spice_cflags"
spice_protocol_version=$($pkg_config --modversion spice-protocol)
spice_server_version=$($pkg_config --modversion spice-server)
else
if test "$spice" = "yes" ; then
feature_not_found "spice" \
"Install spice-server(>=0.12.0) and spice-protocol(>=0.12.3) devel"
fi
spice="no"
fi
I don't know how far back QEMU wants to support spice-server.
Apart from this, the patch looks good to me.
Christophe
>
> Signed-off-by: Frediano Ziglio
> ---
> ui/spice-core.c | 16
> 1 file changed, 8 insertions(+), 8 deletions(-)
>
> diff --git a/ui/spice-core.c b/ui/spice-core.c
> index ebaae24643..2e6a255a35 100644
> --- a/ui/spice-core.c
> +++ b/ui/spice-core.c
> @@ -331,12 +331,12 @@ static const char *stream_video_names[] = {
> stream_video_names, ARRAY_SIZE(stream_video_names))
>
> static const char *compression_names[] = {
> -[ SPICE_IMAGE_COMPRESS_OFF ] = "off",
> -[ SPICE_IMAGE_COMPRESS_AUTO_GLZ ] = "auto_glz",
> -[ SPICE_IMAGE_COMPRESS_AUTO_LZ ] = "auto_lz",
> -[ SPICE_IMAGE_COMPRESS_QUIC ] = "quic",
> -[ SPICE_IMAGE_COMPRESS_GLZ ] = "glz",
> -[ SPICE_IMAGE_COMPRESS_LZ ] = "lz",
> +[ SPICE_IMAGE_COMPRESSION_OFF ] = "off",
> +[ SPICE_IMAGE_COMPRESSION_AUTO_GLZ ] = "auto_glz",
> +[ SPICE_IMAGE_COMPRESSION_AUTO_LZ ] = "auto_lz",
> +[ SPICE_IMAGE_COMPRESSION_QUIC ] = "quic",
> +[ SPICE_IMAGE_COMPRESSION_GLZ ] = "glz",
> +[ SPICE_IMAGE_COMPRESSION_LZ ] = "lz",
> };
> #define parse_compression(_name)\
> parse_name(_name, "image compression", \
> @@ -643,7 +643,7 @@ void qemu_spice_init(void)
> *x509_cert_file = NULL,
> *x509_cacert_file = NULL;
> int port, tls_port, addr_flags;
> -spice_image_compression_t compression;
> +SpiceImageCompression compression;
> spice_wan_compression_t wan_compr;
> bool seamless_migration;
>
> @@ -754,7 +754,7 @@ void qemu_spice_init(void)
> #endif
> }
>
> -compression = SPICE_IMAGE_COMPRESS_AUTO_GLZ;
> +compression = SPICE_IMAGE_COMPRESSION_AUTO_GLZ;
> str = qemu_opt_get(opts, "image-compression");
> if (str) {
> compression = parse_compression(str);
> --
> 2.17.2
>
> ___
> Spice-devel mailing list
> spice-de...@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel
signature.asc
Description: PGP signature
[Qemu-devel] [PATCH] docs: Document -object tls-creds-x509 priority=xxx
This was added in 13f1243, but is missing from qemu-options.hx
Signed-off-by: Christophe Fergeau <cferg...@redhat.com>
---
qemu-options.hx | 11 ++-
1 file changed, 10 insertions(+), 1 deletion(-)
diff --git a/qemu-options.hx b/qemu-options.hx
index f11c4ac960..118784ceb7 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -4249,7 +4249,7 @@ expensive operation that consumes random pool entropy, so
it is
recommended that a persistent set of parameters be generated
upfront and saved.
-@item -object
tls-creds-x509,id=@var{id},endpoint=@var{endpoint},dir=@var{/path/to/cred/dir},verify-peer=@var{on|off},passwordid=@var{id}
+@item -object
tls-creds-x509,id=@var{id},endpoint=@var{endpoint},dir=@var{/path/to/cred/dir},priority=@var{priority},verify-peer=@var{on|off},passwordid=@var{id}
Creates a TLS anonymous credentials object, which can be used to provide
TLS support on network backends. The @option{id} parameter is a unique
@@ -4282,6 +4282,15 @@ version by providing the @var{passwordid} parameter.
This provides
the ID of a previously created @code{secret} object containing the
password for decryption.
+The @var{priority} parameter allows to override the global default
+priority used by gnutls. This can be useful if the system administrator
+needs to use a weaker set of crypto priorities for QEMU without
+potentially forcing the weakness onto all applications. Or conversely
+if one wants wants a stronger default for QEMU than for all other
+applications, they can do this through this parameter. Its format is
+a gnutls priority string as described at
+@url{https://gnutls.org/manual/html_node/Priority-Strings.html}.
+
@item -object
filter-buffer,id=@var{id},netdev=@var{netdevid},interval=@var{t}[,queue=@var{all|rx|tx}][,status=@var{on|off}]
Interval @var{t} can't be 0, this filter batches the packet delivery: all
--
2.14.3
[Qemu-devel] [PATCH v2] qxl: Only emit QXL_INTERRUPT_CLIENT_MONITORS_CONFIG on config changes
Currently if the client keeps sending the same monitor config to
QEMU/spice-server, QEMU will always raise
a QXL_INTERRUPT_CLIENT_MONITORS_CONFIG regardless of whether there was a
change or not.
Guest-side (with fedora 25), the kernel QXL KMS driver will also forward the
event to user-space without checking if there were actual changes.
Next in line are gnome-shell/mutter (on a default f25 install), which
will try to reconfigure everything without checking if there is anything
to do.
Where this gets ugly is that when applying the resolution changes,
gnome-shell/mutter will call drmModeRmFB, drmModeAddFB, and
drmModeSetCrtc, which will cause the primary surface to be destroyed and
recreated by the QXL KMS driver. This in turn will cause the client to
resend a client monitors config message, which will cause QEMU to reemit
an interrupt with an unchanged monitors configuration, ...
This causes https://bugzilla.redhat.com/show_bug.cgi?id=1266484
This commit makes sure that we only emit
QXL_INTERRUPT_CLIENT_MONITORS_CONFIG when there are actual configuration
changes the guest should act on.
Signed-off-by: Christophe Fergeau <cferg...@redhat.com>
---
Changes since v1:
- made sure checkpatch.pl passes
- moved the code to a helper function
hw/display/qxl.c | 37 -
1 file changed, 36 insertions(+), 1 deletion(-)
diff --git a/hw/display/qxl.c b/hw/display/qxl.c
index 0e2682d..62d0c80 100644
--- a/hw/display/qxl.c
+++ b/hw/display/qxl.c
@@ -992,6 +992,34 @@ static uint32_t qxl_crc32(const uint8_t *p, unsigned len)
return crc32(0x, p, len) ^ 0x;
}
+static bool qxl_rom_monitors_config_changed(QXLRom *rom,
+VDAgentMonitorsConfig *monitors_config,
+unsigned int max_outputs)
+{
+int i;
+unsigned int monitors_count;
+
+monitors_count = MIN(monitors_config->num_of_monitors, max_outputs);
+
+if (rom->client_monitors_config.count != monitors_count) {
+return true;
+}
+
+for (i = 0 ; i < rom->client_monitors_config.count ; ++i) {
+VDAgentMonConfig *monitor = _config->monitors[i];
+QXLURect *rect = >client_monitors_config.heads[i];
+/* monitor->depth ignored */
+if ((rect->left != monitor->x) ||
+(rect->top != monitor->y) ||
+(rect->right != monitor->x + monitor->width) ||
+(rect->bottom != monitor->y + monitor->height)) {
+return true;
+}
+}
+
+return false;
+}
+
/* called from main context only */
static int interface_client_monitors_config(QXLInstance *sin,
VDAgentMonitorsConfig *monitors_config)
@@ -1000,6 +1028,7 @@ static int interface_client_monitors_config(QXLInstance
*sin,
QXLRom *rom = memory_region_get_ram_ptr(>rom_bar);
int i;
unsigned max_outputs = ARRAY_SIZE(rom->client_monitors_config.heads);
+bool config_changed = false;
if (qxl->revision < 4) {
trace_qxl_client_monitors_config_unsupported_by_device(qxl->id,
@@ -1030,6 +1059,10 @@ static int interface_client_monitors_config(QXLInstance
*sin,
}
#endif
+config_changed = qxl_rom_monitors_config_changed(rom,
+ monitors_config,
+ max_outputs);
+
memset(>client_monitors_config, 0,
sizeof(rom->client_monitors_config));
rom->client_monitors_config.count = monitors_config->num_of_monitors;
@@ -1059,7 +1092,9 @@ static int interface_client_monitors_config(QXLInstance
*sin,
trace_qxl_interrupt_client_monitors_config(qxl->id,
rom->client_monitors_config.count,
rom->client_monitors_config.heads);
-qxl_send_events(qxl, QXL_INTERRUPT_CLIENT_MONITORS_CONFIG);
+if (config_changed) {
+qxl_send_events(qxl, QXL_INTERRUPT_CLIENT_MONITORS_CONFIG);
+}
return 1;
}
--
2.9.3
[Qemu-devel] [PATCH] qxl: Only emit QXL_INTERRUPT_CLIENT_MONITORS_CONFIG on config changes
Currently if the client keeps sending the same monitor config to
QEMU/spice-server, QEMU will always raise
a QXL_INTERRUPT_CLIENT_MONITORS_CONFIG regardless of whether there was a
change or not.
Guest-side (with fedora 25), the kernel QXL KMS driver will also forward the
event to user-space without checking if there were actual changes.
Next in line are gnome-shell/mutter (on a default f25 install), which
will try to reconfigure everything without checking if there is anything
to do.
Where this gets ugly is that when applying the resolution changes,
gnome-shell/mutter will call drmModeRmFB, drmModeAddFB, and
drmModeSetCrtc, which will cause the primary surface to be destroyed and
recreated by the QXL KMS driver. This in turn will cause the client to
resend a client monitors config message, which will cause QEMU to reemit
an interrupt with an unchanged monitors configuration, ...
This causes https://bugzilla.redhat.com/show_bug.cgi?id=1266484
This commit makes sure that we only emit
QXL_INTERRUPT_CLIENT_MONITORS_CONFIG when there are actual configuration
changes the guest should act on.
---
hw/display/qxl.c | 20 +++-
1 file changed, 19 insertions(+), 1 deletion(-)
diff --git a/hw/display/qxl.c b/hw/display/qxl.c
index 0e2682d..56759f8 100644
--- a/hw/display/qxl.c
+++ b/hw/display/qxl.c
@@ -1000,6 +1000,7 @@ static int interface_client_monitors_config(QXLInstance
*sin,
QXLRom *rom = memory_region_get_ram_ptr(>rom_bar);
int i;
unsigned max_outputs = ARRAY_SIZE(rom->client_monitors_config.heads);
+bool config_changed = false;
if (qxl->revision < 4) {
trace_qxl_client_monitors_config_unsupported_by_device(qxl->id,
@@ -1030,6 +1031,21 @@ static int interface_client_monitors_config(QXLInstance
*sin,
}
#endif
+if (rom->client_monitors_config.count !=
MIN(monitors_config->num_of_monitors, max_outputs)) {
+config_changed = true;
+}
+for (i = 0 ; i < rom->client_monitors_config.count ; ++i) {
+VDAgentMonConfig *monitor = _config->monitors[i];
+QXLURect *rect = >client_monitors_config.heads[i];
+/* monitor->depth ignored */
+if ((rect->left != monitor->x) ||
+(rect->top != monitor->y) ||
+(rect->right != monitor->x + monitor->width) ||
+(rect->bottom != monitor->y + monitor->height)) {
+config_changed = true;
+}
+}
+
memset(>client_monitors_config, 0,
sizeof(rom->client_monitors_config));
rom->client_monitors_config.count = monitors_config->num_of_monitors;
@@ -1059,7 +1075,9 @@ static int interface_client_monitors_config(QXLInstance
*sin,
trace_qxl_interrupt_client_monitors_config(qxl->id,
rom->client_monitors_config.count,
rom->client_monitors_config.heads);
-qxl_send_events(qxl, QXL_INTERRUPT_CLIENT_MONITORS_CONFIG);
+if (config_changed) {
+qxl_send_events(qxl, QXL_INTERRUPT_CLIENT_MONITORS_CONFIG);
+}
return 1;
}
--
2.9.3
Re: [Qemu-devel] [Spice-devel] [PATCH Qemu] Change spice-server protocol for GL texture passing
On Tue, Jul 19, 2016 at 09:41:22AM -0400, Frediano Ziglio wrote: > > I don't think we have strong reasons to support software encoding, video > > encoding is really expensive, and that mmap/copy is not going to be > > marginal, so even less these 2 syscalls. > > > > Using HW encoding is not easy at it seems: > - you have to have client supporting server HW encoders; > - you have to install additional software often closed source, accepting > patents; > - you have to have right permission on the system. > What are you doing if these option are not respected? Do not allow > connections? Showing blank screen? > With a good (local) connection I can easily play using software MJPEG, why > we should avoid such configurations? What we should be aiming/optimizing for is the hardware-accelerated case. We will need a fallback when this is not usable, but the various copies/encoding/... are going to be very expensive by themselves. Are these changes (passing texture rather than dmabuf) making a significant difference with software encoding? Christophe signature.asc Description: PGP signature
Re: [Qemu-devel] [PATCH] spice: Disallow use of gl + TCP port
On Fri, Mar 18, 2016 at 09:17:53AM +0100, Gerd Hoffmann wrote:
> On Mo, 2016-03-14 at 12:41 +0100, Christophe Fergeau wrote:
> > Currently, virgl support has to go through a local unix socket, trying
> > to connect to a VM using -spice gl through spice://localhost:5900 will
> > only result in a black screen.
> > This commit errors out when the user tries to start a VM with both GL
> > support and a port/tls-port set.
> > This would fit better in spice-server, but currently QEMU does not call
> > into spice-server when parsing 'gl' on its command line, so we have to
> > do this check in QEMU instead.
> >
> > Signed-off-by: Christophe Fergeau <cferg...@redhat.com>
>
> Picked up for ui patch queue.
Great thanks!
Fwiw, I've changed it locally to address Eric's comments, but did not
send it as v2 since a discussion was ongoing, here is what it looks in
my local clone now (but I'm fine with either version):
commit c0e10fb17a45a9da14d068a58af3a00e78f82403
Author: Christophe Fergeau <cferg...@redhat.com>
Date: Mon Mar 14 12:37:50 2016 +0100
spice: Disallow use of gl + TCP port
Currently, virgl support has to go through a local unix socket, trying
to connect to a VM using -spice gl through spice://localhost:5900 will
only result in a black screen.
This commit errors out when the user tries to start a VM with both GL
support and a port/tls-port set.
This would fit better in spice-server, but currently QEMU does not call
into spice-server when parsing 'gl' on its command line, so we have to
do this check in QEMU instead.
Signed-off-by: Christophe Fergeau <cferg...@redhat.com>
diff --git a/ui/spice-core.c b/ui/spice-core.c
index 7987a4e..83950fe 100644
--- a/ui/spice-core.c
+++ b/ui/spice-core.c
@@ -844,6 +844,10 @@ void qemu_spice_init(void)
#ifdef HAVE_SPICE_GL
if (qemu_opt_get_bool(opts, "gl", 0)) {
+if (port || tls_port) {
+error_report("SPICE GL support is local-only for now and
incompatible with -spice port/tls-port");
+exit(1);
+}
if (egl_rendernode_init() == 0) {
display_opengl = 1;
}
Christophe
signature.asc
Description: PGP signature
Re: [Qemu-devel] [PATCH] spice: Disallow use of gl + TCP port
On Tue, Mar 15, 2016 at 03:32:31PM +0100, Gerd Hoffmann wrote: > > We can do something similar once gl+tcp is available. > > I don't expect adding gl+tcp support to spice needs changes in the > spice-server API and qemu. So ifdef'ing on the spice-server version is > bogous, Hmm, I expected some changes, at least wrt options if the user needs to tweak the format of the video stream, hence the suggestion :) Things get trickier then. Adding a runtime spice_get_version() would not be that great either as the check would have to be updated with each spice-server release. I'd still like to have some failure when people try such configurations, gathering all the pieces is complicated enough, better to let people know when they try doing something that won't work. Christophe signature.asc Description: PGP signature
Re: [Qemu-devel] [PATCH] spice: Disallow use of gl + TCP port
On Tue, Mar 15, 2016 at 02:09:47PM +0100, Gerd Hoffmann wrote: > Hi, > > > Yes, long-term we will want to remove this once spice gets support for > > this. > > How can qemu figure whenever spice supports gl+tcp or not? gl support is already enabled through a spice-server version check #if defined(CONFIG_OPENGL_DMABUF) # if SPICE_SERVER_VERSION >= 0x000d01 /* release 0.13.1 */ # define HAVE_SPICE_GL 1 # include "ui/egl-helpers.h" # include "ui/egl-context.h" # endif #endif We can do something similar once gl+tcp is available. Christophe signature.asc Description: PGP signature
Re: [Qemu-devel] [PATCH] spice: Disallow use of gl + TCP port
On Mon, Mar 14, 2016 at 09:41:34AM -0600, Eric Blake wrote:
> On 03/14/2016 05:41 AM, Christophe Fergeau wrote:
> > Currently, virgl support has to go through a local unix socket, trying
> > to connect to a VM using -spice gl through spice://localhost:5900 will
> > only result in a black screen.
> > This commit errors out when the user tries to start a VM with both GL
> > support and a port/tls-port set.
> > This would fit better in spice-server, but currently QEMU does not call
> > into spice-server when parsing 'gl' on its command line, so we have to
> > do this check in QEMU instead.
> >
> > Signed-off-by: Christophe Fergeau <cferg...@redhat.com>
> > ---
> > ui/spice-core.c | 4
> > 1 file changed, 4 insertions(+)
> >
> > diff --git a/ui/spice-core.c b/ui/spice-core.c
> > index 7987a4e..94f3236 100644
> > --- a/ui/spice-core.c
> > +++ b/ui/spice-core.c
> > @@ -844,6 +844,10 @@ void qemu_spice_init(void)
> >
> > #ifdef HAVE_SPICE_GL
> > if (qemu_opt_get_bool(opts, "gl", 0)) {
> > +if ((port != 0) || (tls_port != 0)) {
>
> Overparenthesized; you could write:
>
> if (port || tls_port) {
>
> for the same effect with less typing.
Yeah I know I'm overly verbose with these tests, the parentheses make it
explicit that there are no operator priority issues, the != 0 emphasize
it's an integer type which is being handled.
I'll change it to your recommendation before sending a v2.
Christophe
signature.asc
Description: PGP signature
Re: [Qemu-devel] [PATCH] spice: Disallow use of gl + TCP port
Hi, On Mon, Mar 14, 2016 at 04:00:11PM +0100, Gerd Hoffmann wrote: > On Mo, 2016-03-14 at 12:41 +0100, Christophe Fergeau wrote: > > Currently, virgl support has to go through a local unix socket, trying > > to connect to a VM using -spice gl through spice://localhost:5900 will > > only result in a black screen. > > This commit errors out when the user tries to start a VM with both GL > > support and a port/tls-port set. > > This would fit better in spice-server, but currently QEMU does not call > > into spice-server when parsing 'gl' on its command line, so we have to > > do this check in QEMU instead. > > Hmm. It's something which we want support long-term though, by encoding > those dma-bufs as video stream and send them off over tcp. > > I don't think this is a good idea long-term. Yes, long-term we will want to remove this once spice gets support for this. Currently however, this imo makes it a bit easier to understand how everything is setup. Otherwise I expect people are going to just take their existing SPICE libvirt configuration listening on the network, add to it, try remote-viewer spice://localhost:5900 and wonder why they get a black screen, and not know whether this is because their guest does not support virgl, or their host, or because of some other issue, ... Having this as a stopgap ensures that they at least be informed this is not a valid usecase. > And even as temporary stopgap: Can libvirt + virt-manager handle this? libvirt can, I haven't tried virt-manager (I believe all the patches from Marc-André haven't been pushed yet) starts QEMU with -spice port=0 Christophe signature.asc Description: PGP signature
[Qemu-devel] [PATCH] spice: Disallow use of gl + TCP port
Currently, virgl support has to go through a local unix socket, trying
to connect to a VM using -spice gl through spice://localhost:5900 will
only result in a black screen.
This commit errors out when the user tries to start a VM with both GL
support and a port/tls-port set.
This would fit better in spice-server, but currently QEMU does not call
into spice-server when parsing 'gl' on its command line, so we have to
do this check in QEMU instead.
Signed-off-by: Christophe Fergeau <cferg...@redhat.com>
---
ui/spice-core.c | 4
1 file changed, 4 insertions(+)
diff --git a/ui/spice-core.c b/ui/spice-core.c
index 7987a4e..94f3236 100644
--- a/ui/spice-core.c
+++ b/ui/spice-core.c
@@ -844,6 +844,10 @@ void qemu_spice_init(void)
#ifdef HAVE_SPICE_GL
if (qemu_opt_get_bool(opts, "gl", 0)) {
+if ((port != 0) || (tls_port != 0)) {
+error_report("SPICE GL support is local-only for now and
incompatible with -spice port/tls-port");
+exit(1);
+}
if (egl_rendernode_init() == 0) {
display_opengl = 1;
}
--
2.5.0
Re: [Qemu-devel] [PATCH] man: virtfs-proxy-helper: Fix 'capbilities' typo
On Fri, Jan 15, 2016 at 04:31:45PM -0700, Eric Blake wrote: > On 01/12/2016 06:39 AM, Christophe Fergeau wrote: > > Signed-off-by: Christophe Fergeau <cferg...@redhat.com> > > --- > > fsdev/virtfs-proxy-helper.texi | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/fsdev/virtfs-proxy-helper.texi b/fsdev/virtfs-proxy-helper.texi > > index e60e3b9..1b905a0 100644 > > --- a/fsdev/virtfs-proxy-helper.texi > > +++ b/fsdev/virtfs-proxy-helper.texi > > @@ -29,7 +29,7 @@ driver sends filesystem request to proxy helper and > > receives the > > response from it. > > > > Proxy helper is designed so that it can drop the root privilege with > > -retaining capbilities needed for doing filesystem operations only. > > +retaining capabilities needed for doing filesystem operations only. > > Still reads awkwardly; better might be: > > The proxy helper is designed so that it can drop root privileges except > for the capabilities needed for doing filesystem operations. Ok, I'll send a v2 changing the sentence to your suggestion, thanks! Christophe signature.asc Description: PGP signature
[Qemu-devel] [PATCH v2] man: virtfs-proxy-helper: Rework awkward sentence
There was a 'capbilities' typo in this man page. This commit reformulates the sentence the typo was in to make it easier to grasp. This is based on a suggestion from Eric Blake. Signed-off-by: Christophe Fergeau <cferg...@redhat.com> --- Changes since v1: - rather than just fixing the typo, reformulate the sentence it's in as suggested by Eric. fsdev/virtfs-proxy-helper.texi | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/fsdev/virtfs-proxy-helper.texi b/fsdev/virtfs-proxy-helper.texi index e60e3b9..a2f4ecc 100644 --- a/fsdev/virtfs-proxy-helper.texi +++ b/fsdev/virtfs-proxy-helper.texi @@ -28,8 +28,8 @@ QEMU and proxy helper communicate using this socket. QEMU proxy fs driver sends filesystem request to proxy helper and receives the response from it. -Proxy helper is designed so that it can drop the root privilege with -retaining capbilities needed for doing filesystem operations only. +The proxy helper is designed so that it can drop root privileges except +for the capabilities needed for doing filesystem operations. @end table @c man end -- 2.5.0
Re: [Qemu-devel] [PATCH] Fix corner-case when using VNC+SASL+SPICE
On Mon, Jan 18, 2016 at 11:11:42AM +0100, Gerd Hoffmann wrote: > On Fr, 2016-01-15 at 16:33 -0700, Eric Blake wrote: > > > This commit unconditionnally calls spice_server_set_sasl_appname() > > > > s/unconditionnally/unconditionally/ > > Fixed up, no need to resend. Thanks! Christophe signature.asc Description: PGP signature
[Qemu-devel] [PATCH] man: virtfs-proxy-helper: Fix 'capbilities' typo
Signed-off-by: Christophe Fergeau <cferg...@redhat.com> --- fsdev/virtfs-proxy-helper.texi | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/fsdev/virtfs-proxy-helper.texi b/fsdev/virtfs-proxy-helper.texi index e60e3b9..1b905a0 100644 --- a/fsdev/virtfs-proxy-helper.texi +++ b/fsdev/virtfs-proxy-helper.texi @@ -29,7 +29,7 @@ driver sends filesystem request to proxy helper and receives the response from it. Proxy helper is designed so that it can drop the root privilege with -retaining capbilities needed for doing filesystem operations only. +retaining capabilities needed for doing filesystem operations only. @end table @c man end -- 2.5.0
[Qemu-devel] [PATCH] Fix corner-case when using VNC+SASL+SPICE
Similarly to the commit 764eb39d1b6 fixing VNC+SASL+QXL, when starting
QEMU with SPICE but no SASL, and at the same time VNC with SASL, then
spice_server_init() will get called without a previous call to
spice_server_set_sasl_appname(), which will cause cyrus-sasl to
try to use /etc/sasl2/spice.conf (spice-server uses "spice" as its
default appname) rather than the expected /etc/sasl2/qemu.conf.
This commit unconditionnally calls spice_server_set_sasl_appname()
before calling spice_server_init() in order to use the correct appname
even if SPICE without SASL was requested on qemu command line.
Signed-off-by: Christophe Fergeau <cferg...@redhat.com>
---
ui/spice-core.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/ui/spice-core.c b/ui/spice-core.c
index 3322bf2..77b209f 100644
--- a/ui/spice-core.c
+++ b/ui/spice-core.c
@@ -743,8 +743,7 @@ void qemu_spice_init(void)
qemu_spice_set_passwd(password, false, false);
}
if (qemu_opt_get_bool(opts, "sasl", 0)) {
-if (spice_server_set_sasl_appname(spice_server, "qemu") == -1 ||
-spice_server_set_sasl(spice_server, 1) == -1) {
+if (spice_server_set_sasl(spice_server, 1) == -1) {
error_report("spice: failed to enable sasl");
exit(1);
}
@@ -810,6 +809,7 @@ void qemu_spice_init(void)
seamless_migration = qemu_opt_get_bool(opts, "seamless-migration", 0);
spice_server_set_seamless_migration(spice_server, seamless_migration);
+spice_server_set_sasl_appname(spice_server, "qemu");
if (spice_server_init(spice_server, _interface) != 0) {
error_report("failed to initialize spice server");
exit(1);
--
2.5.0
[Qemu-devel] [PATCH v3] spice: Allow to set password even if disable-ticketing was used
Before commit b1ea7b79e1, it was possible to start with -spice
disable-ticketing, and then use the "set_password spice" command to
enable ticketing with SPICE. Since commit b1ea7b79e1 this is no longer
possible as qemu_spice_set_ticket() will return an error unless the
'auth' type is "spice". When ticketing is disabled, 'auth' is "none" so
the attempt to set password fails.
This change of behaviour caused a bug in oVirt
https://gerrit.ovirt.org/#/c/44842/
This commit allows to call qemu_spice_set_ticket() when 'auth' is "none"
and changes 'auth' to "spice" when this happens.
Signed-off-by: Christophe Fergeau <cferg...@redhat.com>
Reviewed-by: Daniel P. Berrange <berra...@redhat.com>
---
Changes since v2:
- Added mention of the oVirt bug which was caused by the change of behaviour
ui/spice-core.c | 4
1 file changed, 4 insertions(+)
diff --git a/ui/spice-core.c b/ui/spice-core.c
index 4da3042..3b20c6c 100644
--- a/ui/spice-core.c
+++ b/ui/spice-core.c
@@ -882,6 +882,10 @@ static int qemu_spice_set_ticket(bool fail_if_conn, bool
disconnect_if_conn)
int qemu_spice_set_passwd(const char *passwd,
bool fail_if_conn, bool disconnect_if_conn)
{
+if (strcmp(auth, "none") == 0) {
+/* Allow to set a password when started with 'disable-ticketing' */
+auth = "spice";
+}
if (strcmp(auth, "spice") != 0) {
return -1;
}
--
2.5.0
Re: [Qemu-devel] [PATCH v3] spice: Allow to set password even if disable-ticketing was used
Hey Gerd, On Mon, Oct 12, 2015 at 03:43:51PM +0200, Gerd Hoffmann wrote: > On Mo, 2015-10-12 at 13:25 +0200, Christophe Fergeau wrote: > > Before commit b1ea7b79e1, it was possible to start with -spice > > disable-ticketing, and then use the "set_password spice" command to > > enable ticketing with SPICE. Since commit b1ea7b79e1 this is no longer > > possible as qemu_spice_set_ticket() will return an error unless the > > 'auth' type is "spice". When ticketing is disabled, 'auth' is "none" so > > the attempt to set password fails. > > Huh? And this actually worked? i.e. spice_server_set_ticket() has an > effect after spice_server_set_noauth() was called? Yes, this works on the spice server side. > > > This change of behaviour caused a bug in oVirt > > https://gerrit.ovirt.org/#/c/44842/ > > Hmm, I'd say fix this in ovirt then [1]. Fair enough (I believe this is already fixed in newer versions). > > If you want run with spice authentication, then say so when starting > qemu. Switching authentication methods as side-effect of setting the > password is asking for trouble. We had that with vnc. We finally got > rid of it a while ago. I don't feel like opening that can of worms > again. > > Also it encourages bad security practice. If you turn on password auth > as side effect of setting the password there is a window where one can > access the virtual machine without a password, which probably is not > what you want. > > If there is an actual use case where switching authentication methods at > runtime is needed we can discuss that. But we'll be doing that as > explicit monitor command, not as side-effect of something else. Yeah, I'm not saying this patch is a good idea. However, there was a silent change (by reading the commit log which introduced it, this change is not mentioned at all, so may have been unintentional) in behaviour in QEMU, and this change causes breakage in existing apps using QEMU. This patch only attempts to fix this regression, I'm not saying one behaviour or the other is better. Christophe signature.asc Description: PGP signature
Re: [Qemu-devel] [PATCH v2] spice: Allow to set password even if disable-ticketing was used
Hey,
On Fri, Aug 14, 2015 at 05:10:57PM +0200, Christophe Fergeau wrote:
> Before commit b1ea7b79e1, it was possible to start with -spice
> disable-ticketing, and then use the "set_password spice" command to
> enable ticketing with SPICE. Since commit b1ea7b79e1 this is no longer
> possible as qemu_spice_set_ticket() will return an error unless the
> 'auth' type is "spice". When ticketing is disabled, 'auth' is "none" so
> the attempt to set password fails.
>
> This commit allows to call qemu_spice_set_ticket() when 'auth' is "none"
> and changes 'auth' to "spice" when this happens.
>
> Signed-off-by: Christophe Fergeau <cferg...@redhat.com>
> Reviewed-by: Daniel P. Berrange <berra...@redhat.com>
Any taker for that patch?
Thanks,
Christophe
> ---
> Changes since v1:
> - added Reviewed-by and missing Signed-off-by tags
>
>
> ui/spice-core.c | 4
> 1 file changed, 4 insertions(+)
>
> diff --git a/ui/spice-core.c b/ui/spice-core.c
> index 4da3042..3b20c6c 100644
> --- a/ui/spice-core.c
> +++ b/ui/spice-core.c
> @@ -882,6 +882,10 @@ static int qemu_spice_set_ticket(bool fail_if_conn, bool
> disconnect_if_conn)
> int qemu_spice_set_passwd(const char *passwd,
>bool fail_if_conn, bool disconnect_if_conn)
> {
> +if (strcmp(auth, "none") == 0) {
> +/* Allow to set a password when started with 'disable-ticketing' */
> +auth = "spice";
> +}
> if (strcmp(auth, "spice") != 0) {
> return -1;
> }
> --
> 2.4.3
>
>
signature.asc
Description: PGP signature
Re: [Qemu-devel] [PATCH] spice: Allow to set password even if disable-ticketing was used
On Fri, Aug 14, 2015 at 03:04:48PM +0100, Daniel P. Berrange wrote: Hmm, is oVirt using this via libvirt ? If so, I guess we have to fix it, as that would be a break in current usage. Yes this is done through libvirt. Before commit qemu-2.1.0-rc2~11^2, you could use virsh update-device with graphics type='spice' autoport='yes' listen='127.0.0.1' passwd='bar'/ to set the password for a running domain whose graphics node is graphics type='spice' autoport='yes' listen='127.0.0.1'/ After qemu-2.1.0-rc2~11^2, this results in an error. Christophe pgpTeNIyrUMds.pgp Description: PGP signature
[Qemu-devel] [PATCH v2] spice: Allow to set password even if disable-ticketing was used
Before commit b1ea7b79e1, it was possible to start with -spice
disable-ticketing, and then use the set_password spice command to
enable ticketing with SPICE. Since commit b1ea7b79e1 this is no longer
possible as qemu_spice_set_ticket() will return an error unless the
'auth' type is spice. When ticketing is disabled, 'auth' is none so
the attempt to set password fails.
This commit allows to call qemu_spice_set_ticket() when 'auth' is none
and changes 'auth' to spice when this happens.
Signed-off-by: Christophe Fergeau cferg...@redhat.com
Reviewed-by: Daniel P. Berrange berra...@redhat.com
---
Changes since v1:
- added Reviewed-by and missing Signed-off-by tags
ui/spice-core.c | 4
1 file changed, 4 insertions(+)
diff --git a/ui/spice-core.c b/ui/spice-core.c
index 4da3042..3b20c6c 100644
--- a/ui/spice-core.c
+++ b/ui/spice-core.c
@@ -882,6 +882,10 @@ static int qemu_spice_set_ticket(bool fail_if_conn, bool
disconnect_if_conn)
int qemu_spice_set_passwd(const char *passwd,
bool fail_if_conn, bool disconnect_if_conn)
{
+if (strcmp(auth, none) == 0) {
+/* Allow to set a password when started with 'disable-ticketing' */
+auth = spice;
+}
if (strcmp(auth, spice) != 0) {
return -1;
}
--
2.4.3
Re: [Qemu-devel] [PATCH] spice: Allow to set password even if disable-ticketing was used
On Fri, Aug 14, 2015 at 03:35:21PM +0100, Daniel P. Berrange wrote: On Fri, Aug 14, 2015 at 02:47:15PM +0200, Christophe Fergeau wrote: Before commit b1ea7b79e1, it was possible to start with -spice disable-ticketing, and then use the set_password spice command to enable ticketing with SPICE. Since commit b1ea7b79e1 this is no longer possible as qemu_spice_set_ticket() will return an error unless the 'auth' type is spice. When ticketing is disabled, 'auth' is none so the attempt to set password fails. This commit allows to call qemu_spice_set_ticket() when 'auth' is none and changes 'auth' to spice when this happens. BTW, you need to have a Signed-of-by here Ah right, thanks, just sent a v2 with this added. Christophe pgpgfhbJpuzx3.pgp Description: PGP signature
[Qemu-devel] [PATCH] spice: Allow to set password even if disable-ticketing was used
Before commit b1ea7b79e1, it was possible to start with -spice
disable-ticketing, and then use the set_password spice command to
enable ticketing with SPICE. Since commit b1ea7b79e1 this is no longer
possible as qemu_spice_set_ticket() will return an error unless the
'auth' type is spice. When ticketing is disabled, 'auth' is none so
the attempt to set password fails.
This commit allows to call qemu_spice_set_ticket() when 'auth' is none
and changes 'auth' to spice when this happens.
---
ui/spice-core.c | 4
1 file changed, 4 insertions(+)
diff --git a/ui/spice-core.c b/ui/spice-core.c
index 4da3042..3b20c6c 100644
--- a/ui/spice-core.c
+++ b/ui/spice-core.c
@@ -882,6 +882,10 @@ static int qemu_spice_set_ticket(bool fail_if_conn, bool
disconnect_if_conn)
int qemu_spice_set_passwd(const char *passwd,
bool fail_if_conn, bool disconnect_if_conn)
{
+if (strcmp(auth, none) == 0) {
+/* Allow to set a password when started with 'disable-ticketing' */
+auth = spice;
+}
if (strcmp(auth, spice) != 0) {
return -1;
}
--
2.4.3
Re: [Qemu-devel] [PATCH] spice: Allow to set password even if disable-ticketing was used
Hey, On Fri, Aug 14, 2015 at 01:54:59PM +0100, Daniel P. Berrange wrote: On Fri, Aug 14, 2015 at 02:47:15PM +0200, Christophe Fergeau wrote: Before commit b1ea7b79e1, it was possible to start with -spice disable-ticketing, and then use the set_password spice command to enable ticketing with SPICE. Since commit b1ea7b79e1 this is no longer possible as qemu_spice_set_ticket() will return an error unless the 'auth' type is spice. When ticketing is disabled, 'auth' is none so the attempt to set password fails. This commit allows to call qemu_spice_set_ticket() when 'auth' is none and changes 'auth' to spice when this happens. IMHO we should not be changing the authentication method as a side effect of trying to set the password. If app has disabled ticketing, it should remain disabled and the set password call is right to return an error. In general I agree with you. However in this case, this used to be working until ~1 year ago, and this change of behaviour caused a bug in oVirt (oVirt side is being fixed). This is why I sent this patch. The intent of commit b1ea7b seems to be to prevent qemu_spice_set_passwd() from being called when SASL is used, and does not mention at all whether preventing going from auth being none to spice is intentional. If this change of behaviour was an intentional bug fix, and if we are fine with asking for oVirt changes for this, then I'm ok with dropping this patch. Christophe pgpgri2ftwR_O.pgp Description: PGP signature
Re: [Qemu-devel] [PATCH] qxl: Fix new function name for spice-server library
On Mon, Jul 20, 2015 at 09:43:23AM +0100, Frediano Ziglio wrote:
The new spice-server function to limit the number of monitors (0.12.6)
changed while development from spice_qxl_set_monitors_config_limit to
spice_qxl_max_monitors (accepted upstream).
By mistake I post patch with former name.
This patch fix the function name.
Signed-off-by: Frediano Ziglio fzig...@redhat.com
---
hw/display/qxl.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
I tested again doing a clean build, unfortunately I did some mistake
and my tests worked.
diff --git a/hw/display/qxl.c b/hw/display/qxl.c
index 4e5ff69..2288238 100644
--- a/hw/display/qxl.c
+++ b/hw/display/qxl.c
@@ -273,8 +273,7 @@ static void qxl_spice_monitors_config_async(PCIQXLDevice
*qxl, int replay)
} else {
#if SPICE_SERVER_VERSION = 0x000c06 /* release 0.12.6 */
if (qxl-max_outputs) {
-spice_qxl_set_monitors_config_limit(qxl-ssd.qxl,
-qxl-max_outputs);
+spice_qxl_set_max_monitors(qxl-ssd.qxl, qxl-max_outputs);
}
#endif
qxl-guest_monitors_config = qxl-ram-monitors_config;
ACK from me,
Christophe
pgpmeQQ6uSTL7.pgp
Description: PGP signature
Re: [Qemu-devel] [Spice-devel] [RFC PATCH v2] qxl: allows to specify head limit to qxl driver
On Fri, Jun 12, 2015 at 03:05:10PM +0100, Frediano Ziglio wrote:
This patch allow to limit number of heads using qxl driver. By default
qxl driver is not limited on any kind on head use so can decide to use
as much heads.
libvirt has this as a video card parameter (actually set to 1 but not
used). This parameter will allow to limit setting a use can do (which
could be confusing).
This patch rely on some change in spice-protocol which are not still
accepted. See
http://lists.freedesktop.org/archives/spice-devel/2015-June/020221.html.
Signed-off-by: Frediano Ziglio fzig...@redhat.com
---
hw/display/qxl.c | 26 +-
hw/display/qxl.h | 3 +++
2 files changed, 24 insertions(+), 5 deletions(-)
Change from v1:
- check spice-server version.
diff --git a/hw/display/qxl.c b/hw/display/qxl.c
index b220e2d..d6e9369 100644
--- a/hw/display/qxl.c
+++ b/hw/display/qxl.c
@@ -272,6 +272,12 @@ static void qxl_spice_monitors_config_async(PCIQXLDevice
*qxl, int replay)
QXL_COOKIE_TYPE_POST_LOAD_MONITORS_CONFIG,
0));
} else {
+#if SPICE_SERVER_VERSION = 0x000c06 /* release 0.12.6 */
+if (qxl-max_outputs) {
+spice_qxl_set_monitors_config_limit(qxl-ssd.qxl,
+qxl-max_outputs);
+}
+#endif
qxl-guest_monitors_config = qxl-ram-monitors_config;
spice_qxl_monitors_config_async(qxl-ssd.qxl,
qxl-ram-monitors_config,
@@ -992,6 +998,7 @@ static int interface_client_monitors_config(QXLInstance
*sin,
PCIQXLDevice *qxl = container_of(sin, PCIQXLDevice, ssd.qxl);
QXLRom *rom = memory_region_get_ram_ptr(qxl-rom_bar);
int i;
+unsigned max_outputs = ARRAY_SIZE(rom-client_monitors_config.heads);
if (qxl-revision 4) {
trace_qxl_client_monitors_config_unsupported_by_device(qxl-id,
@@ -1014,17 +1021,23 @@ static int
interface_client_monitors_config(QXLInstance *sin,
if (!monitors_config) {
return 1;
}
+
+#if SPICE_SERVER_VERSION = 0x000c06 /* release 0.12.6 */
+/* limit number of outputs based on setting limit */
« based on 'max_outputs' command line parameter » maybe ?
Patch looks good to me.
Christophe
pgpyhpVkMRU5R.pgp
Description: PGP signature
Re: [Qemu-devel] [Spice-devel] [PATCH] RFC: qxl: allow to specify head limit to qxl driver
On Tue, Jun 09, 2015 at 10:50:48AM +0100, Daniel P. Berrange wrote: On Tue, Jun 09, 2015 at 09:49:44AM +0100, Frediano Ziglio wrote: This patch allow to limit number of heads using qxl driver. By default qxl driver is not limited on any kind on head use so can decide to use as much heads. libvirt has this as a video card parameter (actually set to 1 but not used). This parameter will allow to limit setting a use can do (which could be confusing). This patch rely on some change in spice-protocol which are not still accepted. See http://lists.freedesktop.org/archives/spice-devel/2015-June/020160.html. Main question and stop over are parameter name. Consider that this parameter is actually more a hint to drivers. I'm looking anyway to a way to enforce this in spice-server. What is the actual benefit of being able to limit the number of heads in QXL / SPICE ? I know libvirt has the 'heads' attribute for specifying the number of video outputs. This is used in hypervisors which only support a fixed number of outputs and need up-front configuration. Since QXL/SPICE can dynamically enable/disable heads, there is no need to support this libvirt configuration attribute - it is better from a usability POV to have the head count totally dynamic, than to add a fixed limit. IOW, unless there's some need to use this limit in order to go above the 16 head maximum the code currently has, I think adding this manual limit to QXL/SPICE is a step backwards really. It feels like a feature in search of a purpose IMHO. The head count is indeed dynamic with SPICE, but the available video RAM is fixed after QEMU startup, and if you start trying to fit 4 full HD heads in 16MB of VRAM, you'll get weird failures. If you know you are only going to use one screen, it's nice to be able to allocate a small amount of VRAM and to enforce the maximum number of heads for the amount of VRAM you have assigned to the VM. remote-viewer also gets a 'display' menu with the number of available displays listed in it. I can imagine some people/admins wanted to mandate that only N displays are going to be shown here to avoid user confusion. So while we should keep defaulting to the current dynamic behaviour, a way to optionally limit the number of available heads is useful in some situations Christophe pgpjvpeqKG6KQ.pgp Description: PGP signature
Re: [Qemu-devel] [Spice-devel] [PATCH] spice: fix simple display on bigendian hosts
Hey, On Tue, Apr 14, 2015 at 09:14:53AM +0200, Gerd Hoffmann wrote: Denis Kirjanov is busy getting spice run on ppc64 and trapped into this one. Spice wire format is little endian, so we have to explicitly say we want little endian when letting pixman convert the data for us. This patch looks good to me, would be nice to get some testing from Denis first though. Christophe pgp7nToNGssqB.pgp Description: PGP signature
Re: [Qemu-devel] [Spice-devel] [PATCH] [RFC] LZ4 compression option for SPICE
Hey, On Thu, Jan 08, 2015 at 11:50:13AM +0100, Javier Celaya wrote: Hello Recently, SPICE included the lz4 compression algorithm. This patch adds a command line option to select it. However, SPICE_IMAGE_COMPRESS_LZ4 did not exist before the commit that added this compression algorithm, so it should be guarded with conditional compilation. How do you think this should be done? Wait for the next stable version of spice-server and check for SPICE_SERVER_VERSION? Or add a specific flag? Version check seems good, we probably can raise spice-server version in git preemptively so that you can have a working version check now. Given that lz4 support is optional, spice_server_set_image_compression should probably error out if we try to set lz4 but it's not supported. Christophe pgpsFFLRNycov.pgp Description: PGP signature
Re: [Qemu-devel] [PATCH 3/5] vscclient: use glib thread primitives not qemu
Hey, On Tue, Apr 29, 2014 at 10:02:26AM +0400, Michael Tokarev wrote: Use glib-provided thread primitives in vscclient, not qemu thread primitives. This way, vscclient becomes more stand-alone. For what it's worth, this patch has a few non-threading related bits in it because of the removal of #include qemu/sockets.h I guess.
Re: [Qemu-devel] [PATCH 1/5] do not call g_thread_init() for glib = 2.31
On Tue, Apr 29, 2014 at 10:02:24AM +0400, Michael Tokarev wrote:
glib = 2.31 always enables thread support and g_thread_supported()
is #defined to 1, there's no need to call g_thread_init() anymore,
and it definitely does not need to report error which never happens.
Keep code for old 2.31 glibc anyway for now, just #ifdef it
differently.
This looks good to me, ACK
Signed-off-by: Michael Tokarev m...@tls.msk.ru
---
coroutine-gthread.c |7 ++-
util/osdep.c| 21 +
2 files changed, 11 insertions(+), 17 deletions(-)
diff --git a/coroutine-gthread.c b/coroutine-gthread.c
index d3e5b99..a61efe0 100644
--- a/coroutine-gthread.c
+++ b/coroutine-gthread.c
@@ -115,14 +115,11 @@ static inline GThread *create_thread(GThreadFunc func,
gpointer data)
static void __attribute__((constructor)) coroutine_init(void)
{
-if (!g_thread_supported()) {
#if !GLIB_CHECK_VERSION(2, 31, 0)
+if (!g_thread_supported()) {
g_thread_init(NULL);
-#else
-fprintf(stderr, glib threading failed to initialize.\n);
-exit(1);
-#endif
}
+#endif
init_coroutine_cond();
}
diff --git a/util/osdep.c b/util/osdep.c
index a9029f8..b2bd154 100644
--- a/util/osdep.c
+++ b/util/osdep.c
@@ -436,23 +436,20 @@ int socket_init(void)
return 0;
}
-/* Ensure that glib is running in multi-threaded mode */
+#if !GLIB_CHECK_VERSION(2, 31, 0)
+/* Ensure that glib is running in multi-threaded mode
+ * Old versions of glib require explicit initialization. Failure to do
+ * this results in the single-threaded code paths being taken inside
+ * glib. For example, the g_slice allocator will not be thread-safe
+ * and cause crashes.
+ */
static void __attribute__((constructor)) thread_init(void)
{
if (!g_thread_supported()) {
-#if !GLIB_CHECK_VERSION(2, 31, 0)
-/* Old versions of glib require explicit initialization. Failure to
do
- * this results in the single-threaded code paths being taken inside
- * glib. For example, the g_slice allocator will not be thread-safe
- * and cause crashes.
- */
-g_thread_init(NULL);
-#else
-fprintf(stderr, glib threading failed to initialize.\n);
-exit(1);
-#endif
+ g_thread_init(NULL);
}
}
+#endif
#ifndef CONFIG_IOVEC
/* helper function for iov_send_recv() */
--
1.7.10.4
Re: [Qemu-devel] [PATCH 4/5] libcacard: replace qemu thread primitives with glib ones
Hey, patch looks good On Tue, Apr 29, 2014 at 10:02:27AM +0400, Michael Tokarev wrote: Replace QemuMutex with GMutex and QemuCond with GCond (with corresponding function changes), to make libcacard independent of qemu internal functions. Also replace single instance pstrcpy() in vcard_emul_nss.c to strncpy(). This reverts commit 2e679780ae86c6ca8. An alternative would be to use g_strlcpy which guarantees nul-termination. Christophe
Re: [Qemu-devel] [Spice-devel] Automatic spice port selection
- Mail original - Il 26/03/2014 17:15, Fabio Fantoni ha scritto: Time ago I have read somewhere that there is an option to automatically spice port in qemu as for vnc. I started to write a libxl patch to add this feature like the vnc one: https://github.com/Fantu/Xen/commit/63c54f354a34e7eb27b1c69016789a372a75843c Testing this patch lead to a missing to= parameter and I didn't found nothing else. Is there someone that can tell me that if it is possible to let qemu automatically assign a spice port like for vnc? libvirt does automatic port assignemnt for SPICE, but I think it does it 'manually' (with no support from SPICE). I don't think spice-server/qemu has magic to automatically allocate the spice port to be used. Christophe
Re: [Qemu-devel] [Spice-devel] [Xen-devel] Qemu 2.0 regression with xen: qemu crash on any domUs S.O. start
On Mon, Apr 07, 2014 at 11:59:06AM +0200, Fabio Fantoni wrote:
Today I did some tests also with hvm and spice and I found another
segfault with different backtrace to solve:
(gdb) c
Continuing.
*Program received signal SIGSEGV, Segmentation fault.**
**0x55855d30 in interface_client_monitors_config
(sin=0x563b0260, **
**mc=0x0) at ui/spice-display.c:557**
**557 if (mc-num_of_monitors 0) {*
(gdb) bt full
#0 0x55855d30 in interface_client_monitors_config (
sin=0x563b0260, mc=0x0) at ui/spice-display.c:557
ssd = 0x563b0210
info = {xoff = 0, yoff = 0, width = 0, height = 0}
rc = 32767
__func__ = interface_client_monitors_config
#1 0x74af5113 in ?? ()
from /usr/lib/x86_64-linux-gnu/libspice-server.so.1
No symbol table info available.
A backtrace with spice-server debugging symbols installed would be helpful.
Christophe
pgpLb9qHAjCEt.pgp
Description: PGP signature
Re: [Qemu-devel] [Spice-devel] [PATCH] qxl: add sanity check
Looks good, ACK.
Christophe
On Wed, Feb 19, 2014 at 11:40:50AM +0100, Gerd Hoffmann wrote:
Signed-off-by: Gerd Hoffmann kra...@redhat.com
---
hw/display/qxl.c | 8 +++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/hw/display/qxl.c b/hw/display/qxl.c
index 1471cc0..2a559eb 100644
--- a/hw/display/qxl.c
+++ b/hw/display/qxl.c
@@ -1429,7 +1429,7 @@ static int qxl_destroy_primary(PCIQXLDevice *d,
qxl_async_io async)
return 1;
}
-static void qxl_set_mode(PCIQXLDevice *d, int modenr, int loadvm)
+static void qxl_set_mode(PCIQXLDevice *d, unsigned int modenr, int loadvm)
{
pcibus_t start = d-pci.io_regions[QXL_RAM_RANGE_INDEX].addr;
pcibus_t end = d-pci.io_regions[QXL_RAM_RANGE_INDEX].size + start;
@@ -1439,6 +1439,12 @@ static void qxl_set_mode(PCIQXLDevice *d, int modenr,
int loadvm)
.mem_start = start,
.mem_end = end
};
+
+if (modenr = d-modes-n_modes) {
+qxl_set_guest_bug(d, mode number out of range);
+return;
+}
+
QXLSurfaceCreate surface = {
.width = mode-x_res,
.height = mode-y_res,
--
1.8.3.1
___
Spice-devel mailing list
spice-de...@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel
pgp2rpgihI9gl.pgp
Description: PGP signature
[Qemu-devel] [PATCH] libcacard: Don't link with all libraries QEMU links to
As described in https://bugzilla.redhat.com/show_bug.cgi?id=987441 , libcacard currently links to all the libraries QEMU is linking to, including glusterfs libraries, libiscsi, ... libcacard does not need all of these. This patch ensures it's only linked with the libraries it needs. Signed-off-by: Christophe Fergeau cferg...@redhat.com --- libcacard/Makefile | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/libcacard/Makefile b/libcacard/Makefile index 4d15da4..6b06448 100644 --- a/libcacard/Makefile +++ b/libcacard/Makefile @@ -25,7 +25,7 @@ vscclient$(EXESUF): libcacard/vscclient.o libcacard.la libcacard.la: LDFLAGS += -rpath $(libdir) -no-undefined \ -export-syms $(SRC_PATH)/libcacard/libcacard.syms -libcacard.la: LIBS += $(libcacard_libs) +libcacard.la: LIBS = $(libcacard_libs) libcacard.la: $(libcacard-lobj-y) $(call LINK,$^) -- 1.8.5.3
[Qemu-devel] [PATCH RESEND] Fix another corner-case of using VNC+SASL+SPICE
Similarly to the previous commit fixing VNC+SASL+QXL, when starting
QEMU with SPICE but no SASL, and also VNC with SASL, then
spice_server_init() will get called without a previous call to
spice_server_set_sasl_appname(), which will cause cyrus-sasl to
try to use /etc/sasl2/spice.conf (spice-server uses spice as its
default appname) rather than the expected /etc/sasl2/qemu.conf.
This commit unconditionnally calls spice_server_set_sasl_appname()
before calling spice_server_init() in order to use the correct appname
even if SPICE without SASL was requested on qemu command line.
---
ui/spice-core.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/ui/spice-core.c b/ui/spice-core.c
index 4cce3b3..e7b2b71 100644
--- a/ui/spice-core.c
+++ b/ui/spice-core.c
@@ -725,8 +725,7 @@ void qemu_spice_init(void)
spice_server_set_ticket(spice_server, password, 0, 0, 0);
}
if (qemu_opt_get_bool(opts, sasl, 0)) {
-if (spice_server_set_sasl_appname(spice_server, qemu) == -1 ||
-spice_server_set_sasl(spice_server, 1) == -1) {
+if (spice_server_set_sasl(spice_server, 1) == -1) {
error_report(spice: failed to enable sasl);
exit(1);
}
@@ -791,6 +790,7 @@ void qemu_spice_init(void)
seamless_migration = qemu_opt_get_bool(opts, seamless-migration, 0);
spice_server_set_seamless_migration(spice_server, seamless_migration);
+spice_server_set_sasl_appname(spice_server, qemu);
if (0 != spice_server_init(spice_server, core_interface)) {
error_report(failed to initialize spice server);
exit(1);
--
1.8.4.2
[Qemu-devel] [PATCH] Fix another corner-case of using VNC+SASL+SPICE
Similarly to the previous commit fixing VNC+SASL+QXL, when starting
QEMU with SPICE but no SASL, and also VNC with SASL, then
spice_server_init() will get called without a previous call to
spice_server_set_sasl_appname(), which will cause cyrus-sasl to
try to use /etc/sasl2/spice.conf (spice-server uses spice as its
default appname) rather than the expected /etc/sasl2/qemu.conf.
This commit unconditionnally calls spice_server_set_sasl_appname()
before calling spice_server_init() in order to use the correct appname
even if SPICE without SASL was requested on qemu command line.
---
ui/spice-core.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/ui/spice-core.c b/ui/spice-core.c
index d7566b0..b8af63b 100644
--- a/ui/spice-core.c
+++ b/ui/spice-core.c
@@ -728,8 +728,7 @@ void qemu_spice_init(void)
spice_server_set_ticket(spice_server, password, 0, 0, 0);
}
if (qemu_opt_get_bool(opts, sasl, 0)) {
-if (spice_server_set_sasl_appname(spice_server, qemu) == -1 ||
-spice_server_set_sasl(spice_server, 1) == -1) {
+if (spice_server_set_sasl(spice_server, 1) == -1) {
error_report(spice: failed to enable sasl);
exit(1);
}
@@ -792,6 +791,7 @@ void qemu_spice_init(void)
seamless_migration = qemu_opt_get_bool(opts, seamless-migration, 0);
spice_server_set_seamless_migration(spice_server, seamless_migration);
+spice_server_set_sasl_appname(spice_server, qemu);
if (0 != spice_server_init(spice_server, core_interface)) {
error_report(failed to initialize spice server);
exit(1);
--
1.8.3.1
[Qemu-devel] [PATCH] Fix VNC SASL authentication when using a QXL device
ui/vnc.c:vnc_display_open() and spice-server/server/reds.c:do_spice_init() are both calling sasl_server_init(). If spice_server_set_sasl_appname() hasn't been called, spice-server will call it with spice as an appname, causing cyrus-sasl to try to use a /etc/sasl2/spice.conf config file rather than the /etc/sasl2/qemu.conf file that QEMU uses. When using -spice sasl on the command line, QEMU properly calls spice_server_set_sasl_appname() to set the SASL appname as qemu, but when using a QXL device without using SPICE, spice_server_init() is called from qemu_spice_add_interface() without setting the appname to qemu, which then causes the VNC code to try to use spice.conf instead of qemu.conf. Signed-off-by: Christophe Fergeau cferg...@redhat.com --- ui/spice-core.c | 1 + 1 file changed, 1 insertion(+) diff --git a/ui/spice-core.c b/ui/spice-core.c index 33ef837..d7566b0 100644 --- a/ui/spice-core.c +++ b/ui/spice-core.c @@ -833,6 +833,7 @@ int qemu_spice_add_interface(SpiceBaseInstance *sin) * With a command line like '-vnc :0 -vga qxl' you'll end up here. */ spice_server = spice_server_new(); +spice_server_set_sasl_appname(spice_server, qemu); spice_server_init(spice_server, core_interface); qemu_add_vm_change_state_handler(vm_change_state_handler, NULL); } -- 1.8.3.1
Re: [Qemu-devel] [Spice-devel] Current qemu-master hangs when used with qxl + linux guest
Hey Hans, On Tue, Oct 08, 2013 at 04:27:38PM +0200, Hans de Goede wrote: I'm having this weird problem with qemu master + spice/qxl using guests. As soon as the guest starts Xorg, I get the following message from qemu: main-loop: WARNING: I/O thread spun for 1000 iterations I've also seen that message when giving a quick try to qemu master today, so it's not just your setup. Christophe pgpVzIFUIkMm2.pgp Description: PGP signature
[Qemu-devel] [PATCH] spice-core: Use g_strdup_printf instead of snprintf
Several places in spice-core.c were using either g_malloc+snprintf
or snprintf+g_strdup to achieve the same result as g_strdup_printf.
Signed-off-by: Christophe Fergeau cferg...@redhat.com
---
ui/spice-core.c | 22 --
1 file changed, 8 insertions(+), 14 deletions(-)
diff --git a/ui/spice-core.c b/ui/spice-core.c
index bd7a248..01b9906 100644
--- a/ui/spice-core.c
+++ b/ui/spice-core.c
@@ -511,7 +511,6 @@ SpiceInfo *qmp_query_spice(Error **errp)
int port, tls_port;
const char *addr;
SpiceInfo *info;
-char version_string[20]; /* 12 = |255.255.255\0| is the max */
info = g_malloc0(sizeof(*info));
@@ -534,11 +533,10 @@ SpiceInfo *qmp_query_spice(Error **errp)
info-host = g_strdup(addr ? addr : 0.0.0.0);
info-has_compiled_version = true;
-snprintf(version_string, sizeof(version_string), %d.%d.%d,
- (SPICE_SERVER_VERSION 0xff) 16,
- (SPICE_SERVER_VERSION 0xff00) 8,
- SPICE_SERVER_VERSION 0xff);
-info-compiled_version = g_strdup(version_string);
+info-compiled_version = g_strdup_printf(%d.%d.%d,
+ (SPICE_SERVER_VERSION 0xff)
16,
+ (SPICE_SERVER_VERSION 0xff00)
8,
+ SPICE_SERVER_VERSION 0xff);
if (port) {
info-has_port = true;
@@ -640,7 +638,7 @@ void qemu_spice_init(void)
char *x509_key_file = NULL,
*x509_cert_file = NULL,
*x509_cacert_file = NULL;
-int port, tls_port, len, addr_flags;
+int port, tls_port, addr_flags;
spice_image_compression_t compression;
spice_wan_compression_t wan_compr;
bool seamless_migration;
@@ -671,30 +669,26 @@ void qemu_spice_init(void)
if (NULL == x509_dir) {
x509_dir = .;
}
-len = strlen(x509_dir) + 32;
str = qemu_opt_get(opts, x509-key-file);
if (str) {
x509_key_file = g_strdup(str);
} else {
-x509_key_file = g_malloc(len);
-snprintf(x509_key_file, len, %s/%s, x509_dir,
X509_SERVER_KEY_FILE);
+x509_key_file = g_strdup_printf(%s/%s, x509_dir,
X509_SERVER_KEY_FILE);
}
str = qemu_opt_get(opts, x509-cert-file);
if (str) {
x509_cert_file = g_strdup(str);
} else {
-x509_cert_file = g_malloc(len);
-snprintf(x509_cert_file, len, %s/%s, x509_dir,
X509_SERVER_CERT_FILE);
+x509_cert_file = g_strdup_printf(%s/%s, x509_dir,
X509_SERVER_CERT_FILE);
}
str = qemu_opt_get(opts, x509-cacert-file);
if (str) {
x509_cacert_file = g_strdup(str);
} else {
-x509_cacert_file = g_malloc(len);
-snprintf(x509_cacert_file, len, %s/%s, x509_dir,
X509_CA_CERT_FILE);
+x509_cacert_file = g_strdup_printf(%s/%s, x509_dir,
X509_CA_CERT_FILE);
}
x509_key_password = qemu_opt_get(opts, x509-key-password);
--
1.8.3.1
[Qemu-devel] [PATCHv2] spice-core: Use g_strdup_printf instead of snprintf
Several places in spice-core.c were using either g_malloc+snprintf
or snprintf+g_strdup to achieve the same result as g_strdup_printf.
Signed-off-by: Christophe Fergeau cferg...@redhat.com
---
Changes since v1:
- split lines 80 chars (checkpatch.pl now succeeds)
ui/spice-core.c | 28 ++--
1 file changed, 14 insertions(+), 14 deletions(-)
diff --git a/ui/spice-core.c b/ui/spice-core.c
index 3a2cd7e..33ef837 100644
--- a/ui/spice-core.c
+++ b/ui/spice-core.c
@@ -511,7 +511,9 @@ SpiceInfo *qmp_query_spice(Error **errp)
int port, tls_port;
const char *addr;
SpiceInfo *info;
-char version_string[20]; /* 12 = |255.255.255\0| is the max */
+unsigned int major;
+unsigned int minor;
+unsigned int micro;
info = g_malloc0(sizeof(*info));
@@ -534,11 +536,10 @@ SpiceInfo *qmp_query_spice(Error **errp)
info-host = g_strdup(addr ? addr : 0.0.0.0);
info-has_compiled_version = true;
-snprintf(version_string, sizeof(version_string), %d.%d.%d,
- (SPICE_SERVER_VERSION 0xff) 16,
- (SPICE_SERVER_VERSION 0xff00) 8,
- SPICE_SERVER_VERSION 0xff);
-info-compiled_version = g_strdup(version_string);
+major = (SPICE_SERVER_VERSION 0xff) 16;
+minor = (SPICE_SERVER_VERSION 0xff00) 8;
+micro = SPICE_SERVER_VERSION 0xff;
+info-compiled_version = g_strdup_printf(%d.%d.%d, major, minor, micro);
if (port) {
info-has_port = true;
@@ -640,7 +641,7 @@ void qemu_spice_init(void)
char *x509_key_file = NULL,
*x509_cert_file = NULL,
*x509_cacert_file = NULL;
-int port, tls_port, len, addr_flags;
+int port, tls_port, addr_flags;
spice_image_compression_t compression;
spice_wan_compression_t wan_compr;
bool seamless_migration;
@@ -671,30 +672,29 @@ void qemu_spice_init(void)
if (NULL == x509_dir) {
x509_dir = .;
}
-len = strlen(x509_dir) + 32;
str = qemu_opt_get(opts, x509-key-file);
if (str) {
x509_key_file = g_strdup(str);
} else {
-x509_key_file = g_malloc(len);
-snprintf(x509_key_file, len, %s/%s, x509_dir,
X509_SERVER_KEY_FILE);
+x509_key_file = g_strdup_printf(%s/%s, x509_dir,
+X509_SERVER_KEY_FILE);
}
str = qemu_opt_get(opts, x509-cert-file);
if (str) {
x509_cert_file = g_strdup(str);
} else {
-x509_cert_file = g_malloc(len);
-snprintf(x509_cert_file, len, %s/%s, x509_dir,
X509_SERVER_CERT_FILE);
+x509_cert_file = g_strdup_printf(%s/%s, x509_dir,
+ X509_SERVER_CERT_FILE);
}
str = qemu_opt_get(opts, x509-cacert-file);
if (str) {
x509_cacert_file = g_strdup(str);
} else {
-x509_cacert_file = g_malloc(len);
-snprintf(x509_cacert_file, len, %s/%s, x509_dir,
X509_CA_CERT_FILE);
+x509_cacert_file = g_strdup_printf(%s/%s, x509_dir,
+ X509_CA_CERT_FILE);
}
x509_key_password = qemu_opt_get(opts, x509-key-password);
--
1.8.3.1
Re: [Qemu-devel] [PATCH] spice-core: Use g_strdup_printf instead of snprintf
On Mon, Sep 02, 2013 at 01:36:19PM +0200, Gerd Hoffmann wrote: On Mo, 2013-09-02 at 11:53 +0200, Christophe Fergeau wrote: Several places in spice-core.c were using either g_malloc+snprintf or snprintf+g_strdup to achieve the same result as g_strdup_printf. Patch looks good but fails checkpatch.pl due to long lines. Ah right, I forgot to run it, I just sent a v2. Christophe pgpKRfEW45QKf.pgp Description: PGP signature
Re: [Qemu-devel] [Spice-devel] QEMU hangs when shutdown windows7 guest with virtio-serial drivers installed
On Fri, Nov 16, 2012 at 06:02:33PM +0800, Dunrong Huang wrote: I meet a weird problem: If I I boot QEMU with virtio-serial being enabled and assign only one cpu to windows VM, when I shutdown VM, QEMU hangs with using 100% cpu. What version of virtio-serial were you using? The old virtio-serial driver from spice-guest-tools-0.1 had similar issues. Christophe pgpsuydkg0cDM.pgp Description: PGP signature
Re: [Qemu-devel] [Spice-devel] QEMU hangs when shutdown windows7 guest with virtio-serial drivers installed
Hey,
On Fri, Nov 16, 2012 at 07:22:58PM +0800, Dunrong Huang wrote:
2012/11/16 Christophe Fergeau cferg...@redhat.com:
On Fri, Nov 16, 2012 at 06:02:33PM +0800, Dunrong Huang wrote:
I meet a weird problem:
If I I boot QEMU with virtio-serial being enabled and assign only one
cpu to windows VM, when I shutdown VM, QEMU hangs with using 100% cpu.
What version of virtio-serial were you using? The old virtio-serial driver
from spice-guest-tools-0.1 had similar issues.
The driver I downloaded is from
http://alt.fedoraproject.org/pub/alt/virtio-win/latest/images/bin/virtio-win-0.1-30.iso.
After mounting ISO, I found following content:
$ cat win7/x86/vioser.inf
[Version]
Signature=$WINDOWS NT$
Class=System
ClassGuid={4d36e97d-e325-11ce-bfc1-08002be10318}
Provider=%REDHAT%
DriverVer=07/03/2012,61.63.103.3000
CatalogFile=vioser.cat
DriverPackageType = PlugAndPlay
DriverPackageDisplayName = %VirtioSerial.DeviceDesc%
Is there a newer driver that has fixed the issue?
The virtio-win-0.1-30 drivers are the newest ones I know of.
Christophe
pgpdfBHZvYhOL.pgp
Description: PGP signature
[Qemu-devel] [PATCH] spice: abort on invalid streaming cmdline params
When parsing its command line parameters, spice aborts when it
finds unexpected values, except for the 'streaming-video' option.
This happens because the parsing of the parameters for this option
is done using the 'name2enum' helper, which does not error out
on unknown values. Using the 'parse_name' helper makes sure we
error out in this case. Looking at git history, the use of
'name2enum' instead of 'parse_name' seems to have been an oversight,
so let's change to that now.
Fixes rhbz#831708
---
ui/spice-core.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/ui/spice-core.c b/ui/spice-core.c
index 4fc48f8..bb4f585 100644
--- a/ui/spice-core.c
+++ b/ui/spice-core.c
@@ -344,7 +344,8 @@ static const char *stream_video_names[] = {
[ SPICE_STREAM_VIDEO_FILTER ] = filter,
};
#define parse_stream_video(_name) \
-name2enum(_name, stream_video_names, ARRAY_SIZE(stream_video_names))
+parse_name(_name, stream video control, \
+ stream_video_names, ARRAY_SIZE(stream_video_names))
static const char *compression_names[] = {
[ SPICE_IMAGE_COMPRESS_OFF ] = off,
--
1.7.11.2
Re: [Qemu-devel] [Spice-devel] Vioserial of Windows guest OS on Qemu 0.15
Hey, On Mon, Mar 19, 2012 at 02:29:41PM +0800, Charles.Tsai-蔡清海-研究發展部 wrote: 3. When we disable and enable the new VirtIO driver several times on either Qemu 1.0 or Qemu 1.0.50, VirtIo driver failed to work after enabling the new VirtIO driver. For what it's worth, Alon Levy also hit this particular bug a few months ago, and I successfully reproduced at that time. I think I reported it to Vadim back in the days, but I'm not 100% sure :) Christophe pgpxQ20RMnnkg.pgp Description: PGP signature
Re: [Qemu-devel] [Spice-devel] Vioserial of Windows guest OS on Qemu 0.15
On Mon, Mar 19, 2012 at 01:42:21PM +0200, Vadim Rozenfeld wrote: On Monday, March 19, 2012 12:12:59 PM Christophe Fergeau wrote: Hey, On Mon, Mar 19, 2012 at 02:29:41PM +0800, Charles.Tsai-蔡清海-研究發展部 wrote: 3. When we disable and enable the new VirtIO driver several times on either Qemu 1.0 or Qemu 1.0.50, VirtIo driver failed to work after enabling the new VirtIO driver. For what it's worth, Alon Levy also hit this particular bug a few months ago, and I successfully reproduced at that time. I think I reported it to Vadim back in the days, but I'm not 100% sure :) Christophe Thanks Christophe. In your case (https://bugzilla.redhat.com/show_bug.cgi?id=750773) an UP system hangs on shutdown. In my case, the hang was during boot even though it magically went away sometimes in the last month I'm not sure whether Charles is hitting the same problem. While trying to figure out what was going on with the bug above, I've definitely hit a enabling/disabling vioserial in the device manager 3 times causes the VM to freeze bug. For what it's worth, lately I've also been experiencing VM freezes when installing vioserial in winxp or win7 UP VMs Christophe pgpHSgu7prD1B.pgp Description: PGP signature
[Qemu-devel] [PATCH 1/2] Add \n to the end of fatal spice error messages
Without it the shell prompt doesn't appear on a new line after
qemu dies.
---
ui/spice-core.c |8
1 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/ui/spice-core.c b/ui/spice-core.c
index 1308a3d..6d240a3 100644
--- a/ui/spice-core.c
+++ b/ui/spice-core.c
@@ -568,15 +568,15 @@ void qemu_spice_init(void)
port = qemu_opt_get_number(opts, port, 0);
tls_port = qemu_opt_get_number(opts, tls-port, 0);
if (!port !tls_port) {
-fprintf(stderr, neither port nor tls-port specified for spice.);
+fprintf(stderr, neither port nor tls-port specified for spice.\n);
exit(1);
}
if (port 0 || port 65535) {
-fprintf(stderr, spice port is out of range);
+fprintf(stderr, spice port is out of range\n);
exit(1);
}
if (tls_port 0 || tls_port 65535) {
-fprintf(stderr, spice tls-port is out of range);
+fprintf(stderr, spice tls-port is out of range\n);
exit(1);
}
password = qemu_opt_get(opts, password);
@@ -700,7 +700,7 @@ void qemu_spice_init(void)
qemu_opt_foreach(opts, add_channel, NULL, 0);
if (0 != spice_server_init(spice_server, core_interface)) {
-fprintf(stderr, failed to initialize spice server);
+fprintf(stderr, failed to initialize spice server\n);
exit(1);
};
using_spice = 1;
--
1.7.7.6
[Qemu-devel] [PATCH 2/2] Error out when tls-channel option is used without TLS
It's currently possible to setup spice channels using TLS when
no TLS port has been specified (ie TLS is disabled). This cannot
work, so better to error out in such a situation.
---
ui/spice-core.c |8 +++-
1 files changed, 7 insertions(+), 1 deletions(-)
diff --git a/ui/spice-core.c b/ui/spice-core.c
index 6d240a3..5e644c9 100644
--- a/ui/spice-core.c
+++ b/ui/spice-core.c
@@ -524,8 +524,11 @@ static int add_channel(const char *name, const char
*value, void *opaque)
{
int security = 0;
int rc;
+int *tls_port = opaque;
if (strcmp(name, tls-channel) == 0) {
+if (!*tls_port)
+return 1;
security = SPICE_CHANNEL_SECURITY_SSL;
}
if (strcmp(name, plaintext-channel) == 0) {
@@ -697,7 +700,10 @@ void qemu_spice_init(void)
spice_server_set_playback_compression
(spice_server, qemu_opt_get_bool(opts, playback-compression, 1));
-qemu_opt_foreach(opts, add_channel, NULL, 0);
+if (qemu_opt_foreach(opts, add_channel, tls_port, 1) != 0) {
+fprintf(stderr, tried to setup tls-channel without specifying a TLS
port\n);
+exit(1);
+}
if (0 != spice_server_init(spice_server, core_interface)) {
fprintf(stderr, failed to initialize spice server\n);
--
1.7.7.6
[Qemu-devel] [PATCHv2 1/2] spice: use error_report to report errors
Error message reporting during spice startup wasn't consistent, it was done
with fprintf(stderr, ) but sometimes the message didn't have a trailing
\n. Using error_report make the intent of the message clearer and deal
with the final \n for us.
---
ui/spice-core.c | 22 +++---
1 files changed, 11 insertions(+), 11 deletions(-)
diff --git a/ui/spice-core.c b/ui/spice-core.c
index 1308a3d..a374999 100644
--- a/ui/spice-core.c
+++ b/ui/spice-core.c
@@ -229,8 +229,8 @@ static void channel_event(int event, SpiceChannelEventInfo
*info)
add_addr_info(server, (struct sockaddr *)info-laddr_ext,
info-llen_ext);
} else {
-fprintf(stderr, spice: %s, extended address is expected\n,
-__func__);
+error_report(spice: %s, extended address is expected,
+ __func__);
#endif
add_addr_info(client, info-paddr, info-plen);
add_addr_info(server, info-laddr, info-llen);
@@ -346,7 +346,7 @@ static int parse_name(const char *string, const char
*optname,
if (value != -1) {
return value;
}
-fprintf(stderr, spice: invalid %s: %s\n, optname, string);
+error_report(spice: invalid %s: %s, optname, string);
exit(1);
}
@@ -540,7 +540,7 @@ static int add_channel(const char *name, const char *value,
void *opaque)
rc = spice_server_set_channel_security(spice_server, value, security);
}
if (rc != 0) {
-fprintf(stderr, spice: failed to set channel security for %s\n,
value);
+error_report(spice: failed to set channel security for %s, value);
exit(1);
}
return 0;
@@ -568,15 +568,15 @@ void qemu_spice_init(void)
port = qemu_opt_get_number(opts, port, 0);
tls_port = qemu_opt_get_number(opts, tls-port, 0);
if (!port !tls_port) {
-fprintf(stderr, neither port nor tls-port specified for spice.);
+error_report(neither port nor tls-port specified for spice);
exit(1);
}
if (port 0 || port 65535) {
-fprintf(stderr, spice port is out of range);
+error_report(spice port is out of range);
exit(1);
}
if (tls_port 0 || tls_port 65535) {
-fprintf(stderr, spice tls-port is out of range);
+error_report(spice tls-port is out of range);
exit(1);
}
password = qemu_opt_get(opts, password);
@@ -646,11 +646,11 @@ void qemu_spice_init(void)
#if SPICE_SERVER_VERSION = 0x000900 /* 0.9.0 */
if (spice_server_set_sasl_appname(spice_server, qemu) == -1 ||
spice_server_set_sasl(spice_server, 1) == -1) {
-fprintf(stderr, spice: failed to enable sasl\n);
+error_report(spice: failed to enable sasl);
exit(1);
}
#else
-fprintf(stderr, spice: sasl is not available (spice = 0.9
required)\n);
+error_report(spice: sasl is not available (spice = 0.9 required));
exit(1);
#endif
}
@@ -700,7 +700,7 @@ void qemu_spice_init(void)
qemu_opt_foreach(opts, add_channel, NULL, 0);
if (0 != spice_server_init(spice_server, core_interface)) {
-fprintf(stderr, failed to initialize spice server);
+error_report(failed to initialize spice server);
exit(1);
};
using_spice = 1;
@@ -725,7 +725,7 @@ int qemu_spice_add_interface(SpiceBaseInstance *sin)
{
if (!spice_server) {
if (QTAILQ_FIRST(qemu_spice_opts.head) != NULL) {
-fprintf(stderr, Oops: spice configured but not active\n);
+error_report(Oops: spice configured but not active);
exit(1);
}
/*
--
1.7.7.6
[Qemu-devel] [PATCHv2 2/2] Error out when tls-channel option is used without TLS
It's currently possible to setup spice channels using TLS when
no TLS port has been specified (ie TLS is disabled). This cannot
work, so better to error out in such a situation.
---
ui/spice-core.c |7 ++-
1 files changed, 6 insertions(+), 1 deletions(-)
diff --git a/ui/spice-core.c b/ui/spice-core.c
index a374999..083af4f 100644
--- a/ui/spice-core.c
+++ b/ui/spice-core.c
@@ -526,6 +526,11 @@ static int add_channel(const char *name, const char
*value, void *opaque)
int rc;
if (strcmp(name, tls-channel) == 0) {
+int *tls_port = opaque;
+if (!*tls_port) {
+error_report(spice: tried to setup tls-channel without specifying
a TLS port\n);
+exit(1);
+}
security = SPICE_CHANNEL_SECURITY_SSL;
}
if (strcmp(name, plaintext-channel) == 0) {
@@ -697,7 +702,7 @@ void qemu_spice_init(void)
spice_server_set_playback_compression
(spice_server, qemu_opt_get_bool(opts, playback-compression, 1));
-qemu_opt_foreach(opts, add_channel, NULL, 0);
+qemu_opt_foreach(opts, add_channel, tls_port, 0);
if (0 != spice_server_init(spice_server, core_interface)) {
error_report(failed to initialize spice server);
--
1.7.7.6
[Qemu-devel] [PATCHv3] Error out when tls-channel option is used without TLS
It's currently possible to setup spice channels using TLS when
no TLS port has been specified (ie TLS is disabled). This cannot
work, so better to error out in such a situation.
---
ui/spice-core.c |8 +++-
1 files changed, 7 insertions(+), 1 deletions(-)
diff --git a/ui/spice-core.c b/ui/spice-core.c
index a374999..9a7912a 100644
--- a/ui/spice-core.c
+++ b/ui/spice-core.c
@@ -526,6 +526,12 @@ static int add_channel(const char *name, const char
*value, void *opaque)
int rc;
if (strcmp(name, tls-channel) == 0) {
+int *tls_port = opaque;
+if (!*tls_port) {
+error_report(spice: tried to setup tls-channel
+ without specifying a TLS port);
+exit(1);
+}
security = SPICE_CHANNEL_SECURITY_SSL;
}
if (strcmp(name, plaintext-channel) == 0) {
@@ -697,7 +703,7 @@ void qemu_spice_init(void)
spice_server_set_playback_compression
(spice_server, qemu_opt_get_bool(opts, playback-compression, 1));
-qemu_opt_foreach(opts, add_channel, NULL, 0);
+qemu_opt_foreach(opts, add_channel, tls_port, 0);
if (0 != spice_server_init(spice_server, core_interface)) {
error_report(failed to initialize spice server);
--
1.7.7.6
Re: [Qemu-devel] [PATCHv3] ps2: migrate ledstate
On Mon, Oct 17, 2011 at 11:25:42AM +0200, Gerd Hoffmann wrote:
static const VMStateDescription vmstate_ps2_common = {
.name = PS2 Common State,
-.version_id = 3,
+.version_id = 4,
.minimum_version_id = 2,
.minimum_version_id_old = 2,
.fields = (VMStateField []) {
version_id in vmstate_ps2_keyboard must be updated too.
Yeah, I somehow updated the field in the wrong struct, /me blushes and
hides. I don't think this struct version needs to be updated.
The version update in vmstate_ps2_common might not be needed, IIRC
the versions for stuff referenced via VMSTATE_STRUCT() isn't used
anyway, Juan?
Ah, ok, I hoped it would help to handle migration between versions with and
without this field, I guess I was too optimistic :)
Thanks,
Christophe
pgpMro5SS9BOn.pgp
Description: PGP signature
Re: [Qemu-devel] [PATCH v4] ps2: migrate ledstate
Hey, On Mon, Oct 17, 2011 at 01:37:34PM +0200, Juan Quintela wrote: Make the ps2 device track its ledstate so that we can migrate it. Otherwise it gets lost across migration, and spice-server gets confused about the actual keyboard state and sends bogus caps/scroll/num key events. This fixes RH bug #729294 We only need to migrate the state when it is different of the default one (0). I tested this version of the patch, and it works for me. Thanks! Christophe pgpVCentKvqMb.pgp Description: PGP signature
[Qemu-devel] [PATCHv3] ps2: migrate ledstate
Make the ps2 device track its ledstate so that we can migrate it.
Otherwise it gets lost across migration, and spice-server gets
confused about the actual keyboard state and sends bogus
caps/scroll/num key events. This fixes RH bug #729294
Signed-off-by: Christophe Fergeau cferg...@redhat.com
---
v3: use VMSTATE_UINT32_V to indicate the ledstate field was added
in version_id == 4
---
hw/ps2.c | 15 ---
1 files changed, 12 insertions(+), 3 deletions(-)
diff --git a/hw/ps2.c b/hw/ps2.c
index 24228c1..f37112b 100644
--- a/hw/ps2.c
+++ b/hw/ps2.c
@@ -92,6 +92,7 @@ typedef struct {
not the keyboard controller. */
int translate;
int scancode_set; /* 1=XT, 2=AT, 3=PS/2 */
+int ledstate;
} PS2KbdState;
typedef struct {
@@ -195,11 +196,17 @@ uint32_t ps2_read_data(void *opaque)
return val;
}
+static void ps2_set_ledstate(PS2KbdState *s, int ledstate)
+{
+s-ledstate = ledstate;
+kbd_put_ledstate(ledstate);
+}
+
static void ps2_reset_keyboard(PS2KbdState *s)
{
s-scan_enabled = 1;
s-scancode_set = 2;
-kbd_put_ledstate(0);
+ps2_set_ledstate(s, 0);
}
void ps2_write_keyboard(void *opaque, int val)
@@ -274,7 +281,7 @@ void ps2_write_keyboard(void *opaque, int val)
s-common.write_cmd = -1;
break;
case KBD_CMD_SET_LEDS:
-kbd_put_ledstate(val);
+ps2_set_ledstate(s, val);
ps2_queue(s-common, KBD_REPLY_ACK);
s-common.write_cmd = -1;
break;
@@ -544,7 +551,7 @@ static void ps2_mouse_reset(void *opaque)
static const VMStateDescription vmstate_ps2_common = {
.name = PS2 Common State,
-.version_id = 3,
+.version_id = 4,
.minimum_version_id = 2,
.minimum_version_id_old = 2,
.fields = (VMStateField []) {
@@ -563,6 +570,7 @@ static int ps2_kbd_post_load(void* opaque, int version_id)
if (version_id == 2)
s-scancode_set=2;
+kbd_put_ledstate(s-ledstate);
return 0;
}
@@ -577,6 +585,7 @@ static const VMStateDescription vmstate_ps2_keyboard = {
VMSTATE_INT32(scan_enabled, PS2KbdState),
VMSTATE_INT32(translate, PS2KbdState),
VMSTATE_INT32_V(scancode_set, PS2KbdState,3),
+VMSTATE_INT32_V(ledstate, PS2KbdState, 4),
VMSTATE_END_OF_LIST()
}
};
--
1.7.6.4
[Qemu-devel] [PATCH] ps2: migrate ledstate
Make the ps2 device track its ledstate so that we can migrate it.
Otherwise it gets lost across migration, and spice-server gets
confused about the actual keyboard state and sends bogus
caps/scroll/num key events.
Signed-off-by: Christophe Fergeau cferg...@redhat.com
---
hw/ps2.c | 13 +++--
1 files changed, 11 insertions(+), 2 deletions(-)
diff --git a/hw/ps2.c b/hw/ps2.c
index 24228c1..f19ea18 100644
--- a/hw/ps2.c
+++ b/hw/ps2.c
@@ -92,6 +92,7 @@ typedef struct {
not the keyboard controller. */
int translate;
int scancode_set; /* 1=XT, 2=AT, 3=PS/2 */
+int ledstate;
} PS2KbdState;
typedef struct {
@@ -195,11 +196,17 @@ uint32_t ps2_read_data(void *opaque)
return val;
}
+static void ps2_set_ledstate(PS2KbdState *s, int ledstate)
+{
+s-ledstate = ledstate;
+kbd_put_ledstate(ledstate);
+}
+
static void ps2_reset_keyboard(PS2KbdState *s)
{
s-scan_enabled = 1;
s-scancode_set = 2;
-kbd_put_ledstate(0);
+ps2_set_ledstate(s, 0);
}
void ps2_write_keyboard(void *opaque, int val)
@@ -274,7 +281,7 @@ void ps2_write_keyboard(void *opaque, int val)
s-common.write_cmd = -1;
break;
case KBD_CMD_SET_LEDS:
-kbd_put_ledstate(val);
+ps2_set_ledstate(s, val);
ps2_queue(s-common, KBD_REPLY_ACK);
s-common.write_cmd = -1;
break;
@@ -563,6 +570,7 @@ static int ps2_kbd_post_load(void* opaque, int version_id)
if (version_id == 2)
s-scancode_set=2;
+kbd_put_ledstate(s-ledstate);
return 0;
}
@@ -577,6 +585,7 @@ static const VMStateDescription vmstate_ps2_keyboard = {
VMSTATE_INT32(scan_enabled, PS2KbdState),
VMSTATE_INT32(translate, PS2KbdState),
VMSTATE_INT32_V(scancode_set, PS2KbdState,3),
+VMSTATE_INT32(ledstate, PS2KbdState),
VMSTATE_END_OF_LIST()
}
};
--
1.7.6.4
Re: [Qemu-devel] [PATCH] ps2: migrate ledstate
Hey, On Wed, Oct 12, 2011 at 06:35:28PM +0200, Christophe Fergeau wrote: Make the ps2 device track its ledstate so that we can migrate it. Otherwise it gets lost across migration, and spice-server gets confused about the actual keyboard state and sends bogus caps/scroll/num key events. This patch adds a ledstate to the PS2 keyboard driver as an alternative to the original patch. I forgot to mention in the changelog entry that this fixes https://bugzilla.redhat.com/show_bug.cgi?id=729294 One thing that could be improved after this patch is that the ledstate is now present both in spice-input and hw/ps2.c, but I couldn't find an easy way to get the ps2 ledstate from spice-input so I left things this way for now. Christophe pgpCVtzFk8rCm.pgp Description: PGP signature
[Qemu-devel] [PATCHv2] ps2: migrate ledstate
Make the ps2 device track its ledstate so that we can migrate it.
Otherwise it gets lost across migration, and spice-server gets
confused about the actual keyboard state and sends bogus
caps/scroll/num key events. This fixes RH bug #729294
Signed-off-by: Christophe Fergeau cferg...@redhat.com
---
hw/ps2.c | 15 ---
1 files changed, 12 insertions(+), 3 deletions(-)
diff --git a/hw/ps2.c b/hw/ps2.c
index 24228c1..3a88681 100644
--- a/hw/ps2.c
+++ b/hw/ps2.c
@@ -92,6 +92,7 @@ typedef struct {
not the keyboard controller. */
int translate;
int scancode_set; /* 1=XT, 2=AT, 3=PS/2 */
+int ledstate;
} PS2KbdState;
typedef struct {
@@ -195,11 +196,17 @@ uint32_t ps2_read_data(void *opaque)
return val;
}
+static void ps2_set_ledstate(PS2KbdState *s, int ledstate)
+{
+s-ledstate = ledstate;
+kbd_put_ledstate(ledstate);
+}
+
static void ps2_reset_keyboard(PS2KbdState *s)
{
s-scan_enabled = 1;
s-scancode_set = 2;
-kbd_put_ledstate(0);
+ps2_set_ledstate(s, 0);
}
void ps2_write_keyboard(void *opaque, int val)
@@ -274,7 +281,7 @@ void ps2_write_keyboard(void *opaque, int val)
s-common.write_cmd = -1;
break;
case KBD_CMD_SET_LEDS:
-kbd_put_ledstate(val);
+ps2_set_ledstate(s, val);
ps2_queue(s-common, KBD_REPLY_ACK);
s-common.write_cmd = -1;
break;
@@ -544,7 +551,7 @@ static void ps2_mouse_reset(void *opaque)
static const VMStateDescription vmstate_ps2_common = {
.name = PS2 Common State,
-.version_id = 3,
+.version_id = 4,
.minimum_version_id = 2,
.minimum_version_id_old = 2,
.fields = (VMStateField []) {
@@ -563,6 +570,7 @@ static int ps2_kbd_post_load(void* opaque, int version_id)
if (version_id == 2)
s-scancode_set=2;
+kbd_put_ledstate(s-ledstate);
return 0;
}
@@ -577,6 +585,7 @@ static const VMStateDescription vmstate_ps2_keyboard = {
VMSTATE_INT32(scan_enabled, PS2KbdState),
VMSTATE_INT32(translate, PS2KbdState),
VMSTATE_INT32_V(scancode_set, PS2KbdState,3),
+VMSTATE_INT32(ledstate, PS2KbdState),
VMSTATE_END_OF_LIST()
}
};
--
1.7.6.4
Re: [Qemu-devel] [PATCH] libcacard: don't leak vcard_emul_alloc_arrays mem
Ping?
On Mon, Jul 04, 2011 at 06:10:43PM +0200, Christophe Fergeau wrote:
vcard_emul_mirror_card and vcard_emul_init use
vcard_emul_alloc_arrays to allocate memory for temporary arrays
which will contain elements that in the end will be used one by
one in cac_card_init. The arrays themselves are never stored
anywhere, they are only used as temporary containers. Hence
the memory that was allocated for these arrays should be freed
after use or they will be leaked.
---
libcacard/vcard_emul_nss.c | 11 ++-
1 files changed, 10 insertions(+), 1 deletions(-)
diff --git a/libcacard/vcard_emul_nss.c b/libcacard/vcard_emul_nss.c
index de324ba..4fee471 100644
--- a/libcacard/vcard_emul_nss.c
+++ b/libcacard/vcard_emul_nss.c
@@ -476,6 +476,7 @@ vcard_emul_mirror_card(VReader *vreader)
VCardKey **keys;
PK11SlotInfo *slot;
PRBool ret;
+VCard *card;
slot = vcard_emul_reader_get_slot(vreader);
if (slot == NULL) {
@@ -535,7 +536,12 @@ vcard_emul_mirror_card(VReader *vreader)
}
/* now create the card */
-return vcard_emul_make_card(vreader, certs, cert_len, keys, cert_count);
+card = vcard_emul_make_card(vreader, certs, cert_len, keys, cert_count);
+qemu_free(certs);
+qemu_free(cert_len);
+qemu_free(keys);
+
+return card;
}
static VCardEmulType default_card_type = VCARD_EMUL_NONE;
@@ -820,6 +826,9 @@ vcard_emul_init(const VCardEmulOptions *options)
vreader_free(vreader);
has_readers = PR_TRUE;
}
+qemu_free(certs);
+qemu_free(cert_len);
+qemu_free(keys);
}
/* if we aren't suppose to use hw, skip looking up hardware tokens */
--
1.7.5.4
pgpOYGNlflJkp.pgp
Description: PGP signature
[Qemu-devel] [PATCHv3 2/4] libcacard: introduce NEXT_TOKEN macro
vcard_emul_options now has repetitive code to read the current
token and advance to the next. After the previous changes,
this repetitive code can be moved in a NEXT_TOKEN macro to
avoid having this code duplicated.
Signed-off-by: Christophe Fergeau cferg...@redhat.com
Reviewed-by: Alon Levy al...@redhat.com
---
libcacard/vcard_emul_nss.c | 71 +++-
1 files changed, 24 insertions(+), 47 deletions(-)
diff --git a/libcacard/vcard_emul_nss.c b/libcacard/vcard_emul_nss.c
index 9271f58..2a20bd6 100644
--- a/libcacard/vcard_emul_nss.c
+++ b/libcacard/vcard_emul_nss.c
@@ -975,6 +975,26 @@ find_blank(const char *str)
static VCardEmulOptions options;
#define READER_STEP 4
+/* Expects args to be at the beginning of a token (ie right after the ','
+ * ending the previous token), and puts the next token start in token,
+ * and its length in token_length. token will not be nul-terminated.
+ * After calling the macro, args will be advanced to the beginning of
+ * the next token.
+ * This macro may call continue or break.
+ */
+#define NEXT_TOKEN(token) \
+(token) = args; \
+args = strpbrk(args, ,)); \
+if (*args == 0) { \
+break; \
+} \
+if (*args == ')') { \
+args++; \
+continue; \
+} \
+(token##_length) = args - (token); \
+args = strip(args+1);
+
VCardEmulOptions *
vcard_emul_options(const char *args)
{
@@ -1010,58 +1030,15 @@ vcard_emul_options(const char *args)
}
args = strip(args+1);
-name = args;
-args = strpbrk(args, ,));
-if (*args == 0) {
-break;
-}
-if (*args == ')') {
-args++;
-continue;
-}
-name_length = args - name;
-args = strip(args+1);
-
-vname = args;
-args = strpbrk(args, ,));
-if (*args == 0) {
-break;
-}
-if (*args == ')') {
-args++;
-continue;
-}
-vname_length = args - vname;
-args = strip(args+1);
-
-type_params = args;
-args = strpbrk(args, ,));
-if (*args == 0) {
-break;
-}
-if (*args == ')') {
-args++;
-continue;
-}
-type_params_length = args - type_params;
-args = strip(args+1);
-
+NEXT_TOKEN(name)
+NEXT_TOKEN(vname)
+NEXT_TOKEN(type_params)
type_params_length = MIN(type_params_length, sizeof(type_str)-1);
strncpy(type_str, type_params, type_params_length);
type_str[type_params_length] = 0;
type = vcard_emul_type_from_string(type_str);
-type_params = args;
-args = strpbrk(args, ,));
-if (*args == 0) {
-break;
-}
-if (*args == ')') {
-args++;
-continue;
-}
-type_params_length = args - type_params;
-args = strip(args+1);
+NEXT_TOKEN(type_params)
if (*args == 0) {
break;
--
1.7.6
[Qemu-devel] [PATCHv3 3/4] libcacard: replace copy_string with strndup
copy_string reimplements strndup, this commit removes it and
replaces all copy_string uses with strndup.
Signed-off-by: Christophe Fergeau cferg...@redhat.com
Reviewed-by: Alon Levy al...@redhat.com
---
libcacard/vcard_emul_nss.c | 23 ++-
1 files changed, 6 insertions(+), 17 deletions(-)
diff --git a/libcacard/vcard_emul_nss.c b/libcacard/vcard_emul_nss.c
index 2a20bd6..de324ba 100644
--- a/libcacard/vcard_emul_nss.c
+++ b/libcacard/vcard_emul_nss.c
@@ -925,17 +925,6 @@ vcard_emul_replay_insertion_events(void)
/*
* Silly little functions to help parsing our argument string
*/
-static char *
-copy_string(const char *str, int str_len)
-{
-char *new_str;
-
-new_str = qemu_malloc(str_len+1);
-memcpy(new_str, str, str_len);
-new_str[str_len] = 0;
-return new_str;
-}
-
static int
count_tokens(const char *str, char token, char token_end)
{
@@ -1054,18 +1043,18 @@ vcard_emul_options(const char *args)
}
opts-vreader = vreaderOpt;
vreaderOpt = vreaderOpt[opts-vreader_count];
-vreaderOpt-name = copy_string(name, name_length);
-vreaderOpt-vname = copy_string(vname, vname_length);
+vreaderOpt-name = qemu_strndup(name, name_length);
+vreaderOpt-vname = qemu_strndup(vname, vname_length);
vreaderOpt-card_type = type;
vreaderOpt-type_params =
-copy_string(type_params, type_params_length);
+qemu_strndup(type_params, type_params_length);
count = count_tokens(args, ',', ')') + 1;
vreaderOpt-cert_count = count;
vreaderOpt-cert_name = (char **)qemu_malloc(count*sizeof(char *));
for (i = 0; i count; i++) {
const char *cert = args;
args = strpbrk(args, ,));
-vreaderOpt-cert_name[i] = copy_string(cert, args - cert);
+vreaderOpt-cert_name[i] = qemu_strndup(cert, args - cert);
args = strip(args+1);
}
if (*args == ')') {
@@ -1092,7 +1081,7 @@ vcard_emul_options(const char *args)
args = strip(args+10);
params = args;
args = find_blank(args);
-opts-hw_type_params = copy_string(params, args-params);
+opts-hw_type_params = qemu_strndup(params, args-params);
/* db=/data/base/path */
} else if (strncmp(args, db=, 3) == 0) {
const char *db;
@@ -1103,7 +1092,7 @@ vcard_emul_options(const char *args)
args++;
db = args;
args = strpbrk(args, \\n);
-opts-nss_db = copy_string(db, args-db);
+opts-nss_db = qemu_strndup(db, args-db);
if (*args != 0) {
args++;
}
--
1.7.6
[Qemu-devel] [PATCHv3 1/4] libcacard: fix soft=... parsing in vcard_emul_options
The previous parser had copy and paste errors when computing
vname_length and type_params_length, name was used instead
of respectively vname and type_params. This led to length that could
be bigger than the input string, and to access out of the array
bounds when trying to copy these strings. valgrind rightfully
complained about this. It also didn't handle empty fields correctly,
Signed-off-by: Christophe Fergeau cferg...@redhat.com
Reviewed-by: Alon Levy al...@redhat.com
---
libcacard/vcard_emul_nss.c | 45 +++
1 files changed, 28 insertions(+), 17 deletions(-)
diff --git a/libcacard/vcard_emul_nss.c b/libcacard/vcard_emul_nss.c
index 184252f..9271f58 100644
--- a/libcacard/vcard_emul_nss.c
+++ b/libcacard/vcard_emul_nss.c
@@ -980,8 +980,6 @@ vcard_emul_options(const char *args)
{
int reader_count = 0;
VCardEmulOptions *opts;
-char type_str[100];
-int type_len;
/* Allow the future use of allocating the options structure on the fly */
memcpy(options, default_options, sizeof(options));
@@ -996,18 +994,24 @@ vcard_emul_options(const char *args)
* cert_2,cert_3...) */
if (strncmp(args, soft=, 5) == 0) {
const char *name;
+size_t name_length;
const char *vname;
+size_t vname_length;
const char *type_params;
+size_t type_params_length;
+char type_str[100];
VCardEmulType type;
-int name_length, vname_length, type_params_length, count, i;
+int count, i;
VirtualReaderOptions *vreaderOpt = NULL;
args = strip(args + 5);
if (*args != '(') {
continue;
}
+args = strip(args+1);
+
name = args;
-args = strpbrk(args + 1, ,));
+args = strpbrk(args, ,));
if (*args == 0) {
break;
}
@@ -1015,10 +1019,11 @@ vcard_emul_options(const char *args)
args++;
continue;
}
+name_length = args - name;
args = strip(args+1);
-name_length = args - name - 2;
+
vname = args;
-args = strpbrk(args + 1, ,));
+args = strpbrk(args, ,));
if (*args == 0) {
break;
}
@@ -1026,13 +1031,10 @@ vcard_emul_options(const char *args)
args++;
continue;
}
-vname_length = args - name - 2;
+vname_length = args - vname;
args = strip(args+1);
-type_len = strpbrk(args, ,)) - args;
-assert(sizeof(type_str) type_len);
-strncpy(type_str, args, type_len);
-type_str[type_len] = 0;
-type = vcard_emul_type_from_string(type_str);
+
+type_params = args;
args = strpbrk(args, ,));
if (*args == 0) {
break;
@@ -1041,9 +1043,16 @@ vcard_emul_options(const char *args)
args++;
continue;
}
+type_params_length = args - type_params;
args = strip(args+1);
+
+type_params_length = MIN(type_params_length, sizeof(type_str)-1);
+strncpy(type_str, type_params, type_params_length);
+type_str[type_params_length] = 0;
+type = vcard_emul_type_from_string(type_str);
+
type_params = args;
-args = strpbrk(args + 1, ,));
+args = strpbrk(args, ,));
if (*args == 0) {
break;
}
@@ -1051,8 +1060,9 @@ vcard_emul_options(const char *args)
args++;
continue;
}
-type_params_length = args - name;
+type_params_length = args - type_params;
args = strip(args+1);
+
if (*args == 0) {
break;
}
@@ -1072,13 +1082,14 @@ vcard_emul_options(const char *args)
vreaderOpt-card_type = type;
vreaderOpt-type_params =
copy_string(type_params, type_params_length);
-count = count_tokens(args, ',', ')');
+count = count_tokens(args, ',', ')') + 1;
vreaderOpt-cert_count = count;
vreaderOpt-cert_name = (char **)qemu_malloc(count*sizeof(char *));
for (i = 0; i count; i++) {
-const char *cert = args + 1;
-args = strpbrk(args + 1, ,));
+const char *cert = args;
+args = strpbrk(args, ,));
vreaderOpt-cert_name[i] = copy_string(cert, args - cert);
+args = strip(args+1);
}
if (*args == ')') {
args++;
--
1.7.6
[Qemu-devel] [PATCHv3 4/4] libcacard: don't leak vcard_emul_alloc_arrays mem
vcard_emul_mirror_card and vcard_emul_init use
vcard_emul_alloc_arrays to allocate memory for temporary arrays
which will contain elements that in the end will be used one by
one in cac_card_init. The arrays themselves are never stored
anywhere, they are only used as temporary containers. Hence
the memory that was allocated for these arrays should be freed
after use or they will be leaked.
Signed-off-by: Christophe Fergeau cferg...@redhat.com
---
libcacard/vcard_emul_nss.c | 11 ++-
1 files changed, 10 insertions(+), 1 deletions(-)
diff --git a/libcacard/vcard_emul_nss.c b/libcacard/vcard_emul_nss.c
index de324ba..4fee471 100644
--- a/libcacard/vcard_emul_nss.c
+++ b/libcacard/vcard_emul_nss.c
@@ -476,6 +476,7 @@ vcard_emul_mirror_card(VReader *vreader)
VCardKey **keys;
PK11SlotInfo *slot;
PRBool ret;
+VCard *card;
slot = vcard_emul_reader_get_slot(vreader);
if (slot == NULL) {
@@ -535,7 +536,12 @@ vcard_emul_mirror_card(VReader *vreader)
}
/* now create the card */
-return vcard_emul_make_card(vreader, certs, cert_len, keys, cert_count);
+card = vcard_emul_make_card(vreader, certs, cert_len, keys, cert_count);
+qemu_free(certs);
+qemu_free(cert_len);
+qemu_free(keys);
+
+return card;
}
static VCardEmulType default_card_type = VCARD_EMUL_NONE;
@@ -820,6 +826,9 @@ vcard_emul_init(const VCardEmulOptions *options)
vreader_free(vreader);
has_readers = PR_TRUE;
}
+qemu_free(certs);
+qemu_free(cert_len);
+qemu_free(keys);
}
/* if we aren't suppose to use hw, skip looking up hardware tokens */
--
1.7.6
[Qemu-devel] [PATCHv4 0/4] libcacard fixes
Hi, This is a resend of my series of libcacard fixes, with the correct patches this time. Changes since v3: - the series now includes the intended patches Changes since v2: - added Reviewed-by: Alon Levy al...@redhat.com to the 4 patches Changes since v1: - split first patch into 3 patches, v1 2/2 is unchanged Christophe Fergeau (4): libcacard: fix soft=... parsing in vcard_emul_options libcacard: introduce NEXT_TOKEN macro libcacard: replace copy_string with strndup libcacard: don't leak vcard_emul_alloc_arrays mem libcacard/vcard_emul_nss.c | 124 +++ 1 files changed, 55 insertions(+), 69 deletions(-) -- 1.7.6
[Qemu-devel] [PATCHv4 3/4] libcacard: introduce NEXT_TOKEN macro
vcard_emul_options now has repetitive code to read the current
token and advance to the next. After the previous changes,
this repetitive code can be moved in a NEXT_TOKEN macro to
avoid having this code duplicated.
Signed-off-by: Christophe Fergeau cferg...@redhat.com
Reviewed-by: Alon Levy al...@redhat.com
---
libcacard/vcard_emul_nss.c | 71 +++-
1 files changed, 24 insertions(+), 47 deletions(-)
diff --git a/libcacard/vcard_emul_nss.c b/libcacard/vcard_emul_nss.c
index 9271f58..2a20bd6 100644
--- a/libcacard/vcard_emul_nss.c
+++ b/libcacard/vcard_emul_nss.c
@@ -975,6 +975,26 @@ find_blank(const char *str)
static VCardEmulOptions options;
#define READER_STEP 4
+/* Expects args to be at the beginning of a token (ie right after the ','
+ * ending the previous token), and puts the next token start in token,
+ * and its length in token_length. token will not be nul-terminated.
+ * After calling the macro, args will be advanced to the beginning of
+ * the next token.
+ * This macro may call continue or break.
+ */
+#define NEXT_TOKEN(token) \
+(token) = args; \
+args = strpbrk(args, ,)); \
+if (*args == 0) { \
+break; \
+} \
+if (*args == ')') { \
+args++; \
+continue; \
+} \
+(token##_length) = args - (token); \
+args = strip(args+1);
+
VCardEmulOptions *
vcard_emul_options(const char *args)
{
@@ -1010,58 +1030,15 @@ vcard_emul_options(const char *args)
}
args = strip(args+1);
-name = args;
-args = strpbrk(args, ,));
-if (*args == 0) {
-break;
-}
-if (*args == ')') {
-args++;
-continue;
-}
-name_length = args - name;
-args = strip(args+1);
-
-vname = args;
-args = strpbrk(args, ,));
-if (*args == 0) {
-break;
-}
-if (*args == ')') {
-args++;
-continue;
-}
-vname_length = args - vname;
-args = strip(args+1);
-
-type_params = args;
-args = strpbrk(args, ,));
-if (*args == 0) {
-break;
-}
-if (*args == ')') {
-args++;
-continue;
-}
-type_params_length = args - type_params;
-args = strip(args+1);
-
+NEXT_TOKEN(name)
+NEXT_TOKEN(vname)
+NEXT_TOKEN(type_params)
type_params_length = MIN(type_params_length, sizeof(type_str)-1);
strncpy(type_str, type_params, type_params_length);
type_str[type_params_length] = 0;
type = vcard_emul_type_from_string(type_str);
-type_params = args;
-args = strpbrk(args, ,));
-if (*args == 0) {
-break;
-}
-if (*args == ')') {
-args++;
-continue;
-}
-type_params_length = args - type_params;
-args = strip(args+1);
+NEXT_TOKEN(type_params)
if (*args == 0) {
break;
--
1.7.6
[Qemu-devel] [PATCHv4 4/4] libcacard: replace copy_string with strndup
copy_string reimplements strndup, this commit removes it and
replaces all copy_string uses with strndup.
Signed-off-by: Christophe Fergeau cferg...@redhat.com
Reviewed-by: Alon Levy al...@redhat.com
---
libcacard/vcard_emul_nss.c | 23 ++-
1 files changed, 6 insertions(+), 17 deletions(-)
diff --git a/libcacard/vcard_emul_nss.c b/libcacard/vcard_emul_nss.c
index 2a20bd6..de324ba 100644
--- a/libcacard/vcard_emul_nss.c
+++ b/libcacard/vcard_emul_nss.c
@@ -925,17 +925,6 @@ vcard_emul_replay_insertion_events(void)
/*
* Silly little functions to help parsing our argument string
*/
-static char *
-copy_string(const char *str, int str_len)
-{
-char *new_str;
-
-new_str = qemu_malloc(str_len+1);
-memcpy(new_str, str, str_len);
-new_str[str_len] = 0;
-return new_str;
-}
-
static int
count_tokens(const char *str, char token, char token_end)
{
@@ -1054,18 +1043,18 @@ vcard_emul_options(const char *args)
}
opts-vreader = vreaderOpt;
vreaderOpt = vreaderOpt[opts-vreader_count];
-vreaderOpt-name = copy_string(name, name_length);
-vreaderOpt-vname = copy_string(vname, vname_length);
+vreaderOpt-name = qemu_strndup(name, name_length);
+vreaderOpt-vname = qemu_strndup(vname, vname_length);
vreaderOpt-card_type = type;
vreaderOpt-type_params =
-copy_string(type_params, type_params_length);
+qemu_strndup(type_params, type_params_length);
count = count_tokens(args, ',', ')') + 1;
vreaderOpt-cert_count = count;
vreaderOpt-cert_name = (char **)qemu_malloc(count*sizeof(char *));
for (i = 0; i count; i++) {
const char *cert = args;
args = strpbrk(args, ,));
-vreaderOpt-cert_name[i] = copy_string(cert, args - cert);
+vreaderOpt-cert_name[i] = qemu_strndup(cert, args - cert);
args = strip(args+1);
}
if (*args == ')') {
@@ -1092,7 +1081,7 @@ vcard_emul_options(const char *args)
args = strip(args+10);
params = args;
args = find_blank(args);
-opts-hw_type_params = copy_string(params, args-params);
+opts-hw_type_params = qemu_strndup(params, args-params);
/* db=/data/base/path */
} else if (strncmp(args, db=, 3) == 0) {
const char *db;
@@ -1103,7 +1092,7 @@ vcard_emul_options(const char *args)
args++;
db = args;
args = strpbrk(args, \\n);
-opts-nss_db = copy_string(db, args-db);
+opts-nss_db = qemu_strndup(db, args-db);
if (*args != 0) {
args++;
}
--
1.7.6
[Qemu-devel] [PATCHv4 2/4] libcacard: fix soft=... parsing in vcard_emul_options
The previous parser had copy and paste errors when computing
vname_length and type_params_length, name was used instead
of respectively vname and type_params. This led to length that could
be bigger than the input string, and to access out of the array
bounds when trying to copy these strings. valgrind rightfully
complained about this. It also didn't handle empty fields correctly,
Signed-off-by: Christophe Fergeau cferg...@redhat.com
Reviewed-by: Alon Levy al...@redhat.com
---
libcacard/vcard_emul_nss.c | 45 +++
1 files changed, 28 insertions(+), 17 deletions(-)
diff --git a/libcacard/vcard_emul_nss.c b/libcacard/vcard_emul_nss.c
index 184252f..9271f58 100644
--- a/libcacard/vcard_emul_nss.c
+++ b/libcacard/vcard_emul_nss.c
@@ -980,8 +980,6 @@ vcard_emul_options(const char *args)
{
int reader_count = 0;
VCardEmulOptions *opts;
-char type_str[100];
-int type_len;
/* Allow the future use of allocating the options structure on the fly */
memcpy(options, default_options, sizeof(options));
@@ -996,18 +994,24 @@ vcard_emul_options(const char *args)
* cert_2,cert_3...) */
if (strncmp(args, soft=, 5) == 0) {
const char *name;
+size_t name_length;
const char *vname;
+size_t vname_length;
const char *type_params;
+size_t type_params_length;
+char type_str[100];
VCardEmulType type;
-int name_length, vname_length, type_params_length, count, i;
+int count, i;
VirtualReaderOptions *vreaderOpt = NULL;
args = strip(args + 5);
if (*args != '(') {
continue;
}
+args = strip(args+1);
+
name = args;
-args = strpbrk(args + 1, ,));
+args = strpbrk(args, ,));
if (*args == 0) {
break;
}
@@ -1015,10 +1019,11 @@ vcard_emul_options(const char *args)
args++;
continue;
}
+name_length = args - name;
args = strip(args+1);
-name_length = args - name - 2;
+
vname = args;
-args = strpbrk(args + 1, ,));
+args = strpbrk(args, ,));
if (*args == 0) {
break;
}
@@ -1026,13 +1031,10 @@ vcard_emul_options(const char *args)
args++;
continue;
}
-vname_length = args - name - 2;
+vname_length = args - vname;
args = strip(args+1);
-type_len = strpbrk(args, ,)) - args;
-assert(sizeof(type_str) type_len);
-strncpy(type_str, args, type_len);
-type_str[type_len] = 0;
-type = vcard_emul_type_from_string(type_str);
+
+type_params = args;
args = strpbrk(args, ,));
if (*args == 0) {
break;
@@ -1041,9 +1043,16 @@ vcard_emul_options(const char *args)
args++;
continue;
}
+type_params_length = args - type_params;
args = strip(args+1);
+
+type_params_length = MIN(type_params_length, sizeof(type_str)-1);
+strncpy(type_str, type_params, type_params_length);
+type_str[type_params_length] = 0;
+type = vcard_emul_type_from_string(type_str);
+
type_params = args;
-args = strpbrk(args + 1, ,));
+args = strpbrk(args, ,));
if (*args == 0) {
break;
}
@@ -1051,8 +1060,9 @@ vcard_emul_options(const char *args)
args++;
continue;
}
-type_params_length = args - name;
+type_params_length = args - type_params;
args = strip(args+1);
+
if (*args == 0) {
break;
}
@@ -1072,13 +1082,14 @@ vcard_emul_options(const char *args)
vreaderOpt-card_type = type;
vreaderOpt-type_params =
copy_string(type_params, type_params_length);
-count = count_tokens(args, ',', ')');
+count = count_tokens(args, ',', ')') + 1;
vreaderOpt-cert_count = count;
vreaderOpt-cert_name = (char **)qemu_malloc(count*sizeof(char *));
for (i = 0; i count; i++) {
-const char *cert = args + 1;
-args = strpbrk(args + 1, ,));
+const char *cert = args;
+args = strpbrk(args, ,));
vreaderOpt-cert_name[i] = copy_string(cert, args - cert);
+args = strip(args+1);
}
if (*args == ')') {
args++;
--
1.7.6
[Qemu-devel] [PATCHv4 1/4] libcacard: s/strip(args++)/strip(args+1)
vcard_emul_options used args = strip(args++) a few times, which
was not returning the expected result since the rest of the code
expected args to be increased by at least 1, which is not the case
if *args is not a blank space when this function is called.
Replace these calls by strip(args+1) which will do what we expect.
Signed-off-by: Christophe Fergeau cferg...@redhat.com
Reviewed-by: Alon Levy al...@redhat.com
---
libcacard/vcard_emul_nss.c |4 ++--
1 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/libcacard/vcard_emul_nss.c b/libcacard/vcard_emul_nss.c
index f3db657..184252f 100644
--- a/libcacard/vcard_emul_nss.c
+++ b/libcacard/vcard_emul_nss.c
@@ -1041,7 +1041,7 @@ vcard_emul_options(const char *args)
args++;
continue;
}
-args = strip(args++);
+args = strip(args+1);
type_params = args;
args = strpbrk(args + 1, ,));
if (*args == 0) {
@@ -1052,7 +1052,7 @@ vcard_emul_options(const char *args)
continue;
}
type_params_length = args - name;
-args = strip(args++);
+args = strip(args+1);
if (*args == 0) {
break;
}
--
1.7.6
Re: [Qemu-devel] [PATCHv3 0/4] libcacard fixes
On Fri, Jul 22, 2011 at 01:33:26PM +0200, Christophe Fergeau wrote: This is a resend of my series of libcacard fixes, no change since last version apart from the added Reviewed-by tag. Scratch that, I put the wrong patches in the series (which strongly indicates that I should merge libcacard: fix soft=... parsing in vcard_emul_options as a fifth patch in this set). I'll resend the actual series. Christophe pgpoDL7I17Z62.pgp Description: PGP signature
[Qemu-devel] [PATCH] libcacard: don't leak vcard_emul_alloc_arrays mem
vcard_emul_mirror_card and vcard_emul_init use
vcard_emul_alloc_arrays to allocate memory for temporary arrays
which will contain elements that in the end will be used one by
one in cac_card_init. The arrays themselves are never stored
anywhere, they are only used as temporary containers. Hence
the memory that was allocated for these arrays should be freed
after use or they will be leaked.
---
libcacard/vcard_emul_nss.c | 11 ++-
1 files changed, 10 insertions(+), 1 deletions(-)
diff --git a/libcacard/vcard_emul_nss.c b/libcacard/vcard_emul_nss.c
index de324ba..4fee471 100644
--- a/libcacard/vcard_emul_nss.c
+++ b/libcacard/vcard_emul_nss.c
@@ -476,6 +476,7 @@ vcard_emul_mirror_card(VReader *vreader)
VCardKey **keys;
PK11SlotInfo *slot;
PRBool ret;
+VCard *card;
slot = vcard_emul_reader_get_slot(vreader);
if (slot == NULL) {
@@ -535,7 +536,12 @@ vcard_emul_mirror_card(VReader *vreader)
}
/* now create the card */
-return vcard_emul_make_card(vreader, certs, cert_len, keys, cert_count);
+card = vcard_emul_make_card(vreader, certs, cert_len, keys, cert_count);
+qemu_free(certs);
+qemu_free(cert_len);
+qemu_free(keys);
+
+return card;
}
static VCardEmulType default_card_type = VCARD_EMUL_NONE;
@@ -820,6 +826,9 @@ vcard_emul_init(const VCardEmulOptions *options)
vreader_free(vreader);
has_readers = PR_TRUE;
}
+qemu_free(certs);
+qemu_free(cert_len);
+qemu_free(keys);
}
/* if we aren't suppose to use hw, skip looking up hardware tokens */
--
1.7.5.4
Re: [Qemu-devel] [PATCH 2/2] libcacard: add pc file, install it + includes
Hi,
On Mon, Jun 27, 2011 at 12:34:44PM +0200, Alon Levy wrote:
Also add --pkgconfigdir and --includedir configure parameters.
---
configure | 10 ++
libcacard/Makefile| 25 ++---
libcacard/libcacard.pc.in | 13 +
3 files changed, 45 insertions(+), 3 deletions(-)
create mode 100644 libcacard/libcacard.pc.in
diff --git a/configure b/configure
index e523976..d08818f 100755
--- a/configure
+++ b/configure
@@ -146,6 +146,8 @@ datadir=\${prefix}/share/qemu
docdir=\${prefix}/share/doc/qemu
bindir=\${prefix}/bin
libdir=\${prefix}/lib
+pkgconfigdir=\${prefix}/lib/pkgconfig
It would be more convenient if it was \${libdir}/pkgconfig and if it was
assigned a default value after --libdir has been parsed, though that would
look a bit out of place compared to how other values are set.
Christophe
pgpnCtuhP8i6i.pgp
Description: PGP signature
Re: [Qemu-devel] [PATCH 1/2] libcacard: fix soft=... parsing in vcard_emul_options
On Fri, Jun 24, 2011 at 06:51:51PM +0200, Alon Levy wrote:
On Fri, Jun 24, 2011 at 04:37:39PM +0200, Christophe Fergeau wrote:
The previous parser had copy and paste errors when computing
vname_length and type_params_length, name was used instead
of respectively vname and type_params. This led to length that could
be bigger than the input string, and to access out of the array
bounds when trying to copy these strings. valgrind rightfully
complained about this. It also didn't handle empty fields correctly,
and there were some args = strip(args++); which also didn't do what
was expected.
Aren't there token parsing functions in libc that can be used if we
want to fix the repetitiveness?
The only token parsing function I know of in libc is strtok{_r} which
modifies the input string, so it's not very well suited here. I'm not a
libc specialist though, so there might be some functions in libc (or qemu
helpers) that I don't know of.
Christophe
pgpZkrVNT3zIm.pgp
Description: PGP signature
[Qemu-devel] [PATCHv2 2/4] libcacard: fix soft=... parsing in vcard_emul_options
The previous parser had copy and paste errors when computing
vname_length and type_params_length, name was used instead
of respectively vname and type_params. This led to length that could
be bigger than the input string, and to access out of the array
bounds when trying to copy these strings. valgrind rightfully
complained about this. It also didn't handle empty fields correctly,
Signed-off-by: Christophe Fergeau cferg...@redhat.com
---
libcacard/vcard_emul_nss.c | 45 +++
1 files changed, 28 insertions(+), 17 deletions(-)
diff --git a/libcacard/vcard_emul_nss.c b/libcacard/vcard_emul_nss.c
index 184252f..9271f58 100644
--- a/libcacard/vcard_emul_nss.c
+++ b/libcacard/vcard_emul_nss.c
@@ -980,8 +980,6 @@ vcard_emul_options(const char *args)
{
int reader_count = 0;
VCardEmulOptions *opts;
-char type_str[100];
-int type_len;
/* Allow the future use of allocating the options structure on the fly */
memcpy(options, default_options, sizeof(options));
@@ -996,18 +994,24 @@ vcard_emul_options(const char *args)
* cert_2,cert_3...) */
if (strncmp(args, soft=, 5) == 0) {
const char *name;
+size_t name_length;
const char *vname;
+size_t vname_length;
const char *type_params;
+size_t type_params_length;
+char type_str[100];
VCardEmulType type;
-int name_length, vname_length, type_params_length, count, i;
+int count, i;
VirtualReaderOptions *vreaderOpt = NULL;
args = strip(args + 5);
if (*args != '(') {
continue;
}
+args = strip(args+1);
+
name = args;
-args = strpbrk(args + 1, ,));
+args = strpbrk(args, ,));
if (*args == 0) {
break;
}
@@ -1015,10 +1019,11 @@ vcard_emul_options(const char *args)
args++;
continue;
}
+name_length = args - name;
args = strip(args+1);
-name_length = args - name - 2;
+
vname = args;
-args = strpbrk(args + 1, ,));
+args = strpbrk(args, ,));
if (*args == 0) {
break;
}
@@ -1026,13 +1031,10 @@ vcard_emul_options(const char *args)
args++;
continue;
}
-vname_length = args - name - 2;
+vname_length = args - vname;
args = strip(args+1);
-type_len = strpbrk(args, ,)) - args;
-assert(sizeof(type_str) type_len);
-strncpy(type_str, args, type_len);
-type_str[type_len] = 0;
-type = vcard_emul_type_from_string(type_str);
+
+type_params = args;
args = strpbrk(args, ,));
if (*args == 0) {
break;
@@ -1041,9 +1043,16 @@ vcard_emul_options(const char *args)
args++;
continue;
}
+type_params_length = args - type_params;
args = strip(args+1);
+
+type_params_length = MIN(type_params_length, sizeof(type_str)-1);
+strncpy(type_str, type_params, type_params_length);
+type_str[type_params_length] = 0;
+type = vcard_emul_type_from_string(type_str);
+
type_params = args;
-args = strpbrk(args + 1, ,));
+args = strpbrk(args, ,));
if (*args == 0) {
break;
}
@@ -1051,8 +1060,9 @@ vcard_emul_options(const char *args)
args++;
continue;
}
-type_params_length = args - name;
+type_params_length = args - type_params;
args = strip(args+1);
+
if (*args == 0) {
break;
}
@@ -1072,13 +1082,14 @@ vcard_emul_options(const char *args)
vreaderOpt-card_type = type;
vreaderOpt-type_params =
copy_string(type_params, type_params_length);
-count = count_tokens(args, ',', ')');
+count = count_tokens(args, ',', ')') + 1;
vreaderOpt-cert_count = count;
vreaderOpt-cert_name = (char **)qemu_malloc(count*sizeof(char *));
for (i = 0; i count; i++) {
-const char *cert = args + 1;
-args = strpbrk(args + 1, ,));
+const char *cert = args;
+args = strpbrk(args, ,));
vreaderOpt-cert_name[i] = copy_string(cert, args - cert);
+args = strip(args+1);
}
if (*args == ')') {
args++;
--
1.7.5.4
[Qemu-devel] [PATCHv2 4/4] libcacard: replace copy_string with strndup
copy_string reimplements strndup, this commit removes it and
replaces all copy_string uses with strndup.
Signed-off-by: Christophe Fergeau cferg...@redhat.com
---
libcacard/vcard_emul_nss.c | 23 ++-
1 files changed, 6 insertions(+), 17 deletions(-)
diff --git a/libcacard/vcard_emul_nss.c b/libcacard/vcard_emul_nss.c
index 2a20bd6..de324ba 100644
--- a/libcacard/vcard_emul_nss.c
+++ b/libcacard/vcard_emul_nss.c
@@ -925,17 +925,6 @@ vcard_emul_replay_insertion_events(void)
/*
* Silly little functions to help parsing our argument string
*/
-static char *
-copy_string(const char *str, int str_len)
-{
-char *new_str;
-
-new_str = qemu_malloc(str_len+1);
-memcpy(new_str, str, str_len);
-new_str[str_len] = 0;
-return new_str;
-}
-
static int
count_tokens(const char *str, char token, char token_end)
{
@@ -1054,18 +1043,18 @@ vcard_emul_options(const char *args)
}
opts-vreader = vreaderOpt;
vreaderOpt = vreaderOpt[opts-vreader_count];
-vreaderOpt-name = copy_string(name, name_length);
-vreaderOpt-vname = copy_string(vname, vname_length);
+vreaderOpt-name = qemu_strndup(name, name_length);
+vreaderOpt-vname = qemu_strndup(vname, vname_length);
vreaderOpt-card_type = type;
vreaderOpt-type_params =
-copy_string(type_params, type_params_length);
+qemu_strndup(type_params, type_params_length);
count = count_tokens(args, ',', ')') + 1;
vreaderOpt-cert_count = count;
vreaderOpt-cert_name = (char **)qemu_malloc(count*sizeof(char *));
for (i = 0; i count; i++) {
const char *cert = args;
args = strpbrk(args, ,));
-vreaderOpt-cert_name[i] = copy_string(cert, args - cert);
+vreaderOpt-cert_name[i] = qemu_strndup(cert, args - cert);
args = strip(args+1);
}
if (*args == ')') {
@@ -1092,7 +1081,7 @@ vcard_emul_options(const char *args)
args = strip(args+10);
params = args;
args = find_blank(args);
-opts-hw_type_params = copy_string(params, args-params);
+opts-hw_type_params = qemu_strndup(params, args-params);
/* db=/data/base/path */
} else if (strncmp(args, db=, 3) == 0) {
const char *db;
@@ -1103,7 +1092,7 @@ vcard_emul_options(const char *args)
args++;
db = args;
args = strpbrk(args, \\n);
-opts-nss_db = copy_string(db, args-db);
+opts-nss_db = qemu_strndup(db, args-db);
if (*args != 0) {
args++;
}
--
1.7.5.4
