Merge request https://gitlab.rtems.org/rtems/rtos/rtems/-/merge_requests/118 
was reviewed by Joel Sherrill

--
  
Joel Sherrill started a new discussion on 
cpukit/include/rtems/posix/aio_misc.h: 
https://gitlab.rtems.org/rtems/rtos/rtems/-/merge_requests/118#note_109373

 > +/**
 > + * @brief Generates a notification based on the sigenvent sructure passed 
 > as parameter.
 > + * 

Shorten the brief. Think section heading in Table of Contents.

Turn the details into a first paragraph.

Add @param for the argument.

--
  
Joel Sherrill started a new discussion on cpukit/posix/src/aio_misc.c: 
https://gitlab.rtems.org/rtems/rtos/rtems/-/merge_requests/118#note_109374

 >   */
 >  
 > +

Remove extra blank line.

--
  
Joel Sherrill started a new discussion on cpukit/posix/src/aio_misc.c: 
https://gitlab.rtems.org/rtems/rtos/rtems/-/merge_requests/118#note_109375

 > +
 >  #include <pthread.h>
 > +#include <stdio.h>

Is this for your debugging or needed permanently?

--
  
Joel Sherrill started a new discussion on cpukit/posix/src/aio_misc.c: 
https://gitlab.rtems.org/rtems/rtos/rtems/-/merge_requests/118#note_109376

 > +        sig->sigev_value
 > +      );
 > +      if ( result != 0 ){

Space needed. "){" should be ") {".

I see this multiple places. Search and destroy.

--
  
Joel Sherrill started a new discussion on cpukit/posix/src/aio_misc.c: 
https://gitlab.rtems.org/rtems/rtos/rtems/-/merge_requests/118#note_109377

 > +
 > +      if ( sig->sigev_notify_function == NULL ){
 > +        //handle error

There isn't really anyway to handle an error here. Should this be caught at the 
API level so an error can be returned to the caller?

If so, then the checks which were in here can become debug asserts.

--
  
Joel Sherrill started a new discussion on cpukit/posix/src/aio_misc.c: 
https://gitlab.rtems.org/rtems/rtos/rtems/-/merge_requests/118#note_109378

 > +        &thread,
 > +        attr,
 > +        (void * (*)(void *)) sig->sigev_notify_function,

You need to wrap the sigev_notification function in a wrapper thread body which 
has the proper signature. Pass in the notify function as the argument, cast it, 
call it, and return NULL. It may also make sense to explicitly call 
pthread_exit() at the bottom of the wrapper function.

--
  
Joel Sherrill started a new discussion on cpukit/posix/src/aio_misc.c: 
https://gitlab.rtems.org/rtems/rtos/rtems/-/merge_requests/118#note_109379

 >    }
 > +  rtems_aio_notify(
 > +    &(req->aiocbp->aio_sigevent)

Parentheses are not needed.

Can fit on one line.

--
  
Joel Sherrill started a new discussion on cpukit/posix/src/aio_misc.c: 
https://gitlab.rtems.org/rtems/rtos/rtems/-/merge_requests/118#note_109380

 >        result = -1;
 >    }
 > +  rtems_aio_notify(

Looks like there needs to be a blank line above this.

--
  
Joel Sherrill started a new discussion on 
spec/build/testsuites/psxtests/psxaio04.yml: 
https://gitlab.rtems.org/rtems/rtos/rtems/-/merge_requests/118#note_109381

 > +cflags: []
 > +copyrights:
 > +- Copyright (C) 2020 embedded brains GmbH & Co. KG

You can put your name and copyright on any file of your own creation or that 
you have modified more than a few lines.

--
  
Joel Sherrill started a new discussion on testsuites/psxtests/psxaio04/init.c: 
https://gitlab.rtems.org/rtems/rtos/rtems/-/merge_requests/118#note_109382

 > +}
 > +
 > +

Remove a blank line here.

--
  
Joel Sherrill started a new discussion on testsuites/psxtests/psxaio04/init.c: 
https://gitlab.rtems.org/rtems/rtos/rtems/-/merge_requests/118#note_109383

 > +
 > +/*
 > + * Copyright 2010, Alin Rus <[email protected]>

Is this completely new? Modified? Does your copyright and 2024 need to be added?

--
  
Joel Sherrill started a new discussion on testsuites/psxtests/psxaio04/init.c: 
https://gitlab.rtems.org/rtems/rtos/rtems/-/merge_requests/118#note_109384

 > +  rtems_test_assert( !status );
 > +  
 > +  fd = open( "/tmp/aio_fildes", O_RDWR|O_CREAT, S_IRWXU|S_IRWXG|S_IRWXO );

Is this line too long?

--
  
Joel Sherrill started a new discussion on 
testsuites/psxtests/psxaio04/system.h: 
https://gitlab.rtems.org/rtems/rtos/rtems/-/merge_requests/118#note_109385

 > +
 > +/*
 > + * Copyright 2010, Alin Rus <[email protected]>

Again check need for your copyright




-- 
View it on GitLab: 
https://gitlab.rtems.org/rtems/rtos/rtems/-/merge_requests/118
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

Reply via email to