Skip to content
Snippets Groups Projects

Draft: sanei_thread: Draft fixup for sanei_thread issues.

Open Ralph Little requested to merge sanei_thread_fixup into master
3 unresolved threads

This is a first draft of an attempt to fix a number of issues in the sanei_thread API.

In particular, some backends are making some invalid assumptions about the underlying type of SANE_Pid. So I have converted SANE_Pid to a struct which highlights where backends (and some parts of sanei_thread) are assuming that SANE_Pid is a pthread_t or at least an arithmetic type.

Edited by Ralph Little

Merge request reports

Loading
Loading

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
  • added sanei label

  • assigned to @skelband

    • Author Maintainer

      @Povilas When you have a moment (and I know you are pretty busy), would you be able to review this set of changes to sanei_thread to see if you can spot any issues?

      Strictly speaking, apart from the addition of a new function sanei_thread_pid_compare() and the exposure of the existing function sanei_thread_pid_to_long() the API should be backwardly compatible.

      What I have done is convert the underlying type of SANE_Pid to be a struct containing the old definition of SANE_Pid and a new is_valid flag to determine validity. This is because we cannot reliably determine from the "value" of the underlying type what an invalid pid is.

      Apparently, this was noticed sometime ago and for the MAC platform, the build was converted from using threads to process and this broke libusb functionality. It is surmised that this is responsible for why MAC users have been having issues with scanning using SANE.

      I would be interested in your opinion of the approach. I will also put the general idea out the sane-devel for comment.

    • The current change is an API and ABI break of sanei. I think the most important point is whether sanei is a public interface and we commit to maintaining backwards compatibility, or it is a private interface to be used strictly by the backends within sane-backends repository.

      Documentation at http://www.sane-project.org/docs.html says "The sanei functions should be used by backend programmers whenever possible.". So it's not clear that external backends shouldn't use the API as we don't make this clear.

      On the other hand, we can't completely set the interfaces in stone. Personally I think in this case breaking the interface makes sense. We can increase the soversion of sanei, so if there are any external backends using sanei, the incompatibility will be noticed due to link errors, and there will still be option to provide the old sanei library.

      I would also go one step further and make SANE_Pid managed by sanei completely, thus avoiding any future problems with the ABI. That is, SANE_Pid should become pointer to void* and sanei should perform allocation and freeing of the underlying structure. This way we wouldn't even need is_valid field anymore because we could just check if the SANE_Pid pointer is NULL or not.

      If any of the backends needs direct access to the underlying data contained in the struct, we can make this possible via a separate function.

      Edited by Povilas Kanapickas
    • Author Maintainer

      My understanding is that 3rd party backends are likely making use of sanei_* so I don't think that there is any way that we can "fix" sanei_thread without risking some API breakage. :frowning2:

      Having said that, I believe that current backends and libsane-dll are statically linked to sanei so I don't think that a change to sanei_* would affect previously built binary backends but I will make sure that that is the case :thumbsup:

      Since I have added a couple of functions to the API, we should at least acknowledge that by advancing the version of the API. I'm not sure that we have an official "so version" of the API though although I stand to be corrected on that. There are aspects of the guts of SANE that I am still learning.

      You do make a good point about making SANE_Pid completely opaque by turning it into a pointer. I kinda like the opaque part. The one advantage of making it a struct is that (as far as C goes anyway) a lot of the currently errant behaviour is turned into compiler errors. Noted that we wouldn't require the flag if we used a pointer, but the downside is that backends that use uninitialised SANE_Pids (on systems that have an arithmetic pthread_t) might become crashes from garbage pointer dereferences, rather than errors from the pthread library.

      You have given me a lot to think about.

      Edited to add: I don't know if we currently have a way to manage the memory recovery of a SANE_Pid allocated from the heap in functions. There isn't anything in the API to "destroy" a SANE_Pid and adding something would require us to put extra burden on the backend. We could maintain a structure of the generated pointers in sanei_thread and use that for recovery and validation, thus avoiding the possibility of crashing: we could reject uninitialized SANE_Pid values safely. The overhead wouldn't be too onerous.

      Edited by Ralph Little
    • Please register or sign in to reply
  • Ralph Little mentioned in issue #153

    mentioned in issue #153

  • Ralph Little marked this merge request as draft

    marked this merge request as draft

  • Ralph Little added 1 commit

    added 1 commit

    • 1fc0928a - sanei_thread: correct silly error following quick review.

    Compare with previous version

    • As someone that doesn't know how SANE development works, I have a question. While this isn't an API breaking change, I believe it is an ABI breaking change, i.e. third party backends would need to recompile for the new version. Is that a concern?

    • Author Maintainer

      I don't think that we offer sanei_* as a binary interface from, for example, a libsane-sanei.so shared library. So it's not like third party backends are going to break, although I will thoroughly check some that I know of against the modified binaries.

    • Please register or sign in to reply
  • Tormod Volden
  • 162 143
    144 if (!pid.is_valid)
    145 {
    146 return 0l;
    147 }
    163 148 #ifdef WIN32
    164 149 #ifdef WINPTHREAD_API
    165 rc = (long) pid;
    150 rc = (long) pid.pid;
    166 151 #else
    167 rc = pid.p;
    152 rc = pid.pid.p;
    168 153 #endif
    169 154 #else
    170 rc = (long) pid;
    155 rc = (long) pid.pid;
    • Does that compile if pid.pid is a struct?

      You can also use #ifndef PTHREAD_T_IS_INTEGER and, at least for now, maybe return (long)(int_ptr)&pid.pid in that case (I don't know how these are allocated so how useful that is). Anyway this function seems to be only for debug messages - this could be commented too.

    • Author Maintainer

      I wasn't really sure what do to here. If pid.pid is indeed a structure, a suitable platform-specific way of generating a long might be evident to someone knowledgeable of that tech. I didn't feel that I could put in here anything for those platforms without some knowledge. I have access to an AIX system here: I will have a look to see what the deal is there. I honestly cannot remember from the last time I worked on it.

      I did add a comment above to address this a little:

       * Note: perhaps on platforms where pthread_t is a structure, compute a hash
       * of the structure. It's not ideal but it is a thought.

      Perhaps I will revisit this and perhaps add a #error or somesuch for the porter to indicate some needed work.

      Edit to add: pthread_t on AIX is an unsigned int.

      Edited by Ralph Little
    • Author Maintainer

      Regarding the possibility of using something like (long)(int_ptr)&pid.pid, I'm not sure. It would provide something but I would rather that someone knowledgeable of the platform were to get some notification that they need to do a little bit of work here. Personally, I would prefer #error and have them let us know what they added.

      The current version is just the same as before: I don't think that I added anything other than the required .pid member reference to make it compile so I don't think it is going to catastrophically break anything.

    • Please register or sign in to reply
  • Povilas Kanapickas
  • Ralph Little added 1 commit

    added 1 commit

    • 06241f00 - sanei_thread: Corrected some additional mistakes.

    Compare with previous version

  • Ralph Little mentioned in merge request !765

    mentioned in merge request !765

  • mentioned in issue #655 (closed)

  • Please register or sign in to reply
    Loading