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

[Frediano Ziglio]

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

actually spice_return_if_fail calls abort by default so it's not a
big change

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

In this case the check for NULL is done in the single call to this
function so another option would be to move this check just inside
the function and call always it but probably is not very coherent
with rest of functions.

Can you post your though on "spice-server, logging and style" thread?

Frediano