[Spice-devel] [PATCH] Limit maximum "n-surfaces" via param spec

[Frediano Ziglio]

> 
> On Fri, 2016-10-28 at 06:46 -0400, Frediano Ziglio wrote:
> > > 
> > > 
> > > In commit beec1b41, we manually limited this property value in
> > > _set_property(). But there's a simpler way to do it: via the param
> > > spec
> > > for the property.
> > > 
> > > This also means that we can remove the warning log in
> > > red_worker_new()
> > > since GObject will automatically warn if a property is assigned a
> > > value
> > > outside of its valid range.
> > > ---
> > > 
> > > I know the earlier commit was already pushed, but I just realized a
> > > simpler
> > > way
> > > to fix this issue. What do you think?
> > > 
> > >  server/display-channel.c | 3 +--
> > >  server/red-worker.c      | 2 --
> > >  2 files changed, 1 insertion(+), 4 deletions(-)
> > > 
> > > diff --git a/server/display-channel.c b/server/display-channel.c
> > > index bcf26bb..b838ba6 100644
> > > --- a/server/display-channel.c
> > > +++ b/server/display-channel.c
> > > @@ -60,7 +60,6 @@ display_channel_set_property(GObject *object,
> > >      {
> > >          case PROP_N_SURFACES:
> > >              self->priv->n_surfaces = g_value_get_uint(value);
> > > -            self->priv->n_surfaces = MIN(self->priv->n_surfaces,
> > > NUM_SURFACES);
> > >              break;
> > >          case PROP_VIDEO_CODECS:
> > >              if (self->priv->video_codecs) {
> > > @@ -2232,7 +2231,7 @@
> > > display_channel_class_init(DisplayChannelClass *klass)
> > >                                      g_param_spec_uint("n-
> > > surfaces",
> > >                                                        "number of
> > > surfaces",
> > >                                                        "Number of
> > > surfaces
> > >                                                        for this
> > > channel",
> > > -                                                      0,
> > > G_MAXUINT,
> > > +                                                      0,
> > > NUM_SURFACES,
> > >                                                        0,
> > >                                                        G_PARAM_CONS
> > > TRUCT_ONLY
> > >                                                        |
> > >                                                        G_PARAM_READ
> > > WRITE |
> > > diff --git a/server/red-worker.c b/server/red-worker.c
> > > index f7f0726..2ed2453 100644
> > > --- a/server/red-worker.c
> > > +++ b/server/red-worker.c
> > > @@ -1358,8 +1358,6 @@ RedWorker* red_worker_new(QXLInstance *qxl,
> > >                        init_info.memslot_id_bits,
> > >                        init_info.internal_groupslot_id);
> > >  
> > > -    spice_warn_if_fail(init_info.n_surfaces <= NUM_SURFACES);
> > > -
> > >      worker->event_timeout = INF_EVENT_WAIT;
> > >  
> > >      worker->cursor_channel = cursor_channel_new(reds, qxl,
> > 
> > Sounds good. What happens to the property? Does the value
> > get capped or just discarded?
> 
> The invalid value is discarded and a critical warning is issued.
> 
> > Not that Qemu should try to set crazy values but if you set 20000
> > and got 0 perhaps would be better to get the maximum (10000).
> > Surely better than giving a warning and then having possible
> > overflows (as the current code does!).
> 
> Well, at the moment, we set the default value for that parameter to 0.
> But we could also change the default to make it something more
> reasonable (perhaps the maximum?). Would that be sufficient, or would
> you still prefer to keep your existing "capped" approach?
> 
> Jonathon
> 

Would be fine.
About ranges I think the minimum should be 1 so to allow to
have a primary surface (always 0!).

Frediano