grass CQ: we should check the return value of lseek

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.

May 28 '25 02:05 marisn

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


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

May 28 '25 13:05 wenzeslaus

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.

May 29 '25 00:05 marisn

MacOS failure is caused by #5787

May 29 '25 17:05 marisn

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)

May 30 '25 20:05 echoix

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).

Jun 01 '25 01:06 nilason

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).

That is if we don't want to be K&R compatible any more ;-)

Jul 19 '25 17:07 marisn

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)

Apparently %n$ is a POSIX extension to C. It is supported by all POSIX compilers, including MSVC, but will fail to work in strict ISO C mode. Thus either we abandon strict ISO C requirement or %n$ can not be used.

Jul 19 '25 19:07 marisn

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)

Apparently %n$ is a POSIX extension to C. It is supported by all POSIX compilers, including MSVC, but will fail to work in strict ISO C mode. Thus either we abandon strict ISO C requirement or %n$ can not be used.

I think I read since that comment, that they support positional placeholders, but through a different function name that supports positional explicitly (or maybe even exclusively). I don't think there's this distinction in other platforms

Jul 19 '25 21:07 echoix

What do you think about the message? I think that everyone needs that comment about what this refers to, not only the translators.

Jul 20 '25 07:07 wenzeslaus

I think I read since that comment, that they support positional placeholders, but through a different function name that supports positional explicitly (or maybe even exclusively). I don't think there's this distinction in other platforms

If you refer here to MS C compiler, then yes – we will have to IFDEF code in G_message et al.

From the documentation: „By default, if no positional formatting is present, the positional functions behave identically to the non-positional ones.“ Thus there would be no problems to replace *printf with _*printf_p in messaging functions.

Jul 20 '25 09:07 marisn

What do you think about the message? I think that everyone needs that comment about what this refers to, not only the translators.

I am not good at this. Like „File seek error with code %d: %s“?

Jul 20 '25 10:07 marisn

What do you think about the message? I think that everyone needs that comment about what this refers to, not only the translators.

I am not good at this. Like „File seek error with code %d: %s“?

What about: "File seek error: %s (%d)", which would have resulted in e.g. "ERROR: File seek error: Illegal seek (29)"?

Jul 21 '25 11:07 nilason

What do you think about the message? I think that everyone needs that comment about what this refers to, not only the translators.

I am not good at this. Like „File seek error with code %d: %s“?

What about: "File seek error: %s (%d)", which would have resulted in e.g. "ERROR: File seek error: Illegal seek (29)"?

Or skip the ambivalent word "seek" with e.g. "File read/write operation failed: %s (%d)", giving "ERROR: File read/write operation failed: Illegal seek (29)".

Jul 21 '25 11:07 nilason

Replacing the word "seek" is the way go. More specificity about what file and where would be nice, but as a general and quick way how to implement this, the last suggestion is good.

Jul 21 '25 13:07 wenzeslaus

Replacing the word "seek" is the way go. More specificity about what file and where would be nice, but as a general and quick way how to implement this, the last suggestion is good.

Feel free to write a custom error message for each case after I merge this PR :P

Sep 29 '25 14:09 marisn

Is there anything else appart reading and resolving the last comments remaining to do in this PR?

Oct 22 '25 02:10 echoix

Is there anything else appart reading and resolving the last comments remaining to do in this PR?

I don't think so. Code cleaning and improving is a never ending story, let's get this in (and off my neck).

Oct 25 '25 17:10 marisn

I'll launch manually this week's updating of translations right after having this merged, to not have it wait another week

Oct 25 '25 17:10 echoix