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@xxxxxxxxxxxxxxx>
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/spice-devel