[Spice-devel] [RFC PATCH spice-server] replay: Use setjmp/longjmp for error handling
Marc-André Lureau
mlureau at redhat.com
Mon Oct 31 11:49:25 UTC 2016
Hi
----- Original Message -----
> To avoid checking for error in all the path use setjmp/longjmp.
> This allow to jump directly to the error handling code in
> spice_replay_next_cmd.
>
> Signed-off-by: Frediano Ziglio <fziglio at redhat.com>
> ---
> server/red-replay-qxl.c | 142
> ++++++++++++------------------------------------
> 1 file changed, 35 insertions(+), 107 deletions(-)
>
> I'm not a great fun of setjmp/longjmp but I have to say
> this way simplify different checks.
>
/me neither
cheers
> diff --git a/server/red-replay-qxl.c b/server/red-replay-qxl.c
> index 78de48b..daaca0d 100644
> --- a/server/red-replay-qxl.c
> +++ b/server/red-replay-qxl.c
> @@ -23,13 +23,16 @@
> #include <inttypes.h>
> #include <zlib.h>
> #include <pthread.h>
> +#include <glib.h>
> +#include <assert.h>
> +#include <setjmp.h>
> +
> #include "reds.h"
> #include "red-qxl.h"
> #include "red-common.h"
> #include "memslot.h"
> #include "red-parse-qxl.h"
> #include "red-replay-qxl.h"
> -#include <glib.h>
>
> #define QXLPHYSICAL_FROM_PTR(ptr) ((QXLPHYSICAL)(intptr_t)(ptr))
> #define QXLPHYSICAL_TO_PTR(phy) ((void*)(intptr_t)(phy))
> @@ -42,6 +45,7 @@ typedef enum {
> struct SpiceReplay {
> FILE *fd;
> gboolean error;
> + jmp_buf jmp_error;
> int counter;
> bool created_primary;
>
> @@ -57,18 +61,24 @@ struct SpiceReplay {
> pthread_cond_t cond;
> };
>
> +static void SPICE_GNUC_NORETURN replay_got_error(SpiceReplay *replay)
> +{
> + replay->error = TRUE;
> + longjmp(replay->jmp_error, 1);
> +}
> +
> static ssize_t replay_fread(SpiceReplay *replay, uint8_t *buf, size_t size)
> {
> if (replay->error || feof(replay->fd) ||
> fread(buf, 1, size, replay->fd) != size) {
> - replay->error = TRUE;
> + replay_got_error(replay);
> return 0;
> }
> return size;
> }
>
> __attribute__((format(scanf, 2, 3)))
> -static replay_t replay_fscanf_check(SpiceReplay *replay, const char *fmt,
> ...)
> +static void replay_fscanf_check(SpiceReplay *replay, const char *fmt, ...)
> {
> va_list ap;
> int ret;
> @@ -76,19 +86,17 @@ static replay_t replay_fscanf_check(SpiceReplay *replay,
> const char *fmt, ...)
> replay->end_pos = -1;
>
> if (replay->error) {
> - return REPLAY_ERROR;
> + replay_got_error(replay);
> }
> if (feof(replay->fd)) {
> - replay->error = TRUE;
> - return REPLAY_ERROR;
> + replay_got_error(replay);
> }
> va_start(ap, fmt);
> ret = vfscanf(replay->fd, fmt, ap);
> va_end(ap);
> if (ret == EOF || replay->end_pos < 0) {
> - replay->error = TRUE;
> + replay_got_error(replay);
> }
> - return replay->error ? REPLAY_ERROR : REPLAY_OK;
> }
> #define replay_fscanf(r, fmt, ...) \
> replay_fscanf_check(r, fmt "%n", ## __VA_ARGS__, &r->end_pos)
> @@ -217,8 +225,8 @@ static void hexdump(uint8_t *hex, uint8_t bytes)
> }
> #endif
>
> -static replay_t read_binary(SpiceReplay *replay, const char *prefix, size_t
> *size, uint8_t
> - **buf, size_t base_size)
> +static void read_binary(SpiceReplay *replay, const char *prefix, size_t
> *size,
> + uint8_t **buf, size_t base_size)
> {
> char template[1024];
> int with_zlib = -1;
> @@ -228,9 +236,6 @@ static replay_t read_binary(SpiceReplay *replay, const
> char *prefix, size_t *siz
>
> snprintf(template, sizeof(template), "binary %%d %s %%ld:%%n", prefix);
> replay_fscanf_check(replay, template, &with_zlib, size,
> &replay->end_pos);
> - if (replay->error) {
> - return REPLAY_ERROR;
> - }
>
> if (*buf == NULL) {
> *buf = replay_malloc(replay, *size + base_size);
> @@ -246,13 +251,8 @@ static replay_t read_binary(SpiceReplay *replay, const
> char *prefix, size_t *siz
> int ret;
>
> replay_fscanf(replay, "%u:", &zlib_size);
> - if (replay->error) {
> - return REPLAY_ERROR;
> - }
> zlib_buffer = replay_malloc(replay, zlib_size);
> - if (replay_fread(replay, zlib_buffer, zlib_size) != zlib_size) {
> - return REPLAY_ERROR;
> - }
> + replay_fread(replay, zlib_buffer, zlib_size);
> strm.zalloc = Z_NULL;
> strm.zfree = Z_NULL;
> strm.opaque = Z_NULL;
> @@ -272,7 +272,7 @@ static replay_t read_binary(SpiceReplay *replay, const
> char *prefix, size_t *siz
> * it seems it may kill the red_worker thread (so a chunk is
> * left hanging and the rest of the message is never
> written).
> * Let it pass */
> - return REPLAY_ERROR;
> + replay_got_error(replay);
> }
> if (ret != Z_OK) {
> spice_warn_if_reached();
> @@ -284,7 +284,7 @@ static replay_t read_binary(SpiceReplay *replay, const
> char *prefix, size_t *siz
> replay_fread(replay, *buf + base_size, *size);
> }
> #endif
> - return replay_fscanf(replay, "\n");
> + replay_fscanf(replay, "\n");
> }
>
> static ssize_t red_replay_data_chunks(SpiceReplay *replay, const char
> *prefix,
> @@ -296,25 +296,19 @@ static ssize_t red_replay_data_chunks(SpiceReplay
> *replay, const char *prefix,
> QXLDataChunk *cur, *next;
>
> replay_fscanf(replay, "data_chunks %u %zu\n", &count_chunks,
> &data_size);
> - if (replay->error) {
> - return -1;
> - }
> if (base_size == 0) {
> base_size = sizeof(QXLDataChunk);
> }
>
> - if (read_binary(replay, prefix, &next_data_size, mem, base_size) ==
> REPLAY_ERROR) {
> - return -1;
> - }
> + read_binary(replay, prefix, &next_data_size, mem, base_size);
> +
> cur = (QXLDataChunk*)(*mem + base_size - sizeof(QXLDataChunk));
> cur->data_size = next_data_size;
> data_size = cur->data_size;
> cur->next_chunk = cur->prev_chunk = 0;
> while (count_chunks-- > 0) {
> - if (read_binary(replay, prefix, &next_data_size,
> (uint8_t**)&cur->next_chunk,
> - sizeof(QXLDataChunk)) == REPLAY_ERROR) {
> - return -1;
> - }
> + read_binary(replay, prefix, &next_data_size,
> (uint8_t**)&cur->next_chunk,
> + sizeof(QXLDataChunk));
> data_size += next_data_size;
> next = QXLPHYSICAL_TO_PTR(cur->next_chunk);
> next->prev_chunk = QXLPHYSICAL_FROM_PTR(cur);
> @@ -389,9 +383,6 @@ static QXLClipRects *red_replay_clip_rects(SpiceReplay
> *replay)
> unsigned int num_rects;
>
> replay_fscanf(replay, "num_rects %u\n", &num_rects);
> - if (replay->error) {
> - return NULL;
> - }
> if (red_replay_data_chunks(replay, "clip_rects", (uint8_t**)&qxl,
> sizeof(QXLClipRects)) < 0) {
> return NULL;
> }
> @@ -423,9 +414,6 @@ static QXLImage *red_replay_image(SpiceReplay *replay,
> uint32_t flags)
> int has_image;
>
> replay_fscanf(replay, "image %d\n", &has_image);
> - if (replay->error) {
> - return NULL;
> - }
> if (!has_image) {
> return NULL;
> }
> @@ -436,9 +424,6 @@ static QXLImage *red_replay_image(SpiceReplay *replay,
> uint32_t flags)
> replay_fscanf(replay, "descriptor.flags %d\n", &temp);
> qxl->descriptor.flags = temp;
> replay_fscanf(replay, "descriptor.width %d\n", &qxl->descriptor.width);
> replay_fscanf(replay, "descriptor.height %d\n",
> &qxl->descriptor.height);
> - if (replay->error) {
> - return NULL;
> - }
>
> switch (qxl->descriptor.type) {
> case SPICE_IMAGE_TYPE_BITMAP:
> @@ -454,9 +439,6 @@ static QXLImage *red_replay_image(SpiceReplay *replay,
> uint32_t flags)
> unsigned int i, num_ents;
>
> replay_fscanf(replay, "qp.num_ents %u\n", &num_ents);
> - if (replay->error) {
> - return NULL;
> - }
> qp = replay_malloc(replay, sizeof(QXLPalette) + num_ents *
> sizeof(qp->ents[0]));
> qp->num_ents = num_ents;
> qxl->bitmap.palette = QXLPHYSICAL_FROM_PTR(qp);
> @@ -481,18 +463,12 @@ static QXLImage *red_replay_image(SpiceReplay *replay,
> uint32_t flags)
> break;
> case SPICE_IMAGE_TYPE_SURFACE:
> replay_fscanf(replay, "surface_image.surface_id %d\n",
> &qxl->surface_image.surface_id);
> - if (replay->error) {
> - return NULL;
> - }
> qxl->surface_image.surface_id = replay_id_get(replay,
> qxl->surface_image.surface_id);
> break;
> case SPICE_IMAGE_TYPE_QUIC:
> // TODO - make this much nicer (precompute size and allocs, store
> them during
> // record, then reread into them. and use MPEG-4).
> replay_fscanf(replay, "quic.data_size %d\n", &qxl->quic.data_size);
> - if (replay->error) {
> - return NULL;
> - }
> qxl = replay_realloc(replay, qxl, sizeof(QXLImageDescriptor) +
> sizeof(QXLQUICData) +
> qxl->quic.data_size);
> size = red_replay_data_chunks(replay, "quic.data",
> (uint8_t**)&qxl->quic.data, 0);
> @@ -534,9 +510,6 @@ static void red_replay_image_free(SpiceReplay *replay,
> QXLPHYSICAL p, uint32_t f
> static void red_replay_brush_ptr(SpiceReplay *replay, QXLBrush *qxl,
> uint32_t flags)
> {
> replay_fscanf(replay, "type %d\n", &qxl->type);
> - if (replay->error) {
> - return;
> - }
>
> switch (qxl->type) {
> case SPICE_BRUSH_TYPE_SOLID:
> @@ -702,9 +675,6 @@ static void red_replay_stroke_ptr(SpiceReplay *replay,
> QXLStroke *qxl, uint32_t
>
> qxl->path = QXLPHYSICAL_FROM_PTR(red_replay_path(replay));
> replay_fscanf(replay, "attr.flags %d\n", &temp); qxl->attr.flags = temp;
> - if (replay->error) {
> - return;
> - }
> if (qxl->attr.flags & SPICE_LINE_FLAGS_STYLED) {
> size_t size;
>
> @@ -805,9 +775,6 @@ static void red_replay_invers_free(SpiceReplay *replay,
> QXLInvers *qxl, uint32_t
> static void red_replay_clip_ptr(SpiceReplay *replay, QXLClip *qxl)
> {
> replay_fscanf(replay, "type %d\n", &qxl->type);
> - if (replay->error) {
> - return;
> - }
> switch (qxl->type) {
> case SPICE_CLIP_TYPE_RECTS:
> qxl->data = QXLPHYSICAL_FROM_PTR(red_replay_clip_rects(replay));
> @@ -877,24 +844,15 @@ static QXLDrawable
> *red_replay_native_drawable(SpiceReplay *replay, uint32_t fla
> replay_fscanf(replay, "self_bitmap %d\n", &temp); qxl->self_bitmap =
> temp;
> red_replay_rect_ptr(replay, "self_bitmap_area", &qxl->self_bitmap_area);
> replay_fscanf(replay, "surface_id %d\n", &qxl->surface_id);
> - if (replay->error) {
> - return NULL;
> - }
> qxl->surface_id = replay_id_get(replay, qxl->surface_id);
>
> for (i = 0; i < 3; i++) {
> replay_fscanf(replay, "surfaces_dest %d\n", &qxl->surfaces_dest[i]);
> - if (replay->error) {
> - return NULL;
> - }
> qxl->surfaces_dest[i] = replay_id_get(replay,
> qxl->surfaces_dest[i]);
> red_replay_rect_ptr(replay, "surfaces_rects",
> &qxl->surfaces_rects[i]);
> }
>
> replay_fscanf(replay, "type %d\n", &temp); qxl->type = temp;
> - if (replay->error) {
> - return NULL;
> - }
> switch (qxl->type) {
> case QXL_DRAW_ALPHA_BLEND:
> red_replay_alpha_blend_ptr(replay, &qxl->u.alpha_blend, flags);
> @@ -1017,9 +975,6 @@ static QXLCompatDrawable
> *red_replay_compat_drawable(SpiceReplay *replay, uint32
> red_replay_rect_ptr(replay, "bitmap_area", &qxl->bitmap_area);
>
> replay_fscanf(replay, "type %d\n", &temp); qxl->type = temp;
> - if (replay->error) {
> - return NULL;
> - }
> switch (qxl->type) {
> case QXL_DRAW_ALPHA_BLEND:
> red_replay_alpha_blend_ptr_compat(replay, &qxl->u.alpha_blend,
> flags);
> @@ -1072,9 +1027,6 @@ static QXLCompatDrawable
> *red_replay_compat_drawable(SpiceReplay *replay, uint32
> static QXLPHYSICAL red_replay_drawable(SpiceReplay *replay, uint32_t flags)
> {
> replay_fscanf(replay, "drawable\n");
> - if (replay->error) {
> - return 0;
> - }
> if (flags & QXL_COMMAND_FLAG_COMPAT) {
> return QXLPHYSICAL_FROM_PTR(red_replay_compat_drawable(replay,
> flags));
> } else {
> @@ -1090,9 +1042,6 @@ static QXLUpdateCmd *red_replay_update_cmd(SpiceReplay
> *replay)
> red_replay_rect_ptr(replay, "area", &qxl->area);
> replay_fscanf(replay, "update_id %d\n", &qxl->update_id);
> replay_fscanf(replay, "surface_id %d\n", &qxl->surface_id);
> - if (replay->error) {
> - return NULL;
> - }
> qxl->surface_id = replay_id_get(replay, qxl->surface_id);
>
> return qxl;
> @@ -1118,9 +1067,6 @@ static QXLSurfaceCmd
> *red_replay_surface_cmd(SpiceReplay *replay)
> replay_fscanf(replay, "surface_id %d\n", &qxl->surface_id);
> replay_fscanf(replay, "type %d\n", &temp); qxl->type = temp;
> replay_fscanf(replay, "flags %d\n", &qxl->flags);
> - if (replay->error) {
> - return NULL;
> - }
>
> switch (qxl->type) {
> case QXL_SURFACE_CMD_CREATE:
> @@ -1128,9 +1074,6 @@ static QXLSurfaceCmd
> *red_replay_surface_cmd(SpiceReplay *replay)
> replay_fscanf(replay, "u.surface_create.width %d\n",
> &qxl->u.surface_create.width);
> replay_fscanf(replay, "u.surface_create.height %d\n",
> &qxl->u.surface_create.height);
> replay_fscanf(replay, "u.surface_create.stride %d\n",
> &qxl->u.surface_create.stride);
> - if (replay->error) {
> - return NULL;
> - }
> size = qxl->u.surface_create.height *
> abs(qxl->u.surface_create.stride);
> if ((qxl->flags & QXL_SURF_FLAG_KEEP_DATA) != 0) {
> read_binary(replay, "data", &read_size,
> (uint8_t**)&qxl->u.surface_create.data, 0);
> @@ -1178,9 +1121,6 @@ static QXLCursor *red_replay_cursor(SpiceReplay
> *replay)
> cursor.header.hot_spot_y = temp;
>
> replay_fscanf(replay, "data_size %d\n", &temp);
> - if (replay->error) {
> - return NULL;
> - }
> data_size = red_replay_data_chunks(replay, "cursor", (uint8_t**)&qxl,
> sizeof(QXLCursor));
> if (data_size < 0) {
> return NULL;
> @@ -1197,9 +1137,6 @@ static QXLCursorCmd *red_replay_cursor_cmd(SpiceReplay
> *replay)
>
> replay_fscanf(replay, "cursor_cmd\n");
> replay_fscanf(replay, "type %d\n", &temp);
> - if (replay->error) {
> - return NULL;
> - }
> qxl->type = temp;
> switch (qxl->type) {
> case QXL_CURSOR_SET:
> @@ -1249,9 +1186,6 @@ static void replay_handle_create_primary(QXLWorker
> *worker, SpiceReplay *replay)
> &surface.stride, &surface.format);
> replay_fscanf(replay, "%d %d %d %d\n", &surface.position,
> &surface.mouse_mode,
> &surface.flags, &surface.type);
> - if (replay->error) {
> - return;
> - }
> read_binary(replay, "data", &size, &mem, 0);
> surface.group_id = 0;
> surface.mem = QXLPHYSICAL_FROM_PTR(mem);
> @@ -1302,12 +1236,19 @@ SPICE_GNUC_VISIBLE QXLCommandExt*
> spice_replay_next_cmd(SpiceReplay *replay,
> int what = -1;
> int counter;
>
> + if (setjmp(replay->jmp_error)) {
> + /* if there were some allocation during the read free all
> + * buffers allocated to avoid leaks */
> + if (replay->allocated) {
> + g_list_free_full(replay->allocated, free);
> + replay->allocated = NULL;
> + }
> + return NULL;
> + }
> +
> while (what != 0) {
> replay_fscanf(replay, "event %d %d %d %"SCNu64"\n", &counter,
> &what, &type, ×tamp);
> - if (replay->error) {
> - goto error;
> - }
> if (what == 1) {
> replay_handle_dev_input(worker, replay, type);
> }
> @@ -1335,10 +1276,6 @@ SPICE_GNUC_VISIBLE QXLCommandExt*
> spice_replay_next_cmd(SpiceReplay *replay,
> break;
> }
>
> - if (replay->error) {
> - goto error;
> - }
> -
> QXLReleaseInfo *info;
> switch (cmd->cmd.type) {
> case QXL_CMD_DRAW:
> @@ -1359,15 +1296,6 @@ SPICE_GNUC_VISIBLE QXLCommandExt*
> spice_replay_next_cmd(SpiceReplay *replay,
> replay->counter++;
>
> return cmd;
> -
> -error:
> - /* if there were some allocation during the read free all
> - * buffers allocated to avoid leaks */
> - if (replay->allocated) {
> - g_list_free_full(replay->allocated, free);
> - replay->allocated = NULL;
> - }
> - return NULL;
> }
>
> SPICE_GNUC_VISIBLE void spice_replay_free_cmd(SpiceReplay *replay,
> QXLCommandExt *cmd)
> --
> 2.7.4
>
> _______________________________________________
> Spice-devel mailing list
> Spice-devel at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel
>
More information about the Spice-devel
mailing list