[Qemu-devel] [PATCH v2 3/3] linux-user: exclude SO_TIMESTAMP cmsg_type from unsuppoted ancillary data

[Qemu-devel] [PATCH v2 3/3] linux-user: exclude SO_TIMESTAMP cmsg_type from unsuppoted ancillary data

[Jing Huang]

This patch excludes SO_TIMESTAMP cmsg_type from unsuppoted ancillary data.

Signed-off-by: Jing Huang <jing.huang.pku@gmail.com>
---
 linux-user/syscall.c |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index fc8690d..9fce57c 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -1349,7 +1349,9 @@ static inline abi_long host_to_target_cmsg(struct target_msghdr *target_msgh,
         target_cmsg->cmsg_type = tswap32(cmsg->cmsg_type);
         target_cmsg->cmsg_len = tswapal(TARGET_CMSG_LEN(len));
 
-        if (cmsg->cmsg_level != TARGET_SOL_SOCKET || cmsg->cmsg_type != SCM_RIGHTS) {
+        if (cmsg->cmsg_level != TARGET_SOL_SOCKET ||
+                    (cmsg->cmsg_type != SCM_RIGHTS &&
+                                    cmsg->cmsg_type != SO_TIMESTAMP)) {
             gemu_log("Unsupported ancillary data: %d/%d\n", cmsg->cmsg_level, cmsg->cmsg_type);
             memcpy(target_data, data, len);
         } else {
-- 
1.7.8.6

Re: [Qemu-devel] [PATCH v2 3/3] linux-user: exclude SO_TIMESTAMP cmsg_type from unsuppoted ancillary data

[Peter Maydell]

On 16 July 2012 11:15, Jing Huang <jing.huang.pku@gmail.com> wrote:
> This patch excludes SO_TIMESTAMP cmsg_type from unsuppoted ancillary data.

"unsupported".

>
> Signed-off-by: Jing Huang <jing.huang.pku@gmail.com>
> ---
>  linux-user/syscall.c |    4 +++-
>  1 files changed, 3 insertions(+), 1 deletions(-)
>
> diff --git a/linux-user/syscall.c b/linux-user/syscall.c
> index fc8690d..9fce57c 100644
> --- a/linux-user/syscall.c
> +++ b/linux-user/syscall.c
> @@ -1349,7 +1349,9 @@ static inline abi_long host_to_target_cmsg(struct target_msghdr *target_msgh,
>          target_cmsg->cmsg_type = tswap32(cmsg->cmsg_type);
>          target_cmsg->cmsg_len = tswapal(TARGET_CMSG_LEN(len));
>
> -        if (cmsg->cmsg_level != TARGET_SOL_SOCKET || cmsg->cmsg_type != SCM_RIGHTS) {
> +        if (cmsg->cmsg_level != TARGET_SOL_SOCKET ||
> +                    (cmsg->cmsg_type != SCM_RIGHTS &&
> +                                    cmsg->cmsg_type != SO_TIMESTAMP)) {
>              gemu_log("Unsupported ancillary data: %d/%d\n", cmsg->cmsg_level, cmsg->cmsg_type);
>              memcpy(target_data, data, len);
>          } else {

This is kind of ugly -- it causes us to process a SO_TIMESTAMP
payload (which is a 'struct timeval') in the same way as an
SCM_RIGHTS payload (which is an array of file descriptors).
This happens to work because a struct timeval is a pair of
32 bit integers, and so "tswap32() all the words in the data"
works, but it looks pretty weird. If you want to do it this
way you need at least an explanatory comment and really the
variables 'fd', 'target_fd', 'numfds' would need to change
to something more generic and less fd-specific.

-- PMM

Re: [Qemu-devel] [PATCH v2 3/3] linux-user: exclude SO_TIMESTAMP cmsg_type from unsuppoted ancillary data

[Jing Huang]

On Mon, Jul 16, 2012 at 2:45 PM, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 16 July 2012 11:15, Jing Huang <jing.huang.pku@gmail.com> wrote:
>> This patch excludes SO_TIMESTAMP cmsg_type from unsuppoted ancillary data.
>
> "unsupported".
>
>>
>> Signed-off-by: Jing Huang <jing.huang.pku@gmail.com>
>> ---
>>  linux-user/syscall.c |    4 +++-
>>  1 files changed, 3 insertions(+), 1 deletions(-)
>>
>> diff --git a/linux-user/syscall.c b/linux-user/syscall.c
>> index fc8690d..9fce57c 100644
>> --- a/linux-user/syscall.c
>> +++ b/linux-user/syscall.c
>> @@ -1349,7 +1349,9 @@ static inline abi_long host_to_target_cmsg(struct target_msghdr *target_msgh,
>>          target_cmsg->cmsg_type = tswap32(cmsg->cmsg_type);
>>          target_cmsg->cmsg_len = tswapal(TARGET_CMSG_LEN(len));
>>
>> -        if (cmsg->cmsg_level != TARGET_SOL_SOCKET || cmsg->cmsg_type != SCM_RIGHTS) {
>> +        if (cmsg->cmsg_level != TARGET_SOL_SOCKET ||
>> +                    (cmsg->cmsg_type != SCM_RIGHTS &&
>> +                                    cmsg->cmsg_type != SO_TIMESTAMP)) {
>>              gemu_log("Unsupported ancillary data: %d/%d\n", cmsg->cmsg_level, cmsg->cmsg_type);
>>              memcpy(target_data, data, len);
>>          } else {
>
> This is kind of ugly -- it causes us to process a SO_TIMESTAMP
> payload (which is a 'struct timeval') in the same way as an
> SCM_RIGHTS payload (which is an array of file descriptors).
> This happens to work because a struct timeval is a pair of
> 32 bit integers, and so "tswap32() all the words in the data"
> works, but it looks pretty weird. If you want to do it this
> way you need at least an explanatory comment and really the
> variables 'fd', 'target_fd', 'numfds' would need to change
> to something more generic and less fd-specific.
>
> -- PMM


Hmm.. I give up this patch. It is ugly indeed. Additionally, qemu_log
doesn't disturb the normal execution of linux-user and show error
message either.

Re: [Qemu-devel] [PATCH v2 3/3] linux-user: exclude SO_TIMESTAMP cmsg_type from unsuppoted ancillary data

[Peter Maydell]

On 19 July 2012 15:33, Jing Huang <jing.huang.pku@gmail.com> wrote:
> On Mon, Jul 16, 2012 at 2:45 PM, Peter Maydell <peter.maydell@linaro.org> wrote:
>> On 16 July 2012 11:15, Jing Huang <jing.huang.pku@gmail.com> wrote:
>>> This patch excludes SO_TIMESTAMP cmsg_type from unsuppoted ancillary data.
>>
>> "unsupported".
>>
>>>
>>> Signed-off-by: Jing Huang <jing.huang.pku@gmail.com>
>>> ---
>>>  linux-user/syscall.c |    4 +++-
>>>  1 files changed, 3 insertions(+), 1 deletions(-)
>>>
>>> diff --git a/linux-user/syscall.c b/linux-user/syscall.c
>>> index fc8690d..9fce57c 100644
>>> --- a/linux-user/syscall.c
>>> +++ b/linux-user/syscall.c
>>> @@ -1349,7 +1349,9 @@ static inline abi_long host_to_target_cmsg(struct target_msghdr *target_msgh,
>>>          target_cmsg->cmsg_type = tswap32(cmsg->cmsg_type);
>>>          target_cmsg->cmsg_len = tswapal(TARGET_CMSG_LEN(len));
>>>
>>> -        if (cmsg->cmsg_level != TARGET_SOL_SOCKET || cmsg->cmsg_type != SCM_RIGHTS) {
>>> +        if (cmsg->cmsg_level != TARGET_SOL_SOCKET ||
>>> +                    (cmsg->cmsg_type != SCM_RIGHTS &&
>>> +                                    cmsg->cmsg_type != SO_TIMESTAMP)) {
>>>              gemu_log("Unsupported ancillary data: %d/%d\n", cmsg->cmsg_level, cmsg->cmsg_type);
>>>              memcpy(target_data, data, len);
>>>          } else {
>>
>> This is kind of ugly -- it causes us to process a SO_TIMESTAMP
>> payload (which is a 'struct timeval') in the same way as an
>> SCM_RIGHTS payload (which is an array of file descriptors).
>> This happens to work because a struct timeval is a pair of
>> 32 bit integers, and so "tswap32() all the words in the data"
>> works, but it looks pretty weird. If you want to do it this
>> way you need at least an explanatory comment and really the
>> variables 'fd', 'target_fd', 'numfds' would need to change
>> to something more generic and less fd-specific.
>>
>> -- PMM
>
>
> Hmm.. I give up this patch. It is ugly indeed. Additionally, qemu_log
> doesn't disturb the normal execution of linux-user and show error
> message either.

I think it's salvageable. As I say, you can either genericise
the "copy a pile of 32 bit values" code, or just have another
clause to the if() specifically for handling struct timeval
payload data.

-- PMM