>
> On Fri, 2016-09-16 at 12:32 +0100, Frediano Ziglio wrote:
> > To check fscanf read all needed information a dummy "%n" is appended
> > to any string and the value stored there is tested. This as scanf
> > family
> > could return a valid value but not entirely process the string so
> > adding a "%n" and checking this was processed make sure all expected
> > string is found.
> > The code to check for a specific string is now a bit more complicated
> > as replay_fscanf use a macro which append a constant string.
> > The "error" field is used to mark any error happened, so in most
> > cases
> > there is no explicit check beside when this could cause a problem
> > (for instance results of replay_fscanf used which would result in
> > uninitialised variable usage).
> >
> > Signed-off-by: Frediano Ziglio <fziglio@xxxxxxxxxx>
> > ---
> > server/red-replay-qxl.c | 91
> > +++++++++++++++++++++++++++++++++++++++++++------
> > 1 file changed, 80 insertions(+), 11 deletions(-)
> >
> > diff --git a/server/red-replay-qxl.c b/server/red-replay-qxl.c
> > index 7eddaf4..968a605 100644
> > --- a/server/red-replay-qxl.c
> > +++ b/server/red-replay-qxl.c
> > @@ -49,6 +49,7 @@ struct SpiceReplay {
> > GArray *id_map_inv; // replay id -> record id
> > GArray *id_free; // free list
> > int nsurfaces;
> > + int end_pos;
> >
> > GList *allocated;
> >
> > @@ -69,11 +70,13 @@ static int replay_fread(SpiceReplay *replay,
> > uint8_t *buf, size_t size)
> > }
> >
> > __attribute__((format(scanf, 2, 3)))
> > -static replay_t replay_fscanf(SpiceReplay *replay, const char *fmt,
> > ...)
> > +static replay_t replay_fscanf_check(SpiceReplay *replay, const char
> > *fmt, ...)
> > {
> > va_list ap;
> > int ret;
> >
> > + replay->end_pos = -1;
> > +
> > if (replay->error) {
> > return REPLAY_ERROR;
> > }
> > @@ -84,11 +87,13 @@ static replay_t replay_fscanf(SpiceReplay
> > *replay, const char *fmt, ...)
> > va_start(ap, fmt);
> > ret = vfscanf(replay->fd, fmt, ap);
> > va_end(ap);
> > - if (ret == EOF) {
> > + if (ret == EOF || replay->end_pos < 0) {
> > replay->error = TRUE;
> > }
> > return replay->error ? REPLAY_ERROR : REPLAY_OK;
> > }
>
> I like the idea, but I'm not a big fan of the implementation here. This
> function makes two hard assumptions. It assumes that it is called with
> a format string that ends with %n and it assumes that the last argument
> is &r->end_pos. But it doesn't verify either of these assumptions, so
> it's really easy to misuse.
>
> > +#define replay_fscanf(r, fmt, ...) \
> > + replay_fscanf_check(r, fmt "%n", ## __VA_ARGS__, &r->end_pos)
>
> Here we're enforcing the two assumptions mentioned above. So when code
> below uses this macro, things are OK. But we can't use this macro in
> all situations.
>
> >
> > static inline void *replay_malloc(SpiceReplay *replay, size_t size)
> > {
> > @@ -210,9 +215,11 @@ static replay_t read_binary(SpiceReplay *replay,
> > const char *prefix, size_t *siz
> > uint8_t *zlib_buffer;
> > z_stream strm;
> >
> > - snprintf(template, sizeof(template), "binary %%d %s %%ld:",
> > prefix);
> > - if (replay_fscanf(replay, template, &with_zlib, size) ==
> > REPLAY_ERROR)
> > + 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;
> > + }
>
> Here for example.
>
Yes, I always used the macro till Christophe said that template code
was more readable.
Any alternative suggestion that make all happy?
> >
> > if (*buf == NULL) {
> > *buf = replay_malloc(replay, *size + base_size);
> > @@ -224,15 +231,19 @@ static replay_t read_binary(SpiceReplay
> > *replay, const char *prefix, size_t *siz
> > hexdump(*buf + base_size, *size);
> > }
> > #else
> > - spice_return_val_if_fail(with_zlib != -1, REPLAY_ERROR);
> > if (with_zlib) {
> > int ret;
> > GList *elem;
> >
> > replay_fscanf(replay, "%d:", &zlib_size);
> > + if (replay->error) {
> > + return REPLAY_ERROR;
> > + }
> > zlib_buffer = replay_malloc(replay, zlib_size);
> > elem = replay->allocated;
> > - replay_fread(replay, zlib_buffer, zlib_size);
> > + if (replay_fread(replay, zlib_buffer, zlib_size) !=
> > zlib_size) {
> > + return REPLAY_ERROR;
> > + }
> > strm.zalloc = Z_NULL;
> > strm.zfree = Z_NULL;
> > strm.opaque = Z_NULL;
> > @@ -265,8 +276,7 @@ static replay_t read_binary(SpiceReplay *replay,
> > const char *prefix, size_t *siz
> > replay_fread(replay, *buf + base_size, *size);
> > }
> > #endif
> > - replay_fscanf(replay, "\n");
> > - return REPLAY_OK;
> > + return replay_fscanf(replay, "\n");
> > }
> >
> > static size_t red_replay_data_chunks(SpiceReplay *replay, const char
> > *prefix,
> > @@ -337,8 +347,9 @@ static void red_replay_rect_ptr(SpiceReplay
> > *replay, const char *prefix, QXLRect
> > {
> > char template[1024];
> >
> > - snprintf(template, sizeof(template), "rect %s %%d %%d %%d
> > %%d\n", prefix);
> > - replay_fscanf(replay, template, &qxl->top, &qxl->left, &qxl-
> > >bottom, &qxl->right);
> > + snprintf(template, sizeof(template), "rect %s %%d %%d %%d
> > %%d\n%%n", prefix);
> > + replay_fscanf_check(replay, template, &qxl->top, &qxl->left,
> > &qxl->bottom, &qxl->right,
> > + &replay->end_pos);
> > }
> >
> > static QXLPath *red_replay_path(SpiceReplay *replay)
> > @@ -364,6 +375,9 @@ static QXLClipRects
> > *red_replay_clip_rects(SpiceReplay *replay)
> > int num_rects;
> >
> > replay_fscanf(replay, "num_rects %d\n", &num_rects);
> > + if (replay->error) {
> > + return NULL;
> > + }
>
>
> I personally think it would be cleaner in these cases to do something
> like
>
> if (replay_fscanf(...) == REPLAY_ERROR) {
> return NULL;
> }
>
> instead of relying on the side-effect of setting the replay->error
> variable.
>
>
This was previous code but now it's similar to the way ferror works
and also make code similar when there are more replay_fscanf one after
each other where test is only on the last replay_fscanf which was indeed
quite confusing.
Frediano
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel