From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:45937) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1SCY4b-0004aa-7r for qemu-devel@nongnu.org; Tue, 27 Mar 2012 11:12:43 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1SCY4U-0002Mh-U7 for qemu-devel@nongnu.org; Tue, 27 Mar 2012 11:12:36 -0400 Received: from mout.web.de ([212.227.15.3]:52848) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1SCY4U-0002MH-L8 for qemu-devel@nongnu.org; Tue, 27 Mar 2012 11:12:30 -0400 Message-ID: <4F71D8D9.9070109@web.de> Date: Tue, 27 Mar 2012 17:12:25 +0200 From: =?ISO-8859-15?Q?Andreas_F=E4rber?= MIME-Version: 1.0 References: <1332606390-3605-1-git-send-email-lee.essen@nowonline.co.uk> <1332606390-3605-4-git-send-email-lee.essen@nowonline.co.uk> <4F71D530.30302@redhat.com> In-Reply-To: <4F71D530.30302@redhat.com> Content-Type: text/plain; charset=ISO-8859-15 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 4/4] qga/channel-posix: provide Solaris alternative to O_ASYNC List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Paolo Bonzini , Lee Essen Cc: Blue Swirl , qemu-devel@nongnu.org Am 27.03.2012 16:56, schrieb Paolo Bonzini: > Il 24/03/2012 17:26, Lee Essen ha scritto: >> Solaris does not support the O_ASYNC option to open, this patch >> adds the same functionality through the I_SETSIG ioctl. >> >> Signed-off-by: Lee Essen >> --- >> qga/channel-posix.c | 16 ++++++++++++++++ >> 1 files changed, 16 insertions(+), 0 deletions(-) >> >> diff --git a/qga/channel-posix.c b/qga/channel-posix.c >> index 40f7658..86245c1 100644 >> --- a/qga/channel-posix.c >> +++ b/qga/channel-posix.c >> @@ -3,6 +3,10 @@ >> #include "qemu_socket.h" >> #include "qga/channel.h" >> >> +#ifdef CONFIG_SOLARIS >> +#include >> +#endif >> + >> #define GA_CHANNEL_BAUDRATE_DEFAULT B38400 /* for isa-serial channels */ >> >> struct GAChannel { >> @@ -123,7 +127,19 @@ static gboolean ga_channel_open(GAChannel *c, const gchar *path, GAChannelMethod >> >> switch (c->method) { >> case GA_CHANNEL_VIRTIO_SERIAL: { >> +#ifdef CONFIG_SOLARIS >> + int fd = qemu_open(path, O_RDWR | O_NONBLOCK); >> + if (fd == -1) { >> + g_critical("error opening channel: %s", strerror(errno)); >> + exit(EXIT_FAILURE); >> + } >> + if (ioctl(fd, I_SETSIG, S_OUTPUT | S_INPUT | S_HIPRI) < 0) { >> + g_critical("error with setsig on channel: %s", strerror(errno)); >> + exit(EXIT_FAILURE); >> + } >> +#else >> int fd = qemu_open(path, O_RDWR | O_NONBLOCK | O_ASYNC); >> +#endif >> if (fd == -1) { >> g_critical("error opening channel: %s", strerror(errno)); >> exit(EXIT_FAILURE); > > Reviewed-by: Paolo Bonzini I would like to see this improved: This unnecessarily duplicates code into a rarely tested code path, we have the fd == -1 check twice in the suggested Solaris code path above. I would rather have the ioctl in a separate ifdef section below to avoid that. For O_ASYNC someone previously suggested to #define O_ASYNC for Solaris, for which I have an experimental osdep.h patch in my VM. The two options were either 0 or O_NDELAY(?) iirc; don't like the former and not sure about the semantics of the latter. If that's no longer desired, we can at least restrict the #ifdeffery to | O_ASYNC by breaking the line. Andreas