Re: [PATCH v1 1/1] linux-user/signal: Decode waitid si_code
On Mon, Jan 18, 2021 at 6:36 AM Laurent Vivier wrote:
>
> Le 19/12/2020 à 19:11, Alistair Francis a écrit :
> > When mapping the host waitid status to the target status we previously
> > just used decoding information in the status value. This doesn't follow
> > what the waitid documentation describes, which instead suggests using
> > the si_code value for the decoding. This results in the incorrect values
> > seen when calling waitid. This is especially apparent on RV32 where all
> > wait calls use waitid (see the bug case).
> >
> > This patch uses the si_code value to map the waitid status.
> >
> > Buglink: https://bugs.launchpad.net/qemu/+bug/1906193
> > Signed-off-by: Alistair Francis
> > ---
> > linux-user/signal.c | 26 --
> > 1 file changed, 24 insertions(+), 2 deletions(-)
> >
> > diff --git a/linux-user/signal.c b/linux-user/signal.c
> > index 73de934c65..b6c9326521 100644
> > --- a/linux-user/signal.c
> > +++ b/linux-user/signal.c
> > @@ -305,6 +305,7 @@ static inline void
> > host_to_target_siginfo_noswap(target_siginfo_t *tinfo,
> > int sig = host_to_target_signal(info->si_signo);
> > int si_code = info->si_code;
> > int si_type;
> > +int status = info->si_status;
> > tinfo->si_signo = sig;
> > tinfo->si_errno = 0;
> > tinfo->si_code = info->si_code;
> > @@ -349,8 +350,29 @@ static inline void
> > host_to_target_siginfo_noswap(target_siginfo_t *tinfo,
> > case TARGET_SIGCHLD:
> > tinfo->_sifields._sigchld._pid = info->si_pid;
> > tinfo->_sifields._sigchld._uid = info->si_uid;
> > -tinfo->_sifields._sigchld._status
> > -= host_to_target_waitstatus(info->si_status);
> > +
> > +/*
> > + * Map host to target signal numbers for the waitid family of
> > + * syscalls. This is similar to the functionality in
> > + * host_to_target_waitstatus() except we use the si_code to
> > + * determine the operation.
> > + */
> > +switch (info->si_code) {
> > +case CLD_KILLED:
> > +case CLD_DUMPED:
> > +tinfo->_sifields._sigchld._status =
> > +host_to_target_signal(WTERMSIG(status)) |
> > + (status & ~0x7f);
> > +break;
> > +case CLD_STOPPED:
> > +tinfo->_sifields._sigchld._status =
> > +(host_to_target_signal(WSTOPSIG(status)) << 8) |
> > +(status & 0xff);
> > +break;
> > +default:
>
> I guess the the operation is not encoded in the status coming from the host
> as we need to use the
> si_code to decode the status, so why do we need to encode it in the status we
> send to the guest?
>
> Can it be only "tinfo->_sifields._sigchld._status = status" for all the cases?
That also works, I'll send a v2.
Alistair
>
> Thanks,
> Laurent
Re: [PATCH v1 1/1] linux-user/signal: Decode waitid si_code
Le 19/12/2020 à 19:11, Alistair Francis a écrit :
> When mapping the host waitid status to the target status we previously
> just used decoding information in the status value. This doesn't follow
> what the waitid documentation describes, which instead suggests using
> the si_code value for the decoding. This results in the incorrect values
> seen when calling waitid. This is especially apparent on RV32 where all
> wait calls use waitid (see the bug case).
>
> This patch uses the si_code value to map the waitid status.
>
> Buglink: https://bugs.launchpad.net/qemu/+bug/1906193
> Signed-off-by: Alistair Francis
> ---
> linux-user/signal.c | 26 --
> 1 file changed, 24 insertions(+), 2 deletions(-)
>
> diff --git a/linux-user/signal.c b/linux-user/signal.c
> index 73de934c65..b6c9326521 100644
> --- a/linux-user/signal.c
> +++ b/linux-user/signal.c
> @@ -305,6 +305,7 @@ static inline void
> host_to_target_siginfo_noswap(target_siginfo_t *tinfo,
> int sig = host_to_target_signal(info->si_signo);
> int si_code = info->si_code;
> int si_type;
> +int status = info->si_status;
> tinfo->si_signo = sig;
> tinfo->si_errno = 0;
> tinfo->si_code = info->si_code;
> @@ -349,8 +350,29 @@ static inline void
> host_to_target_siginfo_noswap(target_siginfo_t *tinfo,
> case TARGET_SIGCHLD:
> tinfo->_sifields._sigchld._pid = info->si_pid;
> tinfo->_sifields._sigchld._uid = info->si_uid;
> -tinfo->_sifields._sigchld._status
> -= host_to_target_waitstatus(info->si_status);
> +
> +/*
> + * Map host to target signal numbers for the waitid family of
> + * syscalls. This is similar to the functionality in
> + * host_to_target_waitstatus() except we use the si_code to
> + * determine the operation.
> + */
> +switch (info->si_code) {
> +case CLD_KILLED:
> +case CLD_DUMPED:
> +tinfo->_sifields._sigchld._status =
> +host_to_target_signal(WTERMSIG(status)) |
> + (status & ~0x7f);
> +break;
> +case CLD_STOPPED:
> +tinfo->_sifields._sigchld._status =
> +(host_to_target_signal(WSTOPSIG(status)) << 8) |
> +(status & 0xff);
> +break;
> +default:
I guess the the operation is not encoded in the status coming from the host as
we need to use the
si_code to decode the status, so why do we need to encode it in the status we
send to the guest?
Can it be only "tinfo->_sifields._sigchld._status = status" for all the cases?
Thanks,
Laurent
Re: [PATCH v1 1/1] linux-user/signal: Decode waitid si_code
Am Samstag, 19. Dezember 2020, 20:11:13 EET schrieb Alistair Francis:
> When mapping the host waitid status to the target status we previously
> just used decoding information in the status value. This doesn't follow
> what the waitid documentation describes, which instead suggests using
> the si_code value for the decoding. This results in the incorrect values
> seen when calling waitid. This is especially apparent on RV32 where all
> wait calls use waitid (see the bug case).
>
> This patch uses the si_code value to map the waitid status.
>
> Buglink: https://bugs.launchpad.net/qemu/+bug/1906193
> Signed-off-by: Alistair Francis
Tested-by: Andreas K. Hüttel
> ---
> linux-user/signal.c | 26 --
> 1 file changed, 24 insertions(+), 2 deletions(-)
>
> diff --git a/linux-user/signal.c b/linux-user/signal.c
> index 73de934c65..b6c9326521 100644
> --- a/linux-user/signal.c
> +++ b/linux-user/signal.c
> @@ -305,6 +305,7 @@ static inline void
> host_to_target_siginfo_noswap(target_siginfo_t *tinfo, int sig =
> host_to_target_signal(info->si_signo);
> int si_code = info->si_code;
> int si_type;
> +int status = info->si_status;
> tinfo->si_signo = sig;
> tinfo->si_errno = 0;
> tinfo->si_code = info->si_code;
> @@ -349,8 +350,29 @@ static inline void
> host_to_target_siginfo_noswap(target_siginfo_t *tinfo, case TARGET_SIGCHLD:
> tinfo->_sifields._sigchld._pid = info->si_pid;
> tinfo->_sifields._sigchld._uid = info->si_uid;
> -tinfo->_sifields._sigchld._status
> -= host_to_target_waitstatus(info->si_status);
> +
> +/*
> + * Map host to target signal numbers for the waitid family of
> + * syscalls. This is similar to the functionality in
> + * host_to_target_waitstatus() except we use the si_code to
> + * determine the operation.
> + */
> +switch (info->si_code) {
> +case CLD_KILLED:
> +case CLD_DUMPED:
> +tinfo->_sifields._sigchld._status =
> +host_to_target_signal(WTERMSIG(status)) |
> + (status & ~0x7f);
> +break;
> +case CLD_STOPPED:
> +tinfo->_sifields._sigchld._status =
> +(host_to_target_signal(WSTOPSIG(status)) << 8) |
> +(status & 0xff);
> +break;
> +default:
> +tinfo->_sifields._sigchld._status = status;
> +}
> +
> tinfo->_sifields._sigchld._utime = info->si_utime;
> tinfo->_sifields._sigchld._stime = info->si_stime;
> si_type = QEMU_SI_CHLD;
--
Andreas K. Hüttel
dilfri...@gentoo.org
Gentoo Linux developer
(council, qa, toolchain, base-system, perl, libreoffice)
signature.asc
Description: This is a digitally signed message part.
Re: [PATCH v1 1/1] linux-user/signal: Decode waitid si_code
On Sat, Dec 19, 2020 at 10:11 AM Alistair Francis
wrote:
>
> When mapping the host waitid status to the target status we previously
> just used decoding information in the status value. This doesn't follow
> what the waitid documentation describes, which instead suggests using
> the si_code value for the decoding. This results in the incorrect values
> seen when calling waitid. This is especially apparent on RV32 where all
> wait calls use waitid (see the bug case).
>
> This patch uses the si_code value to map the waitid status.
>
> Buglink: https://bugs.launchpad.net/qemu/+bug/1906193
> Signed-off-by: Alistair Francis
Ping!
Alistair
> ---
> linux-user/signal.c | 26 --
> 1 file changed, 24 insertions(+), 2 deletions(-)
>
> diff --git a/linux-user/signal.c b/linux-user/signal.c
> index 73de934c65..b6c9326521 100644
> --- a/linux-user/signal.c
> +++ b/linux-user/signal.c
> @@ -305,6 +305,7 @@ static inline void
> host_to_target_siginfo_noswap(target_siginfo_t *tinfo,
> int sig = host_to_target_signal(info->si_signo);
> int si_code = info->si_code;
> int si_type;
> +int status = info->si_status;
> tinfo->si_signo = sig;
> tinfo->si_errno = 0;
> tinfo->si_code = info->si_code;
> @@ -349,8 +350,29 @@ static inline void
> host_to_target_siginfo_noswap(target_siginfo_t *tinfo,
> case TARGET_SIGCHLD:
> tinfo->_sifields._sigchld._pid = info->si_pid;
> tinfo->_sifields._sigchld._uid = info->si_uid;
> -tinfo->_sifields._sigchld._status
> -= host_to_target_waitstatus(info->si_status);
> +
> +/*
> + * Map host to target signal numbers for the waitid family of
> + * syscalls. This is similar to the functionality in
> + * host_to_target_waitstatus() except we use the si_code to
> + * determine the operation.
> + */
> +switch (info->si_code) {
> +case CLD_KILLED:
> +case CLD_DUMPED:
> +tinfo->_sifields._sigchld._status =
> +host_to_target_signal(WTERMSIG(status)) |
> + (status & ~0x7f);
> +break;
> +case CLD_STOPPED:
> +tinfo->_sifields._sigchld._status =
> +(host_to_target_signal(WSTOPSIG(status)) << 8) |
> +(status & 0xff);
> +break;
> +default:
> +tinfo->_sifields._sigchld._status = status;
> +}
> +
> tinfo->_sifields._sigchld._utime = info->si_utime;
> tinfo->_sifields._sigchld._stime = info->si_stime;
> si_type = QEMU_SI_CHLD;
> --
> 2.29.2
>
