[PATCH rtems-libbsd] ipsec-tools: Fix copying fd_set prior to select
Chris Johns
chrisj at rtems.org
Wed May 24 22:58:29 UTC 2023
On 24/5/2023 11:42 pm, Christian MAUDERER wrote:
> On 2023-05-24 02:33, Chris Johns wrote:
>> On 24/5/2023 9:13 am, Chris Johns wrote:
>>> On 23/5/2023 5:30 pm, Christian MAUDERER wrote:
>>>> Hello Chris,
>>>>
>>>> On 2023-05-23 08:53, Chris Johns wrote:
>>>>> On 23/5/2023 4:25 pm, Christian MAUDERER wrote:
>>>>>> Hello Chris,
>>>>>>
>>>>>> On 2023-05-23 03:36, Chris Johns wrote:
>>>>>>> Hi,
>>>>>>>
>>>>>>> I have been resolving this by adding:
>>>>>>>
>>>>>>> #define preset_mask *preset_mask_prealloc
>>>>>>> #define active_mask *active_mask_prealloc
>>>>>>>
>>>>>>> as the assignment logic works as is. It does not catch FD_ZERO which is a
>>>>>>> shame
>>>>>>> but is helps avoid us adding a special case.
>>>>>>
>>>>>> Thanks for the suggestion. I used that solution already in the file during
>>>>>> the
>>>>>> initial port. But it doesn't work in this case (and maybe we should check
>>>>>> other
>>>>>> cases in libbsd too). It only copies the first element of the array of
>>>>>> fd_sets.
>>>>>>
>>>>>> The variables / defines are (in the patched version - see [1]):
>>>>>>
>>>>>> static fd_set *allocated_preset_mask, *allocated_active_mask;
>>>>>> static struct fd_monitor *allocated_fd_monitors;
>>>>>> #define preset_mask (*allocated_preset_mask)
>>>>>> #define active_mask (*allocated_active_mask)
>>>>>> #define fd_monitors (allocated_fd_monitors)
>>>>>>
>>>>>> They are allocated as follows (see [2]):
>>>>>>
>>>>>> allocated_preset_mask = calloc(sizeof(fd_set),
>>>>>> howmany(rtems_libio_number_iops, sizeof(fd_set) * 8));
>>>>>> allocated_active_mask = calloc(sizeof(fd_set),
>>>>>> howmany(rtems_libio_number_iops, sizeof(fd_set) * 8));
>>>>>> allocated_fd_monitors = calloc(
>>>>>> rtems_libio_number_iops, sizeof(struct fd_monitor));
>>>>>>
>>>>>> So basically it's a bunch of arrays of fd_sets. If I now use the following
>>>>>> (unpatched) assignment:
>>>>>>
>>>>>> active_mask = preset_mask;
>>>>>>
>>>>>> After the preprocessor the line will be
>>>>>>
>>>>>> (*allocated_active_mask) = (*allocated_preset_mask);
>>>>>>
>>>>>> Which copies only the first element. That's why I had to add the memcpy.
>>>>>> Did I
>>>>>> miss some better solution?
>>>>>
>>>>> I am using this in ntpd which I checked yesterday:
>>>>>
>>>>> #ifdef __rtems__
>>>>> #include <rtems/libio_.h>
>>>>> static size_t rtems_ntpd_fds_size;
>>>>> static int rtems_fd_set_alloc(fd_set **setp) {
>>>>> if (*setp == NULL) {
>>>>> rtems_ntpd_fds_size = sizeof(fd_set) *
>>>>> (howmany(rtems_libio_number_iops, sizeof(fd_set) * 8));
>>>>> *setp = malloc(rtems_ntpd_fds_size);
>>>>> if (*setp == NULL) {
>>>>> errno = ENOMEM;
>>>>> return -1;
>>>>> }
>>>>> memset(*setp, 0, rtems_ntpd_fds_size);
>>>>> }
>>>>> return 0;
>>>>> }
>>>>> #define activefds (*activefds_prealloc)
>>>>> static fd_set *activefds_prealloc;
>>>>> #define rtems_activefds_alloc() rtems_fd_set_alloc(&activefds_prealloc)
>>>>> #else /* __rtems__ */
>>>>> static fd_set activefds;
>>>>> #endif /* __rtems__ */
>>>>>
>>>>> and:
>>>>>
>>>>> #if __rtems__
>>>>> #define rdfdes (*rdfdes_prealloc)
>>>>> static fd_set *rdfdes_prealloc;
>>>>> #else /* __rtems__ */
>>>>> fd_set rdfdes;
>>>>> #endif /* __rtems__ */
>>>>> int nfound;
>>>>>
>>>>> #if __rtems__
>>>>> if (rtems_fd_set_alloc(&rdfdes_prealloc) < 0) {
>>>>> return;
>>>>> }
>>>>> memset(rdfdes_prealloc, 0, rtems_ntpd_fds_size);
>>>>> #else /* __rtems__ */
>>>>> FD_ZERO(&set);
>>>>> #endif /* __rtems__ */
>>>>>
>>>>> with:
>>>>>
>>>>> ++handler_calls;
>>>>> rdfdes = activefds;
>>>>
>>>> But isn't that line resolving to
>>>>
>>>> (*rdfdes_prealloc) = (*activefds_prealloc);
>>>>
>>>> That would also copy only the first element.
>>>>
>>>>>
>>>>> Stepping:
>>>>>
>>>>> Thread 9 hit Breakpoint 1, _ntp_io_handler () at
>>>>> ../../bsd/freebsd/contrib/ntp/ntpd/ntp_io.c:3727
>>>>> 3727 rdfdes = activefds;
>>>>> (gdb) x /8bx activefds_prealloc
>>>>> 0x1083e650: 0x20 0x44 0x05 0x20 0x00 0x40 0x00 0x00
>>>>> (gdb) x /8bx rdfdes_prealloc
>>>>> 0x108353a0: 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00
>>>>> (gdb) n
>>>>> 3736 t1.tv_usec = 0;
>>>>> (gdb) x /8bx rdfdes_prealloc
>>>>> 0x108353a0: 0x20 0x44 0x05 0x20 0x00 0x40 0x00 0x00
>>>>>
>>>>> Looks ok to me.
>>>>
>>>> fd_set is 64 bit wide. So a copy of the first 64 bit works. Does it still work
>>>> if you have 256 files in the system?
>>>
>>> Good question. I have not checked but I will.
>>>
>>> FYI EPICS has set its limit to 64 because of this issue with select. I suspect
>>> they do not want to hack their select calls like this and I understand them
>>> taking that position.
>>
>> Yes, I see the issue. The equate only works if the file ops count is the same as
>> the newlib defined fd_set.
>>
>> Chris
>
> Thanks for the confirmation. Do you see a better solution than the memcpy in my
> patch?
Thank you for highlighting the limitation I had.
No better solutions come to mind. I think we are limited by the design of
select. I see two possible paths:
1. Use a local FD_SETSIZE compiler option define for a package to increase the
default size to a larger value. There is a hard limit of 2048 in libbsd which
means a stack overhead of 256 bytes. This is suitable where you can assume there
is memory to use but it is not for the newlib default.
2. The custom per select call site implementation you and I are using.
There is a discussion in EPICS [1] about using FD_SETSIZE because EPCIS also
sets the number of available fds. This is a good solution for them because they
do not need custom code like we do in 3rd party libraries.
Oh and I have no issue with the change you posted. :)
Chris
[1] https://github.com/epics-base/epics-base/pull/382
More information about the devel
mailing list