[Spice-devel] [PATCH v3 2/8] replay: Handle errors reading strings from record file

[Christophe Fergeau]

On Tue, Sep 20, 2016 at 12:00:45PM -0400, Frediano Ziglio wrote:
> > 
> > On Fri, 2016-09-16 at 12:32 +0100, Frediano Ziglio wrote:
> > 
> > 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?

Oh, I did not realize that this would require changing replay_fscanf()
to replay_fscanf_check(), I'm quite happy to take back my comment if
this makes this part worse.

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

Maybe wrap the test up in a if (replay_had_io_error(replay)) { return NULL;
} ? Dunno if that makes things better for everyone from a readability
point of view?

Christophe
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 801 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/spice-devel/attachments/20161003/91f8c960/attachment-0001.sig>