Internal images are just read from the surface, compressed, and sent to the client.
Then, they are destroyed. I can't find any reason for aligning their memory.
rhbz#876685
The current lz implementation does not support such bitmaps.
The following patch will actually prevent allocating stride > bpp*width
for internal images.
Previously, there was no check for the size of the message received from
the client, and all messages were read into a buffer of size 1024.
However, migration data can be bigger than 1024. In such cases, memory
corruption occurred.
red_wait_outgoing_item only waits till the currently outgoing msg is
completely sent.
red_wait_outgoing_items does the same for multi-clients. handle_dev_stop erroneously called
red_wait_outgoing_items, instead of waiting till all the items in the
pipes are sent.
This waiting is necessary because after drawables are sent to the client, we release them from the
device. The device might have been stopped due to moving to the non-live
phase of migration. Accessing the device memory during this phase can lead
to inconsistencies.
Also, MSG_MIGRATE should be the last message sent to the client, before
MSG_MIGRATE_DATA. Due to this bug, msgs were marshalled and sent after
handle_dev_stop and after handle_dev_display_migrate which sometimes led
to the release of surfaces, and inserting MSG_DISPLAY_DESTROY_SURFACE
after MSG_MIGRATE.
This patch also removes the calls to red_wait_outgoing_items, from
dev_flush_surfaces. They were unnecessary.
fix: rhbz#866929
At migration destination side, we need to restore the client's surfaces
state, before sending surfaces related messages.
Before this patch, we stopped the processing of only the cmd ring, till migration data
arrived.
However, some QXL_IOs require reading and rendering the cmd ring (e.g.,
update_area). Moreover, when the device is reset, after destroying all
surfaces, we assert (in qemu) if the cmd ring is not empty (see
rhbz#866929).
This fix makes the red_worker thread wait till the migration data arrives
(or till a timeout), and not process any input from the device after the
vm is started.
We try to inject an interrupt to the vm in this case, which we cannot do
if it is stopped. Instead log this and update when vm restarts.
RHBZ: https://bugzilla.redhat.com/show_bug.cgi?id=870972
(that bz is on qemu, it will be cloned or just changed, not
sure yet)
When a new client connects, there may be commands in the ring that it
can't understand, so we need to process these before forwarding new
commands to the client. By doing this after changing the capability
bits we ensure that the new client will never see a command that it
doesn't understand (under the assumption that the guest will read and
obey the capability bits).
Acked-by: Alon Levy <alonl@redhat.com>
A new interface
set_client_capabilities (QXLInstance *qin,
uint8_t client_present,
uint8_t caps[58]);
is added to QXLInstance, and spice server is changed to call it
whenever a client connects or disconnects. The QXL device in response
is expected to update the client capability bits in the ROM of the
device and raise the QXL_INTERRUPT_CLIENT interrupt.
There is a potential race condition in the case where a client
disconnects and a new client with fewer capabilities connects. There
may be commands in the ring that the new client can't handle. This
case is handled by first changing the capability bits, then processing
all commands in the ring, and then start forwarding commands to the
new client. As long as the guest obeys the capability bits, the new
client will never see anything it doesn't understand.
Just checks stride vs width times bpp.
This fixes a potential abort on guest generated bad images in
glz_encoder.
Other files touched to move some consts to red_common, they are
static so no problem to be defined in both red_worker.c and
red_parse_qxl.c .
replace add_ref with add for stack allocated SpiceMigrateDataDisplay.
This fixes wrong MIGRATE_DATA message in display channel (symptom is
glz_encoder_max being way too big, and malloc failure at target) seen on
F18 with gcc-4.7.1-5.fc18.x86_64 and glibc-2.16-8.fc18.x86_64 (didn't
appear on RHEL 6).
Graduality is irrelevant for A8 images, so instead of using RGB-ness
as a short-cut, add a new macro BITMAP_FMT_HAS_GRADUALITY() that
returns true for the existing RGB images, but false for A8.
After marshalling MSG_STREAM_CREATE, there is no need to ref and
unref stream->current before and after completing the sending of the
message (correspondingly). The referencing is unnecessary because all
the data that is required from the drawable (the clipping), is copied
during marshalling, and no field in the drawable is referenced (see
spice_marshall_msg_display_stream_create).
Moreover, the referencing was bugous:
While display_channel_hold_pipe_item and
display_channel_client_release_item_after_push referenced and
dereferenced, correspondingly, stream->current, stream->current might
have changed in between these calls, and then we ended up with one drawable
leaking, and one drawable released before its time has come (which
of course led to critical errors).
a SpiceMsgDisplayMonitorsConfig is sent on two occasions:
* as a result of a spice_qxl_monitors_config_async
* whenever a client connects and there is a previously set monitors
config
Sending the new message is protected by a new cap,
SPICE_DISPLAY_CAP_MONITORS_CONFIG
More elaborately:
spice_qxl_monitors_config_async receives a QXLPHYSICAL address of a
QXLMonitorsConfig struct and reads it, caching it in the RedWorker, and
sending it to all clients. Whenever a new client connects it receives
a SpiceMsgDisplayMonitorsConfig message as well.
Specifically all those that the previous patch converted to spice_debug.
spice_debug contains very verbose stuff like update_area that drowns out
those relatively rare (client connect / disconnect generated) messages.
Resolves: rhbz#824384
red_wait_pipe_item_sent mistakingly returned without waiting for sending the given pipe item
when the channel wasn't blocked. As a result, we failed when we had to
destroy a surface (e.g., QXL_IO_DESTROY_ALL_SURFACES) and to release all
the drawables that are depended on it (by removing them or waiting they will be sent).
In addition, red_wait_pipe_item_sent increased and decreased the reference to the pipe item
using channel_cbs->hold_item, and channel_cbs->release_item. However,
these calls can be called only by red_channel, otherwise
display_channel_client_release_item_before_push is called twice and
leads to a double call to ring_remove(&dpi->base).
Instead ref/put_drawable_pipe_item should be called.
The above routine was risky, since red_channel_client_init_send_data
can also be called with item==NULL. Thus, not all pipe items can be tracked.
The one call that was made for this routine was not necessary.
Resolves: rhbz#820669
Fix a segfault caused by a call to __red_is_next_stream_frame made by
red_stream_maintenance, with a drawable that is not a DRAW_COPY one.
The segfault is a reault of __red_is_next_stream_frame accessing
red_drawable->u.copy.src_bitmap for a non DRAW_COPY drawable.
Simplify keeping count of self_bitmap_image by putting it in
RedDrawable. It is allocated on reading from the command pipe and
deallocated when the last reference to the RedDrawable is dropped,
instead of keeping track of it in GlzDrawable and Drawable.
After the previous patch moving self_bitmap freeing inside red_drawable
ref count, we have a possible self_bitmap leak:
red_process_commands
red_get_drawable | red_drawable #1, red_drawable->self_bitmap == 1
red_process_drawable | rd #2, d #1, d->self_bitmap != NULL
release_drawable | rd #1, d# = 0, try to release self_bitmap, but
blocked by rd #1
put_red_drawable | rd #0
This patch moves the call to release_drawable after the call to
put_red_drawable, thereby fixing the above situation.
