Changeset View
Standalone View
sys/sys/aio.h
Show All 21 Lines | |||||
#define _SYS_AIO_H_ | #define _SYS_AIO_H_ | ||||
#include <sys/types.h> | #include <sys/types.h> | ||||
#include <sys/signal.h> | #include <sys/signal.h> | ||||
#ifdef _KERNEL | #ifdef _KERNEL | ||||
#include <sys/queue.h> | #include <sys/queue.h> | ||||
#include <sys/event.h> | #include <sys/event.h> | ||||
#include <sys/signalvar.h> | #include <sys/signalvar.h> | ||||
#include <sys/uio.h> | |||||
#endif | #endif | ||||
/* | /* | ||||
* Returned by aio_cancel: | * Returned by aio_cancel: | ||||
*/ | */ | ||||
#define AIO_CANCELED 0x1 | #define AIO_CANCELED 0x1 | ||||
#define AIO_NOTCANCELED 0x2 | #define AIO_NOTCANCELED 0x2 | ||||
#define AIO_ALLDONE 0x3 | #define AIO_ALLDONE 0x3 | ||||
/* | /* | ||||
* LIO opcodes | * LIO opcodes | ||||
*/ | */ | ||||
#define LIO_NOP 0x0 | #define LIO_NOP 0x0 | ||||
#define LIO_WRITE 0x1 | #define LIO_WRITE 0x1 | ||||
#define LIO_READ 0x2 | #define LIO_READ 0x2 | ||||
#ifdef _KERNEL | #ifdef _KERNEL | ||||
#define LIO_SYNC 0x3 | #define LIO_SYNC 0x3 | ||||
#define LIO_MLOCK 0x4 | #define LIO_MLOCK 0x4 | ||||
#define LIO_WRITEV 0x5 | |||||
#define LIO_READV 0x6 | |||||
#endif | #endif | ||||
/* | /* | ||||
* LIO modes | * LIO modes | ||||
*/ | */ | ||||
#define LIO_NOWAIT 0x0 | #define LIO_NOWAIT 0x0 | ||||
#define LIO_WAIT 0x1 | #define LIO_WAIT 0x1 | ||||
Show All 31 Lines | |||||
}; | }; | ||||
/* | /* | ||||
* I/O control block | * I/O control block | ||||
*/ | */ | ||||
typedef struct aiocb { | typedef struct aiocb { | ||||
int aio_fildes; /* File descriptor */ | int aio_fildes; /* File descriptor */ | ||||
off_t aio_offset; /* File offset for I/O */ | off_t aio_offset; /* File offset for I/O */ | ||||
union { | |||||
volatile void *aio_buf; /* I/O buffer in process space */ | volatile void *aio_buf; /* I/O buffer in process space */ | ||||
struct iovec *aio_iov; /* I/O scatter/gather list */ | |||||
}; | |||||
union { | |||||
size_t aio_nbytes; /* Number of bytes for I/O */ | size_t aio_nbytes; /* Number of bytes for I/O */ | ||||
int aio_iovcnt; /* Length of aio_iov */ | |||||
kib: aio_iovcnt is count of array elements, why it is int and not size_t, at least ? If this is… | |||||
Done Inline ActionsI did it to match the signature of pwritev, where iovcnt is an int. Do you think I should change it? asomers: I did it to match the signature of `pwritev`, where `iovcnt` is an int. Do you think I should… | |||||
Not Done Inline ActionsI think it is a type error in preadv/pwritev as well. But I see that our preadv/pwritev have a signature matching Linux implementation. It is too late to fix this, perhaps. Ok then. kib: I think it is a type error in preadv/pwritev as well. But I see that our preadv/pwritev have a… | |||||
}; | |||||
Not Done Inline ActionsAnonymous unions where added in C11. In other words, this change as is breaks system headers use in C99 mode. Also AFAIR they are not in C++11, but you might want to recheck this claim. kib: Anonymous unions where added in C11. In other words, this change as is breaks system headers… | |||||
Done Inline ActionsIs it a requirement for our headers to all be strictly C99 compatible? I notice that we already have many anonymous unions that aren't guarded by #ifdef KERNEL. For example, struct kinfo_file includes an anonymous union. Even so, I could do the stupid "named union with #defines for each element` hack, if you would prefer. asomers: Is it a requirement for our headers to all be strictly C99 compatible? I notice that we… | |||||
Done Inline ActionsI think it is a reasonable goal to have system headers usable for userspace in as many situations as possible. Note that kinfo/user.h is very much FreeBSD-specific header, while aio.h is POSIX-mandated. POSIX still requires compliance with C99, so if we cannot provide aio.h usable with C99, we already lost POSIX. kib: I think it is a reasonable goal to have system headers usable for userspace in as many… | |||||
Done Inline ActionsThe "named union with #defines" hack didn't work very well, because it clashed with local variables and other structs' fields that shared the same name. What do you think about this alternative? We're still POSIX compliant, if the right macros are defined. But we don't have to hack up the structure or abuse the preprocessor too badly. asomers: The "named union with #defines" hack didn't work very well, because it clashed with local… | |||||
Not Done Inline ActionsCan you show the hack that 'did not work very well'. Your patch currently provides different ABI for struct aiocb depending on the compiler flags, and most likely does not work with C++. Even if you fixed what you intended to do, it is ugly and really not needed. We have named unions working in more complex situations without a glitch. kib: Can you show the hack that 'did not work very well'.
Your patch currently provides different… | |||||
Done Inline ActionsHow does it provide different ABIs depending on the compiler flags? The structure layout is the same. Here is the named union hack: --- a/sys/sys/aio.h +++ b/sys/sys/aio.h @@ -96,13 +96,13 @@ typedef struct aiocb { int aio_fildes; /* File descriptor */ off_t aio_offset; /* File offset for I/O */ union { - volatile void *aio_buf; /* I/O buffer in process space */ - struct iovec *aio_iov; /* I/O scatter/gather list */ - }; + volatile void *aio_un1_buf; /* I/O buffer in process space */ + struct iovec *aio_un1_iov; /* I/O scatter/gather list */ + } aio_buf_un; union { - size_t aio_nbytes; /* Number of bytes for I/O */ - int aio_iovcnt; /* Length of aio_iov */ - }; + size_t aio_un2_nbytes; /* Number of bytes for I/O */ + int aio_un2_iovcnt; /* Length of aio_iov */ + } aio_nbytes_un; int __spare__[2]; void *__spare2__; int aio_lio_opcode; /* LIO opcode */ @@ -111,6 +111,11 @@ typedef struct aiocb { struct sigevent aio_sigevent; /* Signal to deliver */ } aiocb_t; +#define aio_buf aio_buf_un.aio_un1_buf +#define aio_iov aio_buf_un.aio_un1_iov +#define aio_nbytes aio_nbytes_un.aio_un2_nbytes +#define aio_iovcnt aio_nbytes_un.aio_un2_iovcnt + #ifdef _KERNEL typedef void aio_cancel_fn_t(struct kaiocb *); The aio_buf macro conflicted with the field of the same name in struct oaiocb, struct oaiocb32 and struct aiocb32. It would also conflict with any similarly named struct definitions or local variables in user code. asomers: How does it provide different ABIs depending on the compiler flags? The structure layout is… | |||||
Not Done Inline ActionsFirst, lets clear my note about ABI difference. I thought that your union hack changes alignment requirements but it seems that you are right. That said, several notes:
But looking at the stuff from some distance, why do you need aio_iov at all ? aio_buf is typed so that assignment of struct iovec * to it should just work. Then aio_nbytes is size_t. So if you use aio_buf/aio_nbytes as the pointer to iovec and counter, everything would work without the surgery on the struct aiocb. kib: First, lets clear my note about ABI difference. I thought that your union hack changes… | |||||
Done Inline ActionsYou mean why not just overload aio_buf and aio_nbytes? I certainly could've. I used the union instead because I thought it would be clearer. As a matter of style, I don't like reusing even local variables for different purposes, let alone struct members. I'll update the review to show you what that option looks like. asomers: You mean why not just overload aio_buf and aio_nbytes? I certainly could've. I used the union… | |||||
int __spare__[2]; | int __spare__[2]; | ||||
void *__spare2__; | void *__spare2__; | ||||
int aio_lio_opcode; /* LIO opcode */ | int aio_lio_opcode; /* LIO opcode */ | ||||
int aio_reqprio; /* Request priority -- ignored */ | int aio_reqprio; /* Request priority -- ignored */ | ||||
struct __aiocb_private _aiocb_private; | struct __aiocb_private _aiocb_private; | ||||
struct sigevent aio_sigevent; /* Signal to deliver */ | struct sigevent aio_sigevent; /* Signal to deliver */ | ||||
} aiocb_t; | } aiocb_t; | ||||
Show All 22 Lines | struct kaiocb { | ||||
int msgrcv; /* (*) messages received */ | int msgrcv; /* (*) messages received */ | ||||
struct proc *userproc; /* (*) user process */ | struct proc *userproc; /* (*) user process */ | ||||
struct ucred *cred; /* (*) active credential when created */ | struct ucred *cred; /* (*) active credential when created */ | ||||
struct file *fd_file; /* (*) pointer to file structure */ | struct file *fd_file; /* (*) pointer to file structure */ | ||||
struct aioliojob *lio; /* (*) optional lio job */ | struct aioliojob *lio; /* (*) optional lio job */ | ||||
struct aiocb *ujob; /* (*) pointer in userspace of aiocb */ | struct aiocb *ujob; /* (*) pointer in userspace of aiocb */ | ||||
struct knlist klist; /* (a) list of knotes */ | struct knlist klist; /* (a) list of knotes */ | ||||
struct aiocb uaiocb; /* (*) copy of user I/O control block */ | struct aiocb uaiocb; /* (*) copy of user I/O control block */ | ||||
struct uio uio; /* (*) storage for non-vectored uio */ | |||||
struct iovec iov[1]; /* (*) storage for non-vectored uio */ | |||||
struct uio *uiop; /* (*) Possibly malloced uio */ | |||||
ksiginfo_t ksi; /* (a) realtime signal info */ | ksiginfo_t ksi; /* (a) realtime signal info */ | ||||
uint64_t seqno; /* (*) job number */ | uint64_t seqno; /* (*) job number */ | ||||
aio_cancel_fn_t *cancel_fn; /* (a) backend cancel function */ | aio_cancel_fn_t *cancel_fn; /* (a) backend cancel function */ | ||||
aio_handle_fn_t *handle_fn; /* (c) backend handle function */ | aio_handle_fn_t *handle_fn; /* (c) backend handle function */ | ||||
union { /* Backend-specific data fields */ | union { /* Backend-specific data fields */ | ||||
struct { /* BIO backend */ | |||||
int nbio; /* Number of remaining bios */ | |||||
int error; /* Worst error of all bios */ | |||||
long nbytes; /* Bytes completed so far */ | |||||
}; | |||||
struct { /* fsync() requests */ | struct { /* fsync() requests */ | ||||
int pending; /* (a) number of pending I/O */ | int pending; /* (a) number of pending I/O */ | ||||
}; | }; | ||||
struct { /* socket backend */ | struct { /* socket backend */ | ||||
void *backend1; | void *backend1; | ||||
long backend3; | long backend3; | ||||
int backend4; | int backend4; | ||||
}; | }; | ||||
▲ Show 20 Lines • Show All 49 Lines • ▼ Show 20 Lines | |||||
struct timespec; | struct timespec; | ||||
__BEGIN_DECLS | __BEGIN_DECLS | ||||
/* | /* | ||||
* Asynchronously read from a file | * Asynchronously read from a file | ||||
*/ | */ | ||||
int aio_read(struct aiocb *); | int aio_read(struct aiocb *); | ||||
#if __BSD_VISIBLE | |||||
int aio_readv(struct aiocb *); | |||||
#endif | |||||
/* | /* | ||||
* Asynchronously write to file | * Asynchronously write to file | ||||
*/ | */ | ||||
int aio_write(struct aiocb *); | int aio_write(struct aiocb *); | ||||
#if __BSD_VISIBLE | |||||
int aio_writev(struct aiocb *); | |||||
#endif | |||||
/* | /* | ||||
* List I/O Asynchronously/synchronously read/write to/from file | * List I/O Asynchronously/synchronously read/write to/from file | ||||
* "lio_mode" specifies whether or not the I/O is synchronous. | * "lio_mode" specifies whether or not the I/O is synchronous. | ||||
* "acb_list" is an array of "nacb_listent" I/O control blocks. | * "acb_list" is an array of "nacb_listent" I/O control blocks. | ||||
* when all I/Os are complete, the optional signal "sig" is sent. | * when all I/Os are complete, the optional signal "sig" is sent. | ||||
*/ | */ | ||||
int lio_listio(int, struct aiocb *__restrict const *__restrict, int, | int lio_listio(int, struct aiocb *__restrict const *__restrict, int, | ||||
▲ Show 20 Lines • Show All 42 Lines • Show Last 20 Lines |
aio_iovcnt is count of array elements, why it is int and not size_t, at least ? If this is done to match the uio_iovcnt type, then arguably it is type error (fortunately kernel-only).
It would definitely require an overflow check on lowering type rank assignment.