RTEMS | Draft: posix/aio: Implemented lio_listio() (!275)
Gedare Bloom (@gedare)
gitlab at rtems.org
Wed Oct 30 15:17:52 UTC 2024
Merge request https://gitlab.rtems.org/rtems/rtos/rtems/-/merge_requests/275 was reviewed by Gedare Bloom
--
Gedare Bloom started a new discussion on cpukit/include/rtems/posix/aio_misc.h: https://gitlab.rtems.org/rtems/rtos/rtems/-/merge_requests/275#note_114016
> +#define AIO_SIGNALED 0
> +
> +/** signal not generated for suspend call */
Capital `Signal` for consistency with nearby comments. Do you need both, or can you use `!(AIO_SIGNALED)`?
It is more common for something that sets a signal to be 1, and to clear the signal with 0.
--
Gedare Bloom started a new discussion on cpukit/include/rtems/posix/aio_misc.h: https://gitlab.rtems.org/rtems/rtos/rtems/-/merge_requests/275#note_114017
> + int notified;
> +
> +} suspendcb;
This name and also `listcb` are not appropriate for use here. Only names that are defined within POSIX or are in the `rtems_` namespace should be exported by header files. I would suggest using `rtems_aio_suspendcb` at a minimum.
--
Gedare Bloom started a new discussion on cpukit/include/rtems/posix/aio_misc.h: https://gitlab.rtems.org/rtems/rtos/rtems/-/merge_requests/275#note_114018
> + * @param listcbp a pointer to the list control block.
> */
> void rtems_aio_completed_list_op( listcb *listcbp );
While you're at it, you could change `listcb` to be `rtems_aio_listcb`. This would constitute an API change and requires further documentation.
--
Gedare Bloom started a new discussion on cpukit/include/rtems/rtems/event.h: https://gitlab.rtems.org/rtems/rtos/rtems/-/merge_requests/275#note_114019
> + * aio list completion, used when lio_listio is called using LIO_WAIT.
> + */
> +#define RTEMS_EVENT_SYSTEM_AIO_SUSPEND RTEMS_EVENT_27
The content in this file is automatically generated. We'll need to get some help how to update this correctly following the guidance from https://docs.rtems.org/branches/master/eng/req/index.html
--
Gedare Bloom started a new discussion on cpukit/include/aio.h: https://gitlab.rtems.org/rtems/rtos/rtems/-/merge_requests/275#note_114020
> +
> +
> +
too many blank lines
--
Gedare Bloom started a new discussion on cpukit/posix/src/aio_misc.c: https://gitlab.rtems.org/rtems/rtos/rtems/-/merge_requests/275#note_114021
> #include <errno.h>
> #include <limits.h>
> +#include <stdio.h>
why do you need this header?
--
Gedare Bloom started a new discussion on cpukit/posix/src/aio_misc.c: https://gitlab.rtems.org/rtems/rtos/rtems/-/merge_requests/275#note_114022
> }
>
> +void rtems_aio_completed_suspend_op( suspendcb *suspendcbp )
should this be returning a status or error codes?
--
Gedare Bloom started a new discussion on cpukit/posix/src/aio_misc.c: https://gitlab.rtems.org/rtems/rtos/rtems/-/merge_requests/275#note_114023
> +
> + if ( suspendcbp->notified == AIO_NONSIGNALED ) {
> + rtems_event_system_send(
I would feel better if the event send can happen outside of the locked critical section. I'd have to check the documentation more closely to see if this is a real problem, but if the code can be refactored to read/update the critical node data first, and then send the event outside the critical section, this may be better.
--
Gedare Bloom started a new discussion on cpukit/posix/src/aio_misc.c: https://gitlab.rtems.org/rtems/rtos/rtems/-/merge_requests/275#note_114024
> +
> + if( suspendcbp->requests_left == 0 ) {
> + free( suspendcbp );
Another thread could be holding the `suspendcbp->mutex`? This is a race condition bug waiting to happen.
--
Gedare Bloom started a new discussion on cpukit/posix/src/aio_misc.c: https://gitlab.rtems.org/rtems/rtos/rtems/-/merge_requests/275#note_114025
> }
>
> +void rtems_aio_completed_suspend_op( suspendcb *suspendcbp )
usually we would like the function to contain a verb. Is this function "completing" or "suspending"? I think the name could use some more discussion.
--
Gedare Bloom started a new discussion on cpukit/posix/src/aio_suspend.c: https://gitlab.rtems.org/rtems/rtos/rtems/-/merge_requests/275#note_114026
> + suspendcbp = malloc( sizeof( suspendcb ) );
> + if ( suspendcbp == NULL ) {
> + rtems_set_errno_and_return_minus_one( EAGAIN );
`ENOMEM`?
--
Gedare Bloom started a new discussion on cpukit/posix/src/aio_suspend.c: https://gitlab.rtems.org/rtems/rtos/rtems/-/merge_requests/275#note_114027
> +
> + /* LOCK REQUEST QUEUE */
> + pthread_mutex_lock( &aio_request_queue.mutex );
Make sure the ordering of these locks is always consistent to avoid deadlock.
--
Gedare Bloom started a new discussion on cpukit/posix/src/aio_suspend.c: https://gitlab.rtems.org/rtems/rtos/rtems/-/merge_requests/275#note_114028
> +
> + /* ITERATE OVER EVERY REQUEST */
> + for ( int i = 0; i < nent; i++ ) {
declare `int i` at the start of the function block, not within the `for()`.
--
Gedare Bloom started a new discussion on cpukit/posix/src/aio_suspend.c: https://gitlab.rtems.org/rtems/rtos/rtems/-/merge_requests/275#note_114029
> + r_chain = rtems_aio_search_fd( work_req_chain, list[i]->aio_fildes, 0 );
> + if ( r_chain != NULL ) {
> + //SEARCH IN CHAIN
use `/* */` for consistency. Also would prefer regular/mixed case, not ALL CAPS
--
Gedare Bloom started a new discussion on cpukit/posix/src/aio_suspend.c: https://gitlab.rtems.org/rtems/rtos/rtems/-/merge_requests/275#note_114030
> + if ( current->suspendcbp == NULL ) {
> + current->suspendcbp = suspendcbp;
> + }
suggest refactoring these lines to a helper function. Especially since you use basically the same logic again near line 129.
--
Gedare Bloom started a new discussion on cpukit/posix/src/aio_suspend.c: https://gitlab.rtems.org/rtems/rtos/rtems/-/merge_requests/275#note_114031
> +
> + //REQUEST NOT PRESENT
> + continue;
This is superfluous at the bottom of a loop.
--
Gedare Bloom started a new discussion on cpukit/posix/src/aio_suspend.c: https://gitlab.rtems.org/rtems/rtos/rtems/-/merge_requests/275#note_114032
> + if ( suspendcbp->requests_left <= 0 ) {
> + pthread_mutex_unlock( &aio_request_queue.mutex );
> + free( suspendcbp );
Here you free `suspendcbp` with the `suspendcbp->mutex` held, and then return. This is a bug.
--
Gedare Bloom started a new discussion on cpukit/posix/src/aio_suspend.c: https://gitlab.rtems.org/rtems/rtos/rtems/-/merge_requests/275#note_114033
> + );
> + }else {
> + long ticks = rtems_timespec_to_ticks( timeout );
This API is being changed to return a `time_t` (i hope). I suggest you cast it here to `time_t` instead of `long`.
--
Gedare Bloom started a new discussion on cpukit/posix/src/aio_suspend.c: https://gitlab.rtems.org/rtems/rtos/rtems/-/merge_requests/275#note_114034
> + }
> + }
> + if ( op_num == 0 ) {
Can it be `< 0`?
--
Gedare Bloom started a new discussion on cpukit/posix/src/aio_suspend.c: https://gitlab.rtems.org/rtems/rtos/rtems/-/merge_requests/275#note_114035
> + &event_out
> + );
> + }
I suggest refactoring this timeout checking/event part to a helper function.
--
Gedare Bloom started a new discussion on testsuites/psxtests/psxaio06/init.c: https://gitlab.rtems.org/rtems/rtos/rtems/-/merge_requests/275#note_114036
> + */
> +
> +
avoid multiple blank lines
--
View it on GitLab: https://gitlab.rtems.org/rtems/rtos/rtems/-/merge_requests/275
You're receiving this email because of your account on gitlab.rtems.org.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.rtems.org/pipermail/bugs/attachments/20241030/3fefa50b/attachment-0001.htm>
More information about the bugs
mailing list