Patchwork [v2,3/3] linux-user: exclude SO_TIMESTAMP cmsg_type from unsuppoted ancillary data

login
register
mail settings
Submitter Jing Huang
Date July 16, 2012, 10:15 a.m.
Message ID <1342433706-3376-1-git-send-email-jing.huang.pku@gmail.com>
Download mbox | patch
Permalink /patch/171116/
State New
Headers show

Comments

Jing Huang - July 16, 2012, 10:15 a.m.
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(-)
Peter Maydell - July 16, 2012, 2:45 p.m.
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
Jing Huang - July 19, 2012, 2:33 p.m.
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.
Peter Maydell - July 19, 2012, 2:34 p.m.
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

Patch

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 {