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

[Jonathon Jongsma]

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