Opened 3 weeks ago

Last modified 3 weeks ago

#854 new defect

Avoid using lseek() to set file position

Reported by: Olly Betts Owned by: Olly Betts
Priority: normal Milestone: 2.1.1
Component: Backend-Glass Version: 2.1.0
Severity: normal Keywords:
Cc: Blocked By:
Blocking: Operating System: All

Description

Constructing a Database object for a glass database from a file descriptor currently uses lseek() to set the file position before reading the version file.

That's not such a problem when we're opening the database as the current file position is used as the offset in the file to read the database from, but a call to Database::reopen() will also do this.

We should ideally only make one call to lseek() to read (and not change) the file position when the database is opened, and then use pread() for all reads of the version file (like we already do for the database tables).

This would make it safer to use dup() to create an fd to pass to Xapian (since such an fd shares its file position with the fd it was created from).

Background: https://github.com/openzim/libzim/pull/1119

Change History (1)

comment:1 by Olly Betts, 3 weeks ago

I have a working patch for this which I plan to merge for the next 2.x release.

I tried to write a note for the docs about it, and it seems awkward to say that you can use dup() here safely provided the platform provides pread() and implements it with the correct POSIX semantics because as the author of code doing this you don't really know what platforms it may get run on, and they can't easily assert(current_platform_has_working_pread();.

I suspect there aren't really any Unix-like platforms without a properly working pread() nowadays, but it's hard to really know.

We could perhaps state it as a requirement for Xapian, but then we should really test for it. A configure test to check it wouldn't work when cross-compiling. We could test it in the testsuite, but not everyone runs that.

More broadly, we have quite a lot of code written to provide compatibility when some feature is missing or not working correctly, but some is 20+ years old at this point. Relevant here for example is that we have a workaround added in 2004 to block trying to use broken pread()/pwrite() on HP-UX (they didn't work when Large File Support was enabled); it's quite likely they aren't broken in more recent releases, though it's probably moot as Wikipedia says HP-UX was discontinued last year.

Another example is handling for O_CLOEXEC and related flags and functions not being available or being available but not working - when these were being added you needed to also be running a kernel with support, but at least for Linux everyone will be now and I'd expect all Unix-like platforms would have added support some time ago.

Note: See TracTickets for help on using tickets.