This is the mail archive of the libc-alpha@sources.redhat.com mailing list for the glibc project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [patch] aio_*() posix compliance


Amos Waterland <apw@us.ibm.com> writes:

> I have converted the code to the GNU coding standards, removed checks
> for null pointers, and removed checks for problems that the standard
> allows to be detected asynchronously.  I grouped the remaining changes
> into a single patch, which does the following.

Thanks for the cleanup.  For a perfect patch a ChangeLog entry is
missing that explains your changes.  Could you come up with one,
please?

>
> aio_cancel()
>     - detect bad file descriptor
> aio_error()
>     - no changes
> aio_fsync()
>     - detect bad file descriptor
>     - detect fd not open for writing
> aio_read()
>     - no changes
> aio_return()
>     - no changes
> aio_suspend()
>     - detect completed element(s) and do not suspend thread
> aio_write()
>     - no changes
> lio_listio()
>     - no changes (see comment below)
>
> I believe that glibc with this patch is fully compliant, with the
> possible exception of this issue: the standard says for lio_listio()
> (http://www.opengroup.org/onlinepubs/007904975/functions/lio_listio.html):
>
>     The lio_listio() function shall fail if:
>
>     [EAGAIN] The number of entries indicated by nent would cause the
>     system-wide limit {AIO_MAX} to be exceeded.
>
>     [EINVAL] The mode argument is not a proper value, or the value of
>     nent was greater than {AIO_LISTIO_MAX}.
>
> Glibc does not seem to define AIO_MAX and AIO_LISTIO_MAX, and because of
> the way sysconf() conditionally compiles, it returns -1 for both (see
> below for test0024.c):
>
>     % ./test0024
>     + _POSIX_ASYNCHRONOUS_IO
>     + _POSIX_ASYNC_IO
>     - AIO_LISTIO_MAX
>     - AIO_MAX
>     (-1) _SC_AIO_LISTIO_MAX
>     (-1) _SC_AIO_MAX
>
> The lio_listio() function does not check for nent exceeding the
> constants of concern.  I know that there might be a simple answer to
> this, so I wanted to ask you all about it before I added definitions to
> unistd.h and patched lio_listio.c.
>
> Here is the patch.
>
> ---- Begin patch ----
>
> Index: sysdeps/pthread/aio_cancel.c
> ===================================================================
> RCS file: /cvs/glibc/libc/sysdeps/pthread/aio_cancel.c,v
> retrieving revision 1.2
> diff -u -r1.2 aio_cancel.c
> --- sysdeps/pthread/aio_cancel.c	6 Jul 2001 04:56:02 -0000	1.2
> +++ sysdeps/pthread/aio_cancel.c	16 Jun 2002 01:16:34 -0000
> @@ -43,6 +43,13 @@
>    struct requestlist *req = NULL;
>    int result = AIO_ALLDONE;
>  
> +  /* If fildes is invalid, error. */
> +  if (fcntl (fildes, F_GETFL) < 0)
> +    {
> +      __set_errno (EBADF);
> +      return -1;
> +    }
> +
>    /* Request the mutex.  */
>    pthread_mutex_lock (&__aio_requests_mutex);
>  
> Index: sysdeps/pthread/aio_fsync.c
> ===================================================================
> RCS file: /cvs/glibc/libc/sysdeps/pthread/aio_fsync.c,v
> retrieving revision 1.2
> diff -u -r1.2 aio_fsync.c
> --- sysdeps/pthread/aio_fsync.c	6 Jul 2001 04:56:02 -0000	1.2
> +++ sysdeps/pthread/aio_fsync.c	16 Jun 2002 01:16:34 -0000
> @@ -36,15 +36,23 @@
>  int
>  aio_fsync (int op, struct aiocb *aiocbp)
>  {
> -  if (op != O_DSYNC && op != O_SYNC)
> +  int flags;
> +
> +  if ((flags = fcntl (aiocbp->aio_fildes, F_GETFL)) >= 0
> +      && (flags & (O_RDWR | O_WRONLY)))
>      {
> -      __set_errno (EINVAL);
> -      return -1;
> +      if (op != O_DSYNC && op != O_SYNC)
> +	{
> +	  __set_errno (EINVAL);
> +	  return -1;
> +	}
> +
> +      return (__aio_enqueue_request ((aiocb_union *) aiocbp,
> +                       op == O_SYNC ? LIO_SYNC : LIO_DSYNC) == NULL ? -1 : 0);
>      }
>  
> -  return (__aio_enqueue_request ((aiocb_union *) aiocbp,
> -				 op == O_SYNC ? LIO_SYNC : LIO_DSYNC) == NULL
> -	  ? -1 : 0);
> +  __set_errno (EBADF);
> +  return -1;
>  }
>  
>  weak_alias (aio_fsync, aio_fsync64)
> Index: sysdeps/pthread/aio_suspend.c
> ===================================================================
> RCS file: /cvs/glibc/libc/sysdeps/pthread/aio_suspend.c,v
> retrieving revision 1.2
> diff -u -r1.2 aio_suspend.c
> --- sysdeps/pthread/aio_suspend.c	6 Jul 2001 04:56:02 -0000	1.2
> +++ sysdeps/pthread/aio_suspend.c	16 Jun 2002 01:16:34 -0000
> @@ -49,6 +49,7 @@
>    int result = 0;
>    int dummy;
>    int none = 1;
> +  int do_not_suspend = 0;
>  
>    /* Request the mutex.  */
>    pthread_mutex_lock (&__aio_requests_mutex);
> @@ -56,8 +57,15 @@
>    /* There is not yet a finished request.  Signal the request that
>       we are working for it.  */
>    for (cnt = 0; cnt < nent; ++cnt)
> -    if (list[cnt] != NULL && list[cnt]->__error_code == EINPROGRESS)
> +    if (list[cnt] != NULL)
>        {
> +	/* This element has already completed, so we must not suspend thread. */
> +	if (list[cnt]->__error_code != EINPROGRESS)
> +          {
> +            do_not_suspend = 1;
> +            continue;
> +          }
> +
>  	requestlist[cnt] = __aio_find_req ((aiocb_union *) list[cnt]);
>  
>  	if (requestlist[cnt] != NULL)
> @@ -83,7 +91,10 @@
>        pthread_setcancelstate (PTHREAD_CANCEL_DISABLE, &oldstate);
>  
>        if (timeout == NULL)
> -	result = pthread_cond_wait (&cond, &__aio_requests_mutex);
> +        if (do_not_suspend)
> +          result = 0;
> +        else
> +          result = pthread_cond_wait (&cond, &__aio_requests_mutex);
>        else
>  	{
>  	  /* We have to convert the relative timeout value into an
> @@ -100,8 +111,11 @@
>  	      abstime.tv_sec += 1;
>  	    }
>  
> -	  result = pthread_cond_timedwait (&cond, &__aio_requests_mutex,
> -					   &abstime);
> +	  if (do_not_suspend)
> +	    result = 0;
> +	  else
> +	    result = pthread_cond_timedwait (&cond, &__aio_requests_mutex,
> +	                                     &abstime);
>  	}
>  
>        /* Now remove the entry in the waiting list for all requests
>
> ---- End patch ----
>

Last time you had a test program for all of these.  I'd like to have
this program as part of glibc's regression suite.  This means it
should live in rt as e.g. tst-aio-7.c and should return 0 in case of
success and 1 in case of an error.  Can you reformat it and change the
program taking your patches in account and send it?

Thanks,
Andreas
-- 
 Andreas Jaeger
  SuSE Labs aj@suse.de
   private aj@arthur.inka.de
    http://www.suse.de/~aj


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]