Re: [Qemu-devel] [PATCH 4/4] qga/channel-posix: provide Solaris alternative to O_ASYNC

["Andreas Färber" <andreas.faerber@web.de>]

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 <lee.essen@nowonline.co.uk>
>> ---
>>  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 <sys/stropts.h>
>> +#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 <pbonzini@redhat.com>

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