On Tue, 2016-09-20 at 12:00 -0400, Frediano Ziglio wrote:
> > 
> > 
> > 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 <fzig...@redhat.com>
> > > ---
> > >  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?
> 

OK, I missed this earlier review. I guess I prefer the old way. But I
don't care too much.

> > 
> > > 
> > >  
> > >      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


Personal preference, I guess. I don't really like this style, but if
you prefer it, that's fine. Then you should ignore the comments on the
following patches as well.


Acked-by: Jonathon Jongsma <jjong...@redhat.com>
_______________________________________________
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Reply via email to