[Spice-devel] [spice] server: Don't check the 'this' mjpeg_encoder pointer

[Francois Gouget]

On Fri, 13 Nov 2015, Frediano Ziglio wrote:
[...]
> >  void mjpeg_encoder_get_stats(MJpegEncoder *encoder, MJpegEncoderStats
> >  *stats)
> >  {
> > -    spice_assert(encoder != NULL && stats != NULL);
> > +    spice_assert(stats != NULL);
> >      stats->starting_bit_rate = encoder->starting_bit_rate;
> >      stats->cur_bit_rate = mjpeg_encoder_get_bit_rate(encoder);
> >      stats->avg_quality = (double)encoder->avg_quality / encoder->num_frames;
> > --
> > 2.6.2
> 
> Personally I think that these kind of checks are not helping that much and
> I would agree. A NULL pointer is a bug but removing the test cause a core
> on modern systems so adding it just slow down the execution and increase code
> size. For the same reason however even the check for stats could be removed
> like many other tests for NULL pointers.
> 
> It's quite question of style subject to personal opinions.

Yes, it's hard to know what to do when writing new code these days:
1) Follow surrounding code and thus use spice_assert(), in particular to 
   document the function's prerequisites.

2) Aknowledge that the server being a library it should not crash 
   the application (which I'm totally on board with, having had this 
   problem with libav), and use the spice_return_if_fail() instead of 
   spice_assert().

3) Still use spice_assert() but only for cases where the function would 
   have crashed anyway. I guess the advantage is that it makes the cause 
   of the crash clear without the need for debugging information in the 
   binary.

4) A mix of spice_assert() (point 3) for cases where Spice has no way 
   to return an error to the caller and would otherwise crash, and 
   spice_return_if_fail() (point 2) for other cases.

5) Consider all of this to drag down performance and use none of them.


For mjpeg_encoder_get_stats() specifically, I think in the end I would 
settle on either:

 * No check: the function is three lines long, it's painfully obvious 
   neither parameter should be NULL. Plus why single out the NULL 
   pointer case when a use after free is probably more likely?

 * Or a single spice_return_if_fail(stat != NULL) which may be more in 
   line with current policy (don't return statistics if not given a 
   place to put them).


-- 
Francois Gouget <fgouget at codeweavers.com>