[Spice-devel] [RFC PATCH spice-server] replay: Use setjmp/longjmp for error handling
Jonathon Jongsma
jjongsma at redhat.com
Mon Oct 31 20:48:24 UTC 2016
On Mon, 2016-10-31 at 07:49 -0400, Marc-André Lureau wrote:
> 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
it does make the "structure" of the code look simpler, but I find it
much more complicated to understand what's going on. I prefer code that
doesn't make me think too hard, even if it's a bit more verbose. So I'm
tempted to NACK this change.
Or convert it to C++ :D
Jonathon
>
> >
> > 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
> >
> _______________________________________________
> 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