[Spice-devel] [PATCH 3/3] replay: Handle errors in record file

[Jonathon Jongsma]

On Thu, 2016-06-09 at 05:35 -0400, Frediano Ziglio wrote:
> > 


[snip]


> > > @@ -678,7 +747,7 @@ static void red_replay_string_free(SpiceReplay
> > > *replay,
> > > QXLString *qxl)
> > >      red_replay_data_chunks_free(replay, qxl, sizeof(*qxl));
> > >  }
> > >  
> > > -static void red_replay_text_ptr(SpiceReplay *replay, QXLText *qxl,
> > > uint32_t
> > > flags)
> > > +static replay_t red_replay_text_ptr(SpiceReplay *replay, QXLText *qxl,
> > > uint32_t flags)
> > >  {
> > >      int temp;
> > >  
> > > @@ -688,6 +757,7 @@ static void red_replay_text_ptr(SpiceReplay *replay,
> > > QXLText *qxl, uint32_t flag
> > >     red_replay_brush_ptr(replay, &qxl->back_brush, flags);
> > >     replay_fscanf(replay, "fore_mode %d\n", &temp); qxl->fore_mode = temp;
> > >     replay_fscanf(replay, "back_mode %d\n", &temp); qxl->back_mode = temp;
> > > +    return REPLAY_OK;
> > >  }
> > >  
> > >  static void red_replay_text_free(SpiceReplay *replay, QXLText *qxl,
> > >  uint32_t
> > > flags)
> > > @@ -697,9 +767,10 @@ static void red_replay_text_free(SpiceReplay *replay,
> > > QXLText *qxl, uint32_t fla
> > >      red_replay_brush_free(replay, &qxl->back_brush, flags);
> > >  }
> > >  
> > > -static void red_replay_whiteness_ptr(SpiceReplay *replay, QXLWhiteness
> > > *qxl,
> > > uint32_t flags)
> > > +static replay_t red_replay_whiteness_ptr(SpiceReplay *replay,
> > > QXLWhiteness
> > > *qxl, uint32_t flags)
> > >  {
> > >      red_replay_qmask_ptr(replay, &qxl->mask, flags);
> > > +    return REPLAY_OK;
> > >  }
> > >  
> > >  static void red_replay_whiteness_free(SpiceReplay *replay, QXLWhiteness
> > >  *qxl,
> > > uint32_t flags)
> > > @@ -707,9 +778,10 @@ static void red_replay_whiteness_free(SpiceReplay
> > > *replay, QXLWhiteness *qxl, ui
> > >      red_replay_qmask_free(replay, &qxl->mask, flags);
> > >  }
> > >  
> > > -static void red_replay_blackness_ptr(SpiceReplay *replay, QXLBlackness
> > > *qxl,
> > > uint32_t flags)
> > > +static replay_t red_replay_blackness_ptr(SpiceReplay *replay,
> > > QXLBlackness
> > > *qxl, uint32_t flags)
> > >  {
> > >      red_replay_qmask_ptr(replay, &qxl->mask, flags);
> > > +    return REPLAY_OK;
> > >  }
> > >  
> > >  static void red_replay_blackness_free(SpiceReplay *replay, QXLBlackness
> > >  *qxl,
> > > uint32_t flags)
> > > @@ -717,9 +789,10 @@ static void red_replay_blackness_free(SpiceReplay
> > > *replay, QXLBlackness *qxl, ui
> > >      red_replay_qmask_free(replay, &qxl->mask, flags);
> > >  }
> > >  
> > > -static void red_replay_invers_ptr(SpiceReplay *replay, QXLInvers *qxl,
> > > uint32_t flags)
> > > +static replay_t red_replay_invers_ptr(SpiceReplay *replay, QXLInvers
> > > *qxl,
> > > uint32_t flags)
> > >  {
> > >      red_replay_qmask_ptr(replay, &qxl->mask, flags);
> > > +    return REPLAY_OK;
> > >  }
> > >  
> > >  static void red_replay_invers_free(SpiceReplay *replay, QXLInvers *qxl,
> > > uint32_t flags)
> > > @@ -727,7 +800,7 @@ static void red_replay_invers_free(SpiceReplay
> > > *replay,
> > > QXLInvers *qxl, uint32_t
> > >      red_replay_qmask_free(replay, &qxl->mask, flags);
> > >  }
> > >  
> > > -static void red_replay_clip_ptr(SpiceReplay *replay, QXLClip *qxl)
> > > +static replay_t red_replay_clip_ptr(SpiceReplay *replay, QXLClip *qxl)
> > >  {
> > >      replay_fscanf(replay, "type %d\n", &qxl->type);
> > >      switch (qxl->type) {
> > > @@ -735,6 +808,7 @@ static void red_replay_clip_ptr(SpiceReplay *replay,
> > > QXLClip *qxl)
> > >          qxl->data = QXLPHYSICAL_FROM_PTR(red_replay_clip_rects(replay));
> > >          break;
> > >      }
> > > +    return REPLAY_OK;
> > >  }
> > > >  


That reminds me. In my previous review, I was going to note that in the
functions above, you don't appear to be handling potential errors from
replay_fscanf(). But I forgot to mention it. I was going to mention it now, but
I just realized that the replay_fscanf() function is actually redefined as a
macro that returns on an error. This is why I don't like to these macros. It
makes the code difficult to understand at a glance.

> > 
> > 
> I'll post another version. It's quite a big change, could be
> that I miss some points. I didn't split the allocated list into a
> separate patch, I think was not clear the purpose, I put more comment
> on the commit.
> 
> Frediano

ok