[Qemu-devel] [PATCH] Use sigwait instead og sigwaitinfo.

[Qemu-devel] [PATCH] Use sigwait instead og sigwaitinfo.

[Tristan Gingold]

Fix compilation failure on Darwin.

Signed-off-by: Tristan Gingold <gingold@adacore.com>
---
 compatfd.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/compatfd.c b/compatfd.c
index a7cebc4..5f7f355 100644
--- a/compatfd.c
+++ b/compatfd.c
@@ -33,9 +33,9 @@ static void *sigwait_compat(void *opaque)
     sigprocmask(SIG_BLOCK, &all, NULL);
 
     do {
-        siginfo_t siginfo;
+        int sig;
 
-        err = sigwaitinfo(&info->mask, &siginfo);
+        err = sigwait(&info->mask, &sig);
         if (err == -1 && errno == EINTR) {
             err = 0;
             continue;
-- 
1.7.3.GIT

[Qemu-devel] Re: [PATCH] Use sigwait instead og sigwaitinfo.

[Paolo Bonzini]

On 02/17/2011 05:24 PM, Tristan Gingold wrote:
> -        siginfo_t siginfo;
> +        int sig;
>
> -        err = sigwaitinfo(&info->mask,&siginfo);
> +        err = sigwait(&info->mask,&sig);

This is doable but the patch is wrong, after sigwaitinfo "err" is the 
signal number, so you should replace

             memcpy(buffer, &err, sizeof(err));

further down with

             memcpy(buffer, &sig, sizeof(sig));

Paolo

[Qemu-devel] [PATCH] Use sigwait instead of sigwaitinfo.

[Tristan Gingold]

Fix compilation failure on Darwin.

Signed-off-by: Tristan Gingold <gingold@adacore.com>
---
 compatfd.c |   36 ++++++++++++++++++------------------
 1 files changed, 18 insertions(+), 18 deletions(-)

diff --git a/compatfd.c b/compatfd.c
index a7cebc4..bd377c4 100644
--- a/compatfd.c
+++ b/compatfd.c
@@ -26,45 +26,45 @@ struct sigfd_compat_info
 static void *sigwait_compat(void *opaque)
 {
     struct sigfd_compat_info *info = opaque;
-    int err;
     sigset_t all;
 
     sigfillset(&all);
     sigprocmask(SIG_BLOCK, &all, NULL);
 
-    do {
-        siginfo_t siginfo;
+    while (1) {
+        int sig;
+        int err;
 
-        err = sigwaitinfo(&info->mask, &siginfo);
-        if (err == -1 && errno == EINTR) {
-            err = 0;
-            continue;
-        }
-
-        if (err > 0) {
-            char buffer[128];
+        err = sigwait(&info->mask, &sig);
+        if (err != 0) {
+            if (errno == EINTR) {
+                continue;
+            } else {
+                return NULL;
+            }
+        } else {
+            struct qemu_signalfd_siginfo buffer;
             size_t offset = 0;
 
-            memcpy(buffer, &err, sizeof(err));
+            memset(&buffer, 0, sizeof(buffer));
+            buffer.ssi_signo = sig;
+
             while (offset < sizeof(buffer)) {
                 ssize_t len;
 
-                len = write(info->fd, buffer + offset,
+                len = write(info->fd, (char *)&buffer + offset,
                             sizeof(buffer) - offset);
                 if (len == -1 && errno == EINTR)
                     continue;
 
                 if (len <= 0) {
-                    err = -1;
-                    break;
+                    return NULL;
                 }
 
                 offset += len;
             }
         }
-    } while (err >= 0);
-
-    return NULL;
+    }
 }
 
 static int qemu_signalfd_compat(const sigset_t *mask)
-- 
1.7.3.GIT

[Qemu-devel] Re: [PATCH] Use sigwait instead of sigwaitinfo.

[Paolo Bonzini]

On 02/18/2011 02:17 PM, Tristan Gingold wrote:
> Fix compilation failure on Darwin.
>
> Signed-off-by: Tristan Gingold<gingold@adacore.com>
> ---
>   compatfd.c |   36 ++++++++++++++++++------------------
>   1 files changed, 18 insertions(+), 18 deletions(-)
>
> diff --git a/compatfd.c b/compatfd.c
> index a7cebc4..bd377c4 100644
> --- a/compatfd.c
> +++ b/compatfd.c
> @@ -26,45 +26,45 @@ struct sigfd_compat_info
>   static void *sigwait_compat(void *opaque)
>   {
>       struct sigfd_compat_info *info = opaque;
> -    int err;
>       sigset_t all;
>
>       sigfillset(&all);
>       sigprocmask(SIG_BLOCK,&all, NULL);
>
> -    do {
> -        siginfo_t siginfo;
> +    while (1) {
> +        int sig;
> +        int err;
>
> -        err = sigwaitinfo(&info->mask,&siginfo);
> -        if (err == -1&&  errno == EINTR) {
> -            err = 0;
> -            continue;
> -        }
> -
> -        if (err>  0) {
> -            char buffer[128];
> +        err = sigwait(&info->mask,&sig);
> +        if (err != 0) {
> +            if (errno == EINTR) {
> +                continue;
> +            } else {
> +                return NULL;
> +            }
> +        } else {
> +            struct qemu_signalfd_siginfo buffer;
>               size_t offset = 0;
>
> -            memcpy(buffer,&err, sizeof(err));
> +            memset(&buffer, 0, sizeof(buffer));
> +            buffer.ssi_signo = sig;
> +
>               while (offset<  sizeof(buffer)) {
>                   ssize_t len;
>
> -                len = write(info->fd, buffer + offset,
> +                len = write(info->fd, (char *)&buffer + offset,
>                               sizeof(buffer) - offset);
>                   if (len == -1&&  errno == EINTR)
>                       continue;
>
>                   if (len<= 0) {
> -                    err = -1;
> -                    break;
> +                    return NULL;
>                   }
>
>                   offset += len;
>               }
>           }
> -    } while (err>= 0);
> -
> -    return NULL;
> +    }
>   }
>
>   static int qemu_signalfd_compat(const sigset_t *mask)

Looks good.

Paolo

[Qemu-devel] Re: [PATCH] Use sigwait instead of sigwaitinfo.

[Jan Kiszka]

On 2011-02-18 14:17, Tristan Gingold wrote:
> Fix compilation failure on Darwin.
> 
> Signed-off-by: Tristan Gingold <gingold@adacore.com>
> ---
>  compatfd.c |   36 ++++++++++++++++++------------------
>  1 files changed, 18 insertions(+), 18 deletions(-)
> 
> diff --git a/compatfd.c b/compatfd.c
> index a7cebc4..bd377c4 100644
> --- a/compatfd.c
> +++ b/compatfd.c
> @@ -26,45 +26,45 @@ struct sigfd_compat_info
>  static void *sigwait_compat(void *opaque)
>  {
>      struct sigfd_compat_info *info = opaque;
> -    int err;
>      sigset_t all;
>  
>      sigfillset(&all);
>      sigprocmask(SIG_BLOCK, &all, NULL);
>  
> -    do {
> -        siginfo_t siginfo;
> +    while (1) {
> +        int sig;
> +        int err;
>  
> -        err = sigwaitinfo(&info->mask, &siginfo);
> -        if (err == -1 && errno == EINTR) {
> -            err = 0;
> -            continue;
> -        }
> -
> -        if (err > 0) {
> -            char buffer[128];
> +        err = sigwait(&info->mask, &sig);
> +        if (err != 0) {
> +            if (errno == EINTR) {
> +                continue;
> +            } else {
> +                return NULL;
> +            }
> +        } else {
> +            struct qemu_signalfd_siginfo buffer;
>              size_t offset = 0;
>  
> -            memcpy(buffer, &err, sizeof(err));
> +            memset(&buffer, 0, sizeof(buffer));
> +            buffer.ssi_signo = sig;
> +
>              while (offset < sizeof(buffer)) {
>                  ssize_t len;
>  
> -                len = write(info->fd, buffer + offset,
> +                len = write(info->fd, (char *)&buffer + offset,
>                              sizeof(buffer) - offset);
>                  if (len == -1 && errno == EINTR)
>                      continue;
>  
>                  if (len <= 0) {
> -                    err = -1;
> -                    break;
> +                    return NULL;

This and the above handling of sigwait return codes changes the error
handling strategy. So far we silently skipped errors, now we silently
terminate the compatfd thread. I think none of both approaches is good.

Failing sigwait is likely a reason to bail out, but loudly, writing some
error message to the console and triggering a shutdown of qemu.

An overflow of the compatfd pipe to the main thread may be due to some
very unfortunate overload scenario. Not sure if that qualifies for a
thread termination (definitely not for a silent one).

Error handling should probably be adjusted independently of this darwin
build fix.

Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

[Qemu-devel] Re: [PATCH] Use sigwait instead of sigwaitinfo.

[Tristan Gingold]

On Feb 18, 2011, at 4:34 PM, Jan Kiszka wrote:
> 
> This and the above handling of sigwait return codes changes the error
> handling strategy.

Did it ?  I don't think so.

> So far we silently skipped errors, now we silently
> terminate the compatfd thread. I think none of both approaches is good.

I think that both silently terminate the compatfd.  The previous code is:

  do {
        siginfo_t siginfo;

        err = sigwaitinfo(&info->mask, &siginfo);
        if (err == -1 && errno == EINTR) {
            err = 0;
            continue;
        }

        if (err > 0) {
            char buffer[128];
            size_t offset = 0;

            memcpy(buffer, &err, sizeof(err));
            while (offset < sizeof(buffer)) {
                ssize_t len;

                len = write(info->fd, buffer + offset,
                            sizeof(buffer) - offset);
                if (len == -1 && errno == EINTR)
                    continue;

                if (len <= 0) {
                    err = -1;
                    break;
                }

                offset += len;
            }
        }
    } while (err >= 0);

So in case of any error, err is set to a negative value which exits the thread.

> Failing sigwait is likely a reason to bail out, but loudly, writing some
> error message to the console and triggering a shutdown of qemu.

I agree with that.

> An overflow of the compatfd pipe to the main thread may be due to some
> very unfortunate overload scenario. Not sure if that qualifies for a
> thread termination (definitely not for a silent one).

What do you mean by overflow ?  Unless I am wrong, the fd is not non-blocking, write will block.

> Error handling should probably be adjusted independently of this darwin
> build fix.

I agree too.

Tristan.

[Qemu-devel] Re: [PATCH] Use sigwait instead of sigwaitinfo.

[Jan Kiszka]

On 2011-02-18 16:50, Tristan Gingold wrote:
> 
> On Feb 18, 2011, at 4:34 PM, Jan Kiszka wrote:
>>
>> This and the above handling of sigwait return codes changes the error
>> handling strategy.
> 
> Did it ?  I don't think so.
> 
>> So far we silently skipped errors, now we silently
>> terminate the compatfd thread. I think none of both approaches is good.
> 
> I think that both silently terminate the compatfd.  The previous code is:
> 
>   do {
>         siginfo_t siginfo;
> 
>         err = sigwaitinfo(&info->mask, &siginfo);
>         if (err == -1 && errno == EINTR) {
>             err = 0;
>             continue;
>         }
> 
>         if (err > 0) {
>             char buffer[128];
>             size_t offset = 0;
> 
>             memcpy(buffer, &err, sizeof(err));
>             while (offset < sizeof(buffer)) {
>                 ssize_t len;
> 
>                 len = write(info->fd, buffer + offset,
>                             sizeof(buffer) - offset);
>                 if (len == -1 && errno == EINTR)
>                     continue;
> 
>                 if (len <= 0) {
>                     err = -1;
>                     break;
>                 }
> 
>                 offset += len;
>             }
>         }
>     } while (err >= 0);
> 
> So in case of any error, err is set to a negative value which exits the thread.

Ah, sorry, oversaw that. In that case the patch is fine as it does not
change the existing behavior.

> 
>> Failing sigwait is likely a reason to bail out, but loudly, writing some
>> error message to the console and triggering a shutdown of qemu.
> 
> I agree with that.
> 
>> An overflow of the compatfd pipe to the main thread may be due to some
>> very unfortunate overload scenario. Not sure if that qualifies for a
>> thread termination (definitely not for a silent one).
> 
> What do you mean by overflow ?  Unless I am wrong, the fd is not non-blocking, write will block.

True. So any error returned here is a real one.

Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

Re: [Qemu-devel] [PATCH] Use sigwait instead of sigwaitinfo.

[Blue Swirl]

On Fri, Feb 18, 2011 at 3:17 PM, Tristan Gingold <gingold@adacore.com> wrote:
> Fix compilation failure on Darwin.

Also OpenBSD needs this patch.

Re: [Qemu-devel] [PATCH] Use sigwait instead of sigwaitinfo.

[Blue Swirl]

Thanks, applied.

On Fri, Feb 18, 2011 at 3:17 PM, Tristan Gingold <gingold@adacore.com> wrote:
> Fix compilation failure on Darwin.
>
> Signed-off-by: Tristan Gingold <gingold@adacore.com>
> ---
>  compatfd.c |   36 ++++++++++++++++++------------------
>  1 files changed, 18 insertions(+), 18 deletions(-)
>
> diff --git a/compatfd.c b/compatfd.c
> index a7cebc4..bd377c4 100644
> --- a/compatfd.c
> +++ b/compatfd.c
> @@ -26,45 +26,45 @@ struct sigfd_compat_info
>  static void *sigwait_compat(void *opaque)
>  {
>     struct sigfd_compat_info *info = opaque;
> -    int err;
>     sigset_t all;
>
>     sigfillset(&all);
>     sigprocmask(SIG_BLOCK, &all, NULL);
>
> -    do {
> -        siginfo_t siginfo;
> +    while (1) {
> +        int sig;
> +        int err;
>
> -        err = sigwaitinfo(&info->mask, &siginfo);
> -        if (err == -1 && errno == EINTR) {
> -            err = 0;
> -            continue;
> -        }
> -
> -        if (err > 0) {
> -            char buffer[128];
> +        err = sigwait(&info->mask, &sig);
> +        if (err != 0) {
> +            if (errno == EINTR) {
> +                continue;
> +            } else {
> +                return NULL;
> +            }
> +        } else {
> +            struct qemu_signalfd_siginfo buffer;
>             size_t offset = 0;
>
> -            memcpy(buffer, &err, sizeof(err));
> +            memset(&buffer, 0, sizeof(buffer));
> +            buffer.ssi_signo = sig;
> +
>             while (offset < sizeof(buffer)) {
>                 ssize_t len;
>
> -                len = write(info->fd, buffer + offset,
> +                len = write(info->fd, (char *)&buffer + offset,
>                             sizeof(buffer) - offset);
>                 if (len == -1 && errno == EINTR)
>                     continue;
>
>                 if (len <= 0) {
> -                    err = -1;
> -                    break;
> +                    return NULL;
>                 }
>
>                 offset += len;
>             }
>         }
> -    } while (err >= 0);
> -
> -    return NULL;
> +    }
>  }
>
>  static int qemu_signalfd_compat(const sigset_t *mask)
> --
> 1.7.3.GIT
>
>
>