| Message ID | 1298035036-51807-1-git-send-email-gingold@adacore.com |
|---|---|
| State | New |
| Headers | show |
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
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
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.
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
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.
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 > > >
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)
Fix compilation failure on Darwin. Signed-off-by: Tristan Gingold <gingold@adacore.com> --- compatfd.c | 36 ++++++++++++++++++------------------ 1 files changed, 18 insertions(+), 18 deletions(-)