[Spice-devel] [PATCH 18/18] worker: remove redundant check

[Pavel Grunt]

On Wed, 2015-11-25 at 09:30 -0500, Frediano Ziglio wrote:
> > 
> > On Tue, 2015-11-24 at 11:13 +0000, Frediano Ziglio wrote:
> > > From: Marc-André Lureau <marcandre.lureau at gmail.com>
> > > 
> > > The check is done in draw()
> > I don't see it in display_channel_draw(), it also prevented calling
> > notify_update().
> > 
> > I think the patch should be dropped.
> > 
> > Pavel
> > 
> > Reviewed-by: Pavel Grunt <pgrunt at redhat.com>
> 
> I'm not really sure. Surely is changing the behavior as you stated.
> There is some checks in the draw function but I like to keep calling
> the validate function (either outside or inside).

Yes, there are some aborting checks instead of the warning, I agree that the
validate function should be called.
> 
> About the notify you made a good point. What about if the guest driver
> for some mistake (like a race) send a draw command with a wrong
> surface_id? Probably not sending the notify is fine, the server didn't
> do any update however even the release_resource is skipped.
> Also red_put_update_cmd is not called while it should (at the moment is
> empty but is not the point).
right
> 
> I think the code
> 
>             RedUpdateCmd update;
>             QXLReleaseInfoExt release_info_ext;
> 
>             if (red_get_update_cmd(&worker->mem_slots, ext_cmd.group_id,
>                                    &update, ext_cmd.cmd.data)) {
>                 break;
>             }
>             if (!validate_surface(worker->display_channel, update.surface_id))
> {
>                 spice_warning("Invalid surface in QXL_CMD_UPDATE");
>                 break;
>             }
>             display_channel_draw(worker->display_channel, &update.area,
> update.surface_id);
>             worker->qxl->st->qif->notify_update(worker->qxl,
> update.update_id);
>             release_info_ext.group_id = ext_cmd.group_id;
>             release_info_ext.info = update.release_info;
>             worker->qxl->st->qif->release_resource(worker->qxl,
> release_info_ext);
>             red_put_update_cmd(&update);
> 
> Could be improved with
> 
>             RedUpdateCmd update;
>             QXLReleaseInfoExt release_info_ext;
> 
>             if (red_get_update_cmd(&worker->mem_slots, ext_cmd.group_id,
>                                    &update, ext_cmd.cmd.data)) {
>                 break;
>             }
>             if (validate_surface(worker->display_channel, update.surface_id))
> {
>                 display_channel_draw(worker->display_channel, &update.area,
> update.surface_id);
>                 worker->qxl->st->qif->notify_update(worker->qxl,
> update.update_id);
>             }
>             release_info_ext.group_id = ext_cmd.group_id;
>             release_info_ext.info = update.release_info;
>             worker->qxl->st->qif->release_resource(worker->qxl,
> release_info_ext);
>             red_put_update_cmd(&update);
> 
> 
Looks good, 

Pavel

> Frediano
> 
> > > ---
> > >  server/red_worker.c | 4 ----
> > >  1 file changed, 4 deletions(-)
> > > 
> > > diff --git a/server/red_worker.c b/server/red_worker.c
> > > index 4cc9fe9..a9fbd64 100644
> > > --- a/server/red_worker.c
> > > +++ b/server/red_worker.c
> > > @@ -647,10 +647,6 @@ static int red_process_commands(RedWorker *worker,
> > > uint32_t max_pipe_size, int *
> > >                                     &update, ext_cmd.cmd.data)) {
> > >                  break;
> > >              }
> > > -            if (!validate_surface(worker->display_channel,
> > > update.surface_id)) {
> > > -                spice_warning("Invalid surface in QXL_CMD_UPDATE");
> > > -                break;
> > > -            }
> > >              display_channel_draw(worker->display_channel, &update.area,
> > > update.surface_id);
> > >              worker->qxl->st->qif->notify_update(worker->qxl,
> > > update.update_id);
> > >              release_info_ext.group_id = ext_cmd.group_id;
> >