Re: [Qemu-devel] [PATCH 38/38] contrib/ivshmem-server: Print "not for production" warning

[Markus Armbruster]

Marc-André Lureau <address@hidden> writes:

> Hi
>
> On Mon, Feb 29, 2016 at 7:40 PM, Markus Armbruster <address@hidden> wrote:
>> The code is okay for illustrating how things work and for testing, but
>> its error handling make it unfit for production use.  Print a warning
>> to protect the innocent.
>>
>> Signed-off-by: Markus Armbruster <address@hidden>
>> ---
>
> I guess David would do something about the problems you report. Tbh, I

That would be nice.

> don't think ivshmem-server is so bad wrt error handling.

ivshmem_server_send_one_msg() returns -1 on error with errno set.  Okay.

ivshmem_server_send_initial_info() fails in turn.
ivshmem_server_handle_new_conn() handles this by closing the connection.
Okay, except for EAGAIN and EINTR.

All other callers ignore ivshmem_server_send_one_msg() failures.  Not
okay.

Here's an example of how things could go haywire:

* The server handles connections one after the other.  It makes the file
  descriptors non-blocking.

* When a client connects, ivshmem-server sends 3 + N*V messages to the
  new client, and V messages to each existing client, where N is the
  number of existing clients, and V is the number of vectors.  Of these,
  only the 3 to the new client are checked for errors.  The unchecked
  messages transmit eventfds for interrupts in groups of V messages.

* With a sufficiently large N*V and a sluggish client, the server can
  conceivably hit EAGAIN.  When it happens, the server drops messages
  silently.

* InterVM interrupts corresponding to dropped eventfds will be silently
  dropped.

* If out a group of V messages any non-trailing messages get dropped,
  the trailing ones get silently miswired to the wrong vector.

Good luck debugging this in the field!

A thorough review of error handling is called for.  Since I can't do
that now, I'm adding the warning.

> Meanwhile:
> Reviewed-by: Marc-André Lureau <address@hidden>

Thanks!