Re: [PATCH 1/2] linux-user: add do_setsockopt SOL_CAN_RAW CAN_RAW_FILTER support

[Laurent Vivier]

Le 12/05/2020 à 23:05, Tomas Krcka a écrit :
> Am Di., 12. Mai 2020 um 22:09 Uhr schrieb Laurent Vivier <address@hidden>:
>>
>> Le 06/05/2020 à 15:21, Tomas Krcka a écrit :
>>> Signed-off-by: Tomas Krcka <address@hidden>
>>> ---
>>>  linux-user/syscall.c | 34 ++++++++++++++++++++++++++++++++++
>>>  1 file changed, 34 insertions(+)
>>>
>>> diff --git a/linux-user/syscall.c b/linux-user/syscall.c
>>> index 05f03919ff..88d4c85b70 100644
>>> --- a/linux-user/syscall.c
>>> +++ b/linux-user/syscall.c
>>> @@ -56,6 +56,7 @@
>>>  #include <linux/wireless.h>
>>>  #include <linux/icmp.h>
>>>  #include <linux/icmpv6.h>
>>> +#include <linux/can/raw.h>
>>>  #include <linux/errqueue.h>
>>>  #include <linux/random.h>
>>>  #ifdef CONFIG_TIMERFD
>>> @@ -2111,6 +2112,39 @@ static abi_long do_setsockopt(int sockfd, int level, 
>>> int optname,
>>>              goto unimplemented;
>>>          }
>>>          break;
>>> +    case SOL_CAN_RAW:
>>> +        switch (optname) {
>>> +        case CAN_RAW_FILTER:
>>> +        {
>>> +            if (optlen % sizeof(struct can_filter) != 0) {
>>> +                return -TARGET_EINVAL;
>>> +            }
>>> +
>>> +            struct can_filter  *can_filters = NULL;
>>
>> Move the declaration to the top of the block.
>>
>>> +            if (optlen != 0) {
>>
>> If you check, like in kernel, "optlen > CAN_RAW_FILTER_MAX *
>> sizeof(struct can_filter)", you can exit here (and no need to set
>> can_filters to NULL).
>>
> 
> The optlen can be 0 and then the can_filter shall be NULL, based on
> the socketcan
> documentation.

Yes, you're right I misread the kernel code.

But check optlen is lesser than "CAN_RAW_FILTER_MAX * sizeof(struct
can_filter)" to avoir too big g_new0() allocation.

And in fact "g_new0()" is wrong in your code: optlen is the byte size,
not the number of entries. You should use g_malloc0(optlen).

> And an additional question, shall I check if optlen is 1 and then use
> non-dynamic allocated
> filters, as it's done in kernel?

No, keep the code as simple as possible.

Thanks,
Laurent