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

[Christophe Fergeau]

On Sat, Nov 14, 2015 at 10:00:37AM +0100, Francois Gouget wrote:
> 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.

The existing code is doing that, but I'm not sure this has been
recommended on this mailing list in recent years, so definitely not
that.


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

Yes, though g_return_if_fail() is going to be better than
spice_return_if_fail() at this point as the latter calls abort().

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

Yes, it's likely that (unfortunately) there will be some (rare?) cases
where assert() is the best we can easily achieve, but this should be the
exception rather than the norm.

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

I'd make the difference between static function, and non-static ones
rather than looking at the length of the function

Christophe
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/spice-devel/attachments/20151116/bb57f1c3/attachment.sig>