[Spice-devel] [PATCH 02/18] Print warnings on untested code paths

[Jonathon Jongsma]

On Thu, 2016-04-28 at 13:02 -0400, Frediano Ziglio wrote:
> > 
> > ---
> >  server/display-channel.c | 3 +++
> >  server/sound.c           | 1 +
> >  2 files changed, 4 insertions(+)
> > 
> > diff --git a/server/display-channel.c b/server/display-channel.c
> > index 1f4d66f..3a06305 100644
> > --- a/server/display-channel.c
> > +++ b/server/display-channel.c
> > @@ -1921,6 +1921,9 @@ void display_channel_create_surface(DisplayChannel
> > *display, uint32_t surface_id
> >          QXLInstance *qxl = display->common.qxl;
> >          RedsState *reds = red_qxl_get_server(qxl->st);
> >          GArray *renderers = reds_get_renderers(reds);
> > +        /* These days, noone is trying to use multiple renderers, the
> > software one
> > +         * is always used */
> > +        g_warn_if_fail(renderers->len == 1);
> >          for (i = 0; i < renderers->len; i++) {
> >              uint32_t renderer = g_array_index(renderers, uint32_t, i);
> >              surface->context.canvas = create_canvas_for_surface(display,
> >              surface, renderer);
> > diff --git a/server/sound.c b/server/sound.c
> > index aae841c..b95e7e7 100644
> > --- a/server/sound.c
> > +++ b/server/sound.c
> > @@ -1620,6 +1620,7 @@ void snd_set_playback_compression(int on)
> >      SndWorker *now = workers;
> >  
> >      for (; now; now = now->next) {
> > +        g_critical("untested code path");
> >          if (now->base_channel->type == SPICE_CHANNEL_PLAYBACK &&
> >          now->connection) {
> >              PlaybackChannel* playback = (PlaybackChannel*)now->connection;
> >              SpicePlaybackState *st = SPICE_CONTAINEROF(now,
> >              SpicePlaybackState, worker);
> 
> I would prefer a better option for this last.
> The function is not currently called by Qemu.

That's not quite true. it is called from qemu_spice_init(). When called at this
point, no sound workers have been created yet, so the code within the loop does
not get executed. So the only effect of qemu calling the function is to set the
value of reds->config->streaming_video, which is then later used when the sound
workers are actually created.

> We could deprecate and make it do nothing. It's just disabling the sound
> compression (which is on by default) and anyway the compression are also
> handled by capabilities.
> Even better would be writing a test for this function.

Yes, it should really have a test. My suggestion would be to move this patch way
to the end of the branch and deal with it later after all other patches have
been merged.

> 
> Frediano