* [Qemu-devel] Re: [PATCH] Use sigwait instead of sigwaitinfo.
2011-02-18 13:17 ` [Qemu-devel] [PATCH] Use sigwait instead of sigwaitinfo Tristan Gingold
@ 2011-02-18 14:09 ` Paolo Bonzini
2011-02-18 15:34 ` Jan Kiszka
` (2 subsequent siblings)
3 siblings, 0 replies; 9+ messages in thread
From: Paolo Bonzini @ 2011-02-18 14:09 UTC (permalink / raw)
To: Tristan Gingold; +Cc: qemu-devel
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
^ permalink raw reply [flat|nested] 9+ messages in thread* [Qemu-devel] Re: [PATCH] Use sigwait instead of sigwaitinfo.
2011-02-18 13:17 ` [Qemu-devel] [PATCH] Use sigwait instead of sigwaitinfo Tristan Gingold
2011-02-18 14:09 ` [Qemu-devel] " Paolo Bonzini
@ 2011-02-18 15:34 ` Jan Kiszka
2011-02-18 15:50 ` Tristan Gingold
2011-02-18 18:15 ` [Qemu-devel] " Blue Swirl
2011-02-25 20:58 ` Blue Swirl
3 siblings, 1 reply; 9+ messages in thread
From: Jan Kiszka @ 2011-02-18 15:34 UTC (permalink / raw)
To: Tristan Gingold; +Cc: Paolo Bonzini, qemu-devel
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
^ permalink raw reply [flat|nested] 9+ messages in thread* [Qemu-devel] Re: [PATCH] Use sigwait instead of sigwaitinfo.
2011-02-18 15:34 ` Jan Kiszka
@ 2011-02-18 15:50 ` Tristan Gingold
2011-02-18 16:16 ` Jan Kiszka
0 siblings, 1 reply; 9+ messages in thread
From: Tristan Gingold @ 2011-02-18 15:50 UTC (permalink / raw)
To: Jan Kiszka; +Cc: Paolo Bonzini, qemu-devel
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.
^ permalink raw reply [flat|nested] 9+ messages in thread* [Qemu-devel] Re: [PATCH] Use sigwait instead of sigwaitinfo.
2011-02-18 15:50 ` Tristan Gingold
@ 2011-02-18 16:16 ` Jan Kiszka
0 siblings, 0 replies; 9+ messages in thread
From: Jan Kiszka @ 2011-02-18 16:16 UTC (permalink / raw)
To: Tristan Gingold; +Cc: Paolo Bonzini, qemu-devel
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH] Use sigwait instead of sigwaitinfo.
2011-02-18 13:17 ` [Qemu-devel] [PATCH] Use sigwait instead of sigwaitinfo Tristan Gingold
2011-02-18 14:09 ` [Qemu-devel] " Paolo Bonzini
2011-02-18 15:34 ` Jan Kiszka
@ 2011-02-18 18:15 ` Blue Swirl
2011-02-25 20:58 ` Blue Swirl
3 siblings, 0 replies; 9+ messages in thread
From: Blue Swirl @ 2011-02-18 18:15 UTC (permalink / raw)
To: Tristan Gingold; +Cc: qemu-devel
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.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH] Use sigwait instead of sigwaitinfo.
2011-02-18 13:17 ` [Qemu-devel] [PATCH] Use sigwait instead of sigwaitinfo Tristan Gingold
` (2 preceding siblings ...)
2011-02-18 18:15 ` [Qemu-devel] " Blue Swirl
@ 2011-02-25 20:58 ` Blue Swirl
3 siblings, 0 replies; 9+ messages in thread
From: Blue Swirl @ 2011-02-25 20:58 UTC (permalink / raw)
To: Tristan Gingold; +Cc: qemu-devel
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
>
>
>
^ permalink raw reply [flat|nested] 9+ messages in thread