Re: [Qemu-devel] [PATCH][TRIVIAL] spice: fix checking for surface bpp match

Re: [Qemu-devel] [PATCH][TRIVIAL] spice: fix checking for surface bpp match

[Michael Tokarev]

17.09.2015 19:18, Léo Grange wrote:
> Ping, as I had no feedback about this small patch.
> Please tell me if I need to do anything else.

Hello!

I've seen initial version of your patch, thought it isn't exactly trivial ;),
and hoped someone will comment.  Only now I realized you sent it to -trivial
but not to -devel, so there's no one to comment on it.  Please don't do this,
always include qemu-devel when sending patches, so a patch will have a
chance to be reviewed.  I've added qemu-devel to Cc now.

Here, I _think_ the patch is okay, but it'd be better for it to have some
review still.

Thanks,

/mjt

> 2015-08-06 17:36 GMT+02:00 Leo Grange <grangeleo@gmail.com>:
>> Spice surface_switch fast-path was checking only width/height matching,
>> which leads to a crash when surface bpp has been increased but size
>> didn't change (and probably other issues if bpp has been decreased).
>>
>> Signed-off-by: Leo Grange <grangeleo@gmail.com>
>> ---
>>  ui/spice-display.c | 7 ++++---
>>  1 file changed, 4 insertions(+), 3 deletions(-)
>>
>> diff --git a/ui/spice-display.c b/ui/spice-display.c
>> index 0360abf..d1e0972 100644
>> --- a/ui/spice-display.c
>> +++ b/ui/spice-display.c
>> @@ -406,10 +406,11 @@ void qemu_spice_display_switch(SimpleSpiceDisplay *ssd,
>>      SimpleSpiceUpdate *update;
>>      bool need_destroy;
>>
>> -    if (surface && ssd->surface &&
>> +    if (surface && ssd->surface && ssd->ds &&
>>          surface_width(surface) == pixman_image_get_width(ssd->surface) &&
>> -        surface_height(surface) == pixman_image_get_height(ssd->surface)) {
>> -        /* no-resize fast path: just swap backing store */
>> +        surface_height(surface) == pixman_image_get_height(ssd->surface) &&
>> +               surface_bits_per_pixel(surface) == surface_bits_per_pixel(ssd->ds)) {
>> +        /* no-resize and no bpp change fast path: just swap backing store */
>>          dprint(1, "%s/%d: fast (%dx%d)\n", __func__, ssd->qxl.id,
>>                 surface_width(surface), surface_height(surface));
>>          qemu_mutex_lock(&ssd->lock);
>> --
>> 2.5.0
>>
> 

Re: [Qemu-devel] [PATCH][TRIVIAL] spice: fix checking for surface bpp match

[Michael Tokarev]

22.09.2015 10:41, Michael Tokarev wrote:
> 17.09.2015 19:18, Léo Grange wrote:
>> Ping, as I had no feedback about this small patch.
>> Please tell me if I need to do anything else.
> 
> Hello!
> 
> I've seen initial version of your patch, thought it isn't exactly trivial ;),
> and hoped someone will comment.  Only now I realized you sent it to -trivial
> but not to -devel, so there's no one to comment on it.  Please don't do this,
> always include qemu-devel when sending patches, so a patch will have a
> chance to be reviewed.  I've added qemu-devel to Cc now.
> 
> Here, I _think_ the patch is okay, but it'd be better for it to have some
> review still.

Gerd, Cc'ing you this time, can you please take a look at this patch?

Thanks,

/mjt

>> 2015-08-06 17:36 GMT+02:00 Leo Grange <grangeleo@gmail.com>:
>>> Spice surface_switch fast-path was checking only width/height matching,
>>> which leads to a crash when surface bpp has been increased but size
>>> didn't change (and probably other issues if bpp has been decreased).
>>>
>>> Signed-off-by: Leo Grange <grangeleo@gmail.com>
>>> ---
>>>  ui/spice-display.c | 7 ++++---
>>>  1 file changed, 4 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/ui/spice-display.c b/ui/spice-display.c
>>> index 0360abf..d1e0972 100644
>>> --- a/ui/spice-display.c
>>> +++ b/ui/spice-display.c
>>> @@ -406,10 +406,11 @@ void qemu_spice_display_switch(SimpleSpiceDisplay *ssd,
>>>      SimpleSpiceUpdate *update;
>>>      bool need_destroy;
>>>
>>> -    if (surface && ssd->surface &&
>>> +    if (surface && ssd->surface && ssd->ds &&
>>>          surface_width(surface) == pixman_image_get_width(ssd->surface) &&
>>> -        surface_height(surface) == pixman_image_get_height(ssd->surface)) {
>>> -        /* no-resize fast path: just swap backing store */
>>> +        surface_height(surface) == pixman_image_get_height(ssd->surface) &&
>>> +               surface_bits_per_pixel(surface) == surface_bits_per_pixel(ssd->ds)) {
>>> +        /* no-resize and no bpp change fast path: just swap backing store */
>>>          dprint(1, "%s/%d: fast (%dx%d)\n", __func__, ssd->qxl.id,
>>>                 surface_width(surface), surface_height(surface));
>>>          qemu_mutex_lock(&ssd->lock);
>>> --
>>> 2.5.0
>>>
>>
> 
> 

Re: [Qemu-devel] [PATCH][TRIVIAL] spice: fix checking for surface bpp match

[Gerd Hoffmann]

On Do, 2015-10-29 at 11:10 +0300, Michael Tokarev wrote:
> 22.09.2015 10:41, Michael Tokarev wrote:
> > 17.09.2015 19:18, Léo Grange wrote:
> >> Ping, as I had no feedback about this small patch.
> >> Please tell me if I need to do anything else.
> > 
> > Hello!
> > 
> > I've seen initial version of your patch, thought it isn't exactly trivial ;),
> > and hoped someone will comment.  Only now I realized you sent it to -trivial
> > but not to -devel, so there's no one to comment on it.  Please don't do this,
> > always include qemu-devel when sending patches, so a patch will have a
> > chance to be reviewed.  I've added qemu-devel to Cc now.
> > 
> > Here, I _think_ the patch is okay, but it'd be better for it to have some
> > review still.
> 
> Gerd, Cc'ing you this time, can you please take a look at this patch?

Obsolete, commit b2af43cc379e1d4c30d92af257bedebf0e3f618a fixes this
(and a few more cases).

cheers,
  Gerd