[Spice-devel] [RFC PATCH spice-server v3 10/20] stream-channel: Allows to register callback to get new stream request

Fri Aug 25 16:22:41 UTC 2017

> 
> On Fri, 2017-08-25 at 05:39 -0400, Frediano Ziglio wrote:
> > > 
> > > I had a similar comment in a different patch during the last
> > > version of
> > > the series, but I personally would prefer a signal to handle this
> > > situation. For example, StreamChannel could have a "supported-
> > > codecs"
> > > property, Then when a new client connected, or a client
> > > disconnected,
> > > it would update this property. The StreamDevice would connect to
> > > the
> > > "notify::supported-codecs" signal, and then it would start and stop
> > > the
> > > stream as necessary. I think that encapsulates the logic of
> > > starting
> > > and stopping the stream a little bit better within the device class
> > > as
> > > well.
> > > 
> > > After writing the above paragraph, I did a little more
> > > investigation
> > > and I noticed that DisplayChannel already has a similar design --
> > > it
> > > has a "video-codecs" property.
> > > 
> > > Reviewed-by: Jonathon Jongsma <jjongsma at redhat.com>
> > > 
> > 
> > As long as I can have type safety, static binding and
> > I don't have to write too much I'm not against properties
> > and signal.
> > 
> > As far as I know this is not possible with GObject,
> > at least not in the current C implementation.
> > 
> > Frediano
> 
> Yes, there is a tradeoff between type-safety and convenience when
> you're using C. But we already have plenty of signals and properties in
> other parts of the code. Are you suggesting that we should not have any
> of these in the code because they're not fully type-safe? And if not:
> why should we treat this class differently and refuse to use them here
> when we already use them (for good reasons) elsewhere?
> 

Callbacks are managed in spice-sever in lot of different way
- normal functions + data (like this);
- passing list of functions;
- overriding methods in GObjects;
- signals (very few!).

We limit the usage of property to the constructor case with few
exceptions and signals are limited to "sin" and "video-codecs".

I personally don't like the way of this "signal" implementation.
It removes lot of possibility to the compiler, speed and
make C unsafe than Perl causing bugs that are hard to spot
and fix.

About speed the work that the CPU has to do to extract one
parameter from a g_object_get call is greater that all the
effort to get the property with a function call passing an
object pointer. And I'm not speaking about retrieving the
property value but just the parameter from the caller.

But more concerning is the level of debug that an easy
usage of these features implemented in that way can take.

Code can evolve in such a complicated way, we don't even
know some dark path of "our" code, not taking into account
that dynamic code usually is bound to lot of tools and
testing that are hard to find in low level languages like
C. At the moment as far as I know we don't have much test
coverage to make sure that future evolutions won't cause
crash just calling a function.

And even if we use very few properties and
signals in spice-server we had to fix different nasty
bugs in which in corner cases memory was corrupted
just to pass a parameters.
Is not a coincidence GCC added functions attributes for the
standard "printf" family functions but GObject sometimes
goes way too far from it and as far as I know there is
no such attributes for GObject functions.

About C is not entirely true that is so unsafe.
If I remove a field from a structure the code accessing
it directly fails to compile, not with properties.
Similar if I decide to rename it.
And if I call a function taking an integer passing a
string the compiler give an error or at least a warning.
Callbacks can be defined in a safe way, could take more
time do declare and an amount of macros to do but
the "C" reason is not an excuse.

What's the purpose of using ton of warnings in the
compiler at the level of suggesting the indentation if
then we use a library that hide the errors?

> > 
> > > 
> > > 
> > > On Wed, 2017-08-23 at 10:14 +0100, Frediano Ziglio wrote:
> > > > The channel needs to communicate when it receive a new
> > > > stream request from the guest.
> > > > 
> > > > Signed-off-by: Frediano Ziglio <fziglio at redhat.com>
> > > > ---
> > > >  server/stream-channel.c | 12 ++++++++++++
> > > >  server/stream-channel.h |  6 ++++++
> > > >  2 files changed, 18 insertions(+)
> > > > 
> > > > diff --git a/server/stream-channel.c b/server/stream-channel.c
> > > > index 08c51272..a89a8f8e 100644
> > > > --- a/server/stream-channel.c
> > > > +++ b/server/stream-channel.c
> > > > @@ -68,6 +68,10 @@ struct StreamChannel {
> > > >      int stream_id;
> > > >      /* size of the current video stream */
> > > >      unsigned width, height;
> > > > +
> > > > +    /* callback to notify when a stream should be started or
> > > > stopped
> > > > */
> > > > +    stream_channel_start_proc start_cb;
> > > > +    void *start_opaque;
> > > >  };
> > > >  
> > > >  struct StreamChannelClass {
> > > > @@ -383,3 +387,11 @@ stream_channel_send_data(StreamChannel
> > > > *channel,
> > > > const void *data, size_t size,
> > > >      red_channel_pipes_new_add(red_channel, pipe_item_new_ref,
> > > > item);
> > > >      red_pipe_item_unref(&item->base);
> > > >  }
> > > > +
> > > > +void
> > > > +stream_channel_register_start_cb(StreamChannel *channel,
> > > > +                                 stream_channel_start_proc cb,
> > > > void
> > > > *opaque)
> > > > +{
> > > > +    channel->start_cb = cb;
> > > > +    channel->start_opaque = opaque;
> > > > +}
> > > > diff --git a/server/stream-channel.h b/server/stream-channel.h
> > > > index 6257f806..37b9e327 100644
> > > > --- a/server/stream-channel.h
> > > > +++ b/server/stream-channel.h
> > > > @@ -49,6 +49,7 @@ GType stream_channel_get_type(void)
> > > > G_GNUC_CONST;
> > > >  StreamChannel* stream_channel_new(RedsState *server);
> > > >  
> > > >  struct StreamMsgFormat;
> > > > +struct StreamMsgStartStop;
> > > >  
> > > >  void stream_channel_change_format(StreamChannel *channel,
> > > >                                    const struct StreamMsgFormat
> > > > *fmt);
> > > > @@ -56,6 +57,11 @@ void stream_channel_send_data(StreamChannel
> > > > *channel,
> > > >                                const void *data, size_t size,
> > > >                                uint32_t mm_time);
> > > >  
> > > > +typedef void (*stream_channel_start_proc)(void *opaque, struct
> > > > StreamMsgStartStop *start,
> > > > +                                          StreamChannel
> > > > *channel);
> > > > +void stream_channel_register_start_cb(StreamChannel *channel,
> > > > +                                      stream_channel_start_proc
> > > > cb,
> > > > void *opaque);
> > > > +
> > > >  G_END_DECLS
> > > >  
> > > >  #endif /* STREAM_CHANNEL_H_ */
> 