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.
_______________________________________________ bugs mailing list [email protected] http://lists.rtems.org/mailman/listinfo/bugs
