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

[Frediano Ziglio]

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

I think is just question of style. replay_fscanf_check is the real
function used by replay_fscanf macro. The macro allow to add the
check parameter either in the format string (adding a constant) and
in the arguments (and hide the necessity of adding them).

Personally I think that generating a not constant format string
with "%%" is confusing so I proposed the constant format string
and the additional strcmp check. But it all end up with style
and personal opinions.

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

Or rename the flag if is not clear, something like

  if (replay->had_errors) ...

(for me error is enough). But I think the comment was more about testing
replay_fscanf result instead of the flag.

Frediano