>
> On Tue, 2015-11-24 at 11:13 +0000, Frediano Ziglio wrote:
> > From: Marc-André Lureau <marcandre.lureau@xxxxxxxxx>
> >
> > 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@xxxxxxxxxx>
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).
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).
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);
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;
>
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/spice-devel