On Sun, Jan 11, 2009 at 8:10 AM, Mattias Ellert <mattias.ellert at fysast.uu.se> wrote: > 22 dec 2008 kl. 02.31 skrev m. allan noah: > >>> On Tue, Dec 9, 2008 at 10:48 AM, Mattias Ellert >>> <mattias.ellert at fysast.uu.se> wrote: >>>> >>>> m?n 2008-12-08 klockan 09:46 -0500 skrev m. allan noah: >>>>> >>>>> After some private mails with Ian, it seems this is a bug in >>>>> sane-avision: >>>>> >>>>> during sane_cancel(), the backend calls: sanei_thread_kill >>>>> (s->reader_pid), but s->reader_pid is 0, which signals the entire >>>>> group. There is a test to try and avoid this, but it relies on prior >>>>> code to have set s->reader_pid = -1, which has not happened in the >>>>> case of no paper. >>>>> >>>>> I just expanded the test to require a positive value, since the pid >>>>> should never be negative anyway? My fix has just been commited to CVS >>>>> (backend version 289 nice round number for Ford and Studebaker fans). >>>>> Ian and Rene- please test. >>>>> >>>>> allan >>>> >>>> This breaks the MacOS X port. The PID number (being a pointer) can be >>>> arbitrary large, and when cast to an integer it can easily overflow to a >>>> negative value. The code was fixed for this problem by removing all >>>> places where the code was checking for a PID > 0. For the avision >>>> backend this was done here: >>>> >>>> >>>> https://alioth.debian.org/plugins/scmcvs/cvsweb.php/sane-backends/backend/avision.c.diff?r1=1.38;r2=1.39;cvsroot=sane >>>> >>>> Your commit: >>>> >>>> >>>> https://alioth.debian.org/plugins/scmcvs/cvsweb.php/sane-backends/backend/avision.c.diff?r1=1.43;r2=1.44;cvsroot=sane >>>> >>>> reintroduces the problem fixed by the earlier commit. Please revert it >>>> and fix the new problem in a way that doesn't break the MacOS X port. >> >> Ok, so what is the correct fix? If OSX is using pthread, is it enough >> to make SANE_Pid pthread_t? >> >> allan > > > The SANE_Pid is properly declared as: > > #ifdef USE_PTHREAD > typedef long SANE_Pid; > #else > typedef int SANE_Pid; > #endif > > This gives the correct size for both fork and pthread on both 32 and 64 bit. > Changing it to pid_t and pthread_t instead of int and long would mean an > interface change (and we get into the change soname or not discussion again) > - you would also loose the abstraction achieved by using an opaque type in > the SANE API rather than the implementation specific types.
Correct me if i am wrong, but we are talking about sanei here, not the sane API. None of this is in the API. > Also since the SANE API states that a SANE_Pid value of -1 indicates an > error, the SANE_Pid must be a signed type. Where does it state this? I dont see SANE_Pid anywere in the API. Changing it to pthread_t (which > essentially is a pointer - hence an unsigned type) will break the API badly. > Any value for a unsigned type will always be different from -1 (a good > compiler will optimise the check away). > > The only thing that must remembered is that negative values for SANE_Pid are > valid (except for -1). You can not check for a valid SANE_Pid with (pid > > 0). Pointers could wrap to -1 as well. This fix is not sufficient. I think we can correct this the right way in sanei. allan -- "The truth is an offense, but not a sin"
