| Message ID | 1456771254-17511-39-git-send-email-armbru@redhat.com |
|---|---|
| State | New |
| Headers | show |
Hi On Mon, Feb 29, 2016 at 7:40 PM, Markus Armbruster <armbru@redhat.com> 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 <armbru@redhat.com> > --- I guess David would do something about the problems you report. Tbh, I don't think ivshmem-server is so bad wrt error handling. Meanwhile: Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com> > contrib/ivshmem-server/main.c | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/contrib/ivshmem-server/main.c b/contrib/ivshmem-server/main.c > index cca1061..97488dc 100644 > --- a/contrib/ivshmem-server/main.c > +++ b/contrib/ivshmem-server/main.c > @@ -197,6 +197,12 @@ main(int argc, char *argv[]) > }; > int ret = 1; > > + /* > + * Do not remove this notice without adding proper error handling! > + * Start with handling ivshmem_server_send_one_msg() failure. > + */ > + printf("*** Example code, do not use in production ***\n"); > + > /* parse arguments, will exit on error */ > ivshmem_server_parse_args(&args, argc, argv); > > -- > 2.4.3 > >
Marc-André Lureau <marcandre.lureau@gmail.com> writes: > Hi > > On Mon, Feb 29, 2016 at 7:40 PM, Markus Armbruster <armbru@redhat.com> 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 <armbru@redhat.com> >> --- > > 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 <marcandre.lureau@redhat.com> Thanks!
diff --git a/contrib/ivshmem-server/main.c b/contrib/ivshmem-server/main.c index cca1061..97488dc 100644 --- a/contrib/ivshmem-server/main.c +++ b/contrib/ivshmem-server/main.c @@ -197,6 +197,12 @@ main(int argc, char *argv[]) }; int ret = 1; + /* + * Do not remove this notice without adding proper error handling! + * Start with handling ivshmem_server_send_one_msg() failure. + */ + printf("*** Example code, do not use in production ***\n"); + /* parse arguments, will exit on error */ ivshmem_server_parse_args(&args, argc, argv);
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 <armbru@redhat.com> --- contrib/ivshmem-server/main.c | 6 ++++++ 1 file changed, 6 insertions(+)