Skip to content

CQ: we should check the return value of lseek #5783

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

marisn
Copy link
Contributor

@marisn marisn commented May 28, 2025

An attempt to unify checks and error messages.

There are more places where lseek return value should be checked, but I don't have time to work on them now.

An attempt to unify checks and error messages
@marisn marisn added the C Related code is in C label May 28, 2025
@github-actions github-actions bot added raster Related to raster data processing libraries module imagery raster3d labels May 28, 2025
@marisn marisn requested a review from petrasovaa May 28, 2025 02:37
@wenzeslaus
Copy link
Member

Perhaps this already revealed some errors. It fails tests:

Running ./raster/r.random/testsuite/test_r_random.py...
========================================================================
FFFFFFFF
======================================================================
FAIL: test_random_raster (__main__.TestRasterTile)
Testing r.random  runs successfully
----------------------------------------------------------------------
Traceback (most recent call last):
  File "etc/python/grass/gunittest/case.py", line 1396, in assertModule
    module.run()
  File "etc/python/grass/pygrass/modules/interface/module.py", line 825, in run
    self.wait()
  File "etc/python/grass/pygrass/modules/interface/module.py", line 846, in wait
    raise CalledModuleError(
grass.exceptions.CalledModuleError: Module run `r.random input=landcover_1m npoints=20 seed=1 raster=landcover_1m_raster_random` ended with an error.
The subprocess ended with a non-zero return code: 1. See the following errors:
b'Collecting Stats...\n0..3..6..9..12..15..18..21..24..27..30..33..36..39..42..45..48..51..54..57..60..63..66..69..72..75..78..81..84..87..90..93..96..99..100\nERROR: Unable to seek: 29 Illegal seek\n'

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "raster/r.random/testsuite/test_r_random.py", line 59, in test_random_raster
    self.assertModule(
  File "etc/python/grass/gunittest/case.py", line 1416, in assertModule
    self.fail(self._formatMessage(msg, stdmsg))
AssertionError: Running <r.random> module ended with non-zero return code (1)
Called (Python): r.random(input='landcover_1m', npoints='20', raster='landcover_1m_raster_random', seed=1)
Called (Bash): r.random input=landcover_1m npoints=20 seed=1 raster=landcover_1m_raster_random
See the following errors:
Collecting Stats...
0..3..6..9..12..15..18..21..24..27..30..33..36..39..42..45..48..51..54..57..60..63..66..69..72..75..78..81..84..87..90..93..96..99..100
ERROR: Unable to seek: 29 Illegal seek


======================================================================

@marisn
Copy link
Contributor Author

marisn commented May 29, 2025

Perhaps this already revealed some errors. It fails tests:

Yes, as it just checks if a return value does not indicate on an error, this really is a bug in r.random. If I read the code correctly, there is no need to call lseek in r.random at all. I'll look into this tomorrow.

Rewinding a raster map can lead to lseek errors and generally
should not be done, as raster reading functions perform seek
on their own if required.
@marisn
Copy link
Contributor Author

marisn commented May 29, 2025

MacOS failure is caused by #5787

@echoix
Copy link
Member

echoix commented May 30, 2025

Concerning the message that you applied almost everywhere, isn't it similar to one of your examples last week where %d %s can't be reordered when requesting translations? (And they are collected for translation)

@nilason
Copy link
Contributor

nilason commented Jun 1, 2025

There shouldn’t be any reason to cast -1 to off_t when directly comparing against the results of lseek. Not a big deal, but just adds clutter (in my opinion).

@marisn marisn marked this pull request as draft June 9, 2025 07:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C Related code is in C imagery libraries module raster Related to raster data processing raster3d
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants