Re: [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


signature.asc
Description: PGP signature
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel

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

[Christophe Fergeau]

On Fri, Nov 13, 2015 at 04:14:17PM +0100, Francois Gouget wrote:
> mjpeg_encoder_get_stats() was the only function to check it.

Could you trigger this assert in a somehow legit case? Or is this just
for consistency?

Christophe


signature.asc
Description: PGP signature
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [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
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel

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

[Francois Gouget]

On Mon, 16 Nov 2015, Christophe Fergeau wrote:

> On Fri, Nov 13, 2015 at 04:14:17PM +0100, Francois Gouget wrote:
> > mjpeg_encoder_get_stats() was the only function to check it.
> 
> Could you trigger this assert in a somehow legit case? Or is this just
> for consistency?

It's for consistency and also as a way to figure out what to do for the 
GStreamer encoder without having to send the whole series for each 
iteration.

-- 
Francois Gouget 
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [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 
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel

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

[Francois Gouget]

mjpeg_encoder_get_stats() was the only function to check it.

Signed-off-by: Francois Gouget 
---
 server/mjpeg_encoder.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/server/mjpeg_encoder.c b/server/mjpeg_encoder.c
index c8253a7..84a0078 100644
--- a/server/mjpeg_encoder.c
+++ b/server/mjpeg_encoder.c
@@ -1333,7 +1333,7 @@ uint64_t mjpeg_encoder_get_bit_rate(MJpegEncoder *encoder)
 
 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
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel

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

[Frediano Ziglio]

> 
> mjpeg_encoder_get_stats() was the only function to check it.
> 
> Signed-off-by: Francois Gouget 
> ---
>  server/mjpeg_encoder.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/server/mjpeg_encoder.c b/server/mjpeg_encoder.c
> index c8253a7..84a0078 100644
> --- a/server/mjpeg_encoder.c
> +++ b/server/mjpeg_encoder.c
> @@ -1333,7 +1333,7 @@ uint64_t mjpeg_encoder_get_bit_rate(MJpegEncoder
> *encoder)
>  
>  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.

If nobody will disagree I will merge it. Please remember me if I forget.

Frediano
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel