[Spice-devel] [PATCH spice-gtk v3 1/6] display-gst: check codec type before creating decoder

[Victor Toso]

Hi,

On Thu, Jun 08, 2017 at 10:39:53AM -0400, Frediano Ziglio wrote:
> > 
> > On Thu, Jun 08, 2017 at 03:55:11PM +0200, Victor Toso wrote:
> > > > 
> > > > Not really worth all that discussion, your patch is ok :)
> > > 
> > > Ah, I liked the discusison mostly because this would be a spice <->
> > > spice-gtk communication bug and I'm not sure either if we should go for
> > > CRITICAL or WARNING messages in such cases.
> > > 
> > > I mean, in the past, for spice server, there was discussion about when
> > > to assert(). The result was that only when it would be a bug inside the
> > > component (never on client's input/configuration for instance)
> > > 
> > > Anyway, thanks for the discussion :)
> > 
> > To tie this in with the logging discussion, you can read the latest
> > comment from ebassi in this blog post about g_log_structured ;)
> > 
> > https://blog.gtk.org/2017/05/04/logging-and-more/
> > 
> > Christophe
> > 
> 
> Wrong thread? Looks like the structured logging would be more
> appropriate in Chistophe D thread.
> 
> About CRITICAL or WARNING personally I think if you are detecting
> that server for some reason is sending garbage telling the used on
> the UI and closing the connection/program is a good solution for
> an user application.

Not really garbage but likely a bug. In this specific case we should
drop the stream and server should handle it gracefully too, no need to
assert or quit the application because of that.

Both warning and critical are fine, I guess. Documentation says:

  "g_return_val_if_reached()) should only be used for programming
  errors, a typical use case is checking for invalid parameters at the
  beginning of a public function. They should not be used if you just
  mean "if (error) return""

I would say that this can be checked as programming error and not just
any-type-of-error, for that reason I'm still keeping the
g_return_if_fail()

  toso

> For the server is a completely different case as the termination
> cause a DoS of the entire VM and also we don't have a user to
> communicate to.
>
> Frediano
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/spice-devel/attachments/20170609/2c4001c4/attachment.sig>