Hi Jeremy! Here I attach patches, that make AIO code COMPILABLE under FreeBSD. Due incomplete implementation of signal handling on sigaction() for AIO code it doesn't work at least for FreeBSD 5.x. Still, for completeness, I think, this code should be there. Basically, changes are quite simple. In configure: Amasingly, members of sigevent struct in BSD have a bit different name, sigval_int vs. sival_int. Weird, I know. So, there is a check against structure members and later usage in the code of defined macros. BTW, there is usage of aio.h to find timespec struct - I don't think it's necessary. But I could be wrong :) Second check is for the presence of aio_read. In BSD this function(s) is a part of the libc, so no need to link against(non-existing) librt. I hope, this is a right code, but feel free to modify it. As for the aio.c patch: There is no SIGRTMIN macro in BSD, so NSIG is used as the boundary. Maybe, adding 3 to it would be nice too. Other macro used to mask difference in the names of structure members. Another change is quiter important - usage of sival_int instead of sival_ptr. There are two reasons - one, is that int is more than enough to pass mid. Second is is more serious - as on BSD this elements are not initialised, value is always a 0, which just sigsev when you try to dereference it. So, we should either use int instead and avoid conversion or at least, check, that value not zero. Of cource, that should be consistent cross the code. And last change - in handle_aio_completed - a chech, that aio_ex is not NULL. It's the result of failed find_aio_ex and not guarded at least in wait_for_aio_completion. With that minor changes aio code compiles on FreeBSD. It still gives short read, as the mid not really passed on signal delivery, but I hope it'll be fixed at least in 6.x. Sorry for being so verbose :) Just wanted to make things clear :) With regards, Timur.
Created attachment 1338 [details] Configure checks
Created attachment 1339 [details] AIO code
This part of your patch : if (signals_received < AIO_PENDING_SIZE - 1) { - aio_pending_array[signals_received] = *(uint16 *)(info->si_value.sival_ptr); + aio_pending_array[signals_received] = (uint16)(info->si_value.sival_int); signals_received++; makes no sense and will break the Linux implementation. Note that the original code sets sival_ptr to be : a->aio_sigevent.sigev_value.sival_ptr = (void *)&aio_ex->mid; That's the *address* of aio_ex->mid, not aio_ex->mid itself. Your patch casts the value into a uint16. It isn't - it's a *pointer*. Please tidy this up and remove some of the spurious whitespace changes. I need to see a much small patch before I can accept this, sorry. Jeremy.
(In reply to comment #3) > > makes no sense and will break the Linux implementation. Note that the original > code sets sival_ptr to be : > > a->aio_sigevent.sigev_value.sival_ptr = (void *)&aio_ex->mid; > > That's the *address* of aio_ex->mid, not aio_ex->mid itself. Your patch casts > the value into a uint16. It isn't - it's a *pointer*. Please tidy this up and > remove some of the spurious whitespace changes. I need to see a much small patch > before I can accept this, sorry. As I wrote in my longish comment, I need your decission - would you preffer to just switch to passing int(as mid fits there) or to put a guardian code around that assignment, that will check, that pointer isn't a zero. Both approaches are ok and it's just a question of your preference. Cheers, Timur.
Created attachment 1340 [details] sival_int based code This is the variant, that uses sival_int to pass the mid.
Jeremy, could you see if the current patches still need more work? If so, then we can look for inclusion in 3.0.21
Ok I'm happy with the change from sival_ptr to sival_int. I'll try and get this in for 3.0.21. Jeremy.
Created attachment 1536 [details] patch Is this enough for FreeBSD ? Or do you need more ? Jeremy.
(In reply to comment #8) > Created an attachment (id=1536) [edit] > patch > > Is this enough for FreeBSD ? Or do you need more ? > Jeremy. At least, it's also necessary to run configure checks(https://bugzilla.samba.org/attachment.cgi?id=1338) and use it's results: --- aio.c.orig Thu Jul 28 01:14:47 2005 +++ aio.c Thu Jul 28 01:18:43 2005 @@ -25,7 +25,17 @@ /* The signal we'll use to signify aio done. */ #ifndef RT_SIGNAL_AIO -#define RT_SIGNAL_AIO (SIGRTMIN+3) +#ifndef SIGRTMIN +#define SIGRTMIN NSIG +#endif +#define RT_SIGNAL_AIO (SIGRTMIN+3) +#endif + +#ifndef HAVE_STRUCT_SIGEVENT_SIGEV_VALUE_SIVAL_PTR +#ifdef HAVE_STRUCT_SIGEVENT_SIGEV_VALUE_SIGVAL_PTR +#define sival_int sigval_int +#define sival_ptr sigval_ptr +#endif #endif > There is no SIGRTMIN macro in BSD, so NSIG is used as the boundary. Maybe, > adding 3 to it would be nice too. Other macro used to mask difference in the > names of structure members. > > And last change - in handle_aio_completed - a chech, that aio_ex is not NULL. > It's the result of failed find_aio_ex and not guarded at least in > wait_for_aio_completion. It's also maybe reasonable to leave that sanity check: @@ -499,6 +509,11 @@ static BOOL handle_aio_completed(struct aio_extra *aio_ex, int *perr) { int err; + + if(!aio_ex) { + DEBUG(3, ("handle_aio_completed: Non-existing aio_ex passed\n")); + return False; + } The rest, I guess, is fine. With regards, Timur
Ok I may put the SIGRTMIN definition in the configure tests - SIGRTMIN is a POSIX realtime requirement, so it *should* be there (this is a bug in FreeBSD IMHO). The other check isn't right - we *need* to crash in this case (IMHO). I've guarded it in all places it's called - we don't want this to silently fail. Jeremy.
(In reply to comment #10) > Ok I may put the SIGRTMIN definition in the configure tests - SIGRTMIN is a > POSIX realtime requirement, so it *should* be there (this is a bug in FreeBSD > IMHO). The other check isn't right - we *need* to crash in this case (IMHO). > I've guarded it in all places it's called - we don't want this to silently fail. > Jeremy. I guess, configure check is a right thing(tm). And agree, it is something wrong on FreeBSD side... As for the sanity check - I think that sigsegv isn't the best way of shuting down the server. So, at least an assertion would be nice... Still I'm not sure, that AIO logic ensures, that all the signals will be delivered/handled. But I'm not very familiar with it anyhow :) Cheers, Timur.
This PR seems to be a bit obsolete, taking into account that AIO code was removed from 4.x.