FS position method return valid position on error, should return -1

[drmpf]

Board

All

Device Description

ESP32
ESP8266 code base has the same issue

Hardware Configuration

all

Version

latest master (checkout manually)

IDE Name

Arduino

Operating System

Windows 10

Flash frequency

40Mhz

PSRAM enabled

yes

Upload speed

115200

Description

Current code returns valid file position on error, should return invalid position, i.e. -1

Current code is

size_t File::position() const {
  if (!*this) {
    return 0;
  }
  return _p->position();
}

should be

size_t File::position() const {
  if (!*this) {
    return -1;  // <<<<<<<<<<<<<<<<
  }
  return _p->position();
}

Sketch

Not needed

Debug Message

None

Other Steps to Reproduce

No response

I have checked existing issues, online documentation and the Troubleshooting Guide

• I confirm I have checked existing issues, online documentation and Troubleshooting guide.

[lbernstone]

size_t is unsigned int. Unless you just opened the file or did a seek to 0, it will be unusual to get a 0 position.

[drmpf]

The problem is 0 is being used as an ERROR return, but it is actually a valid return.
It is not an unusual return value.

It is perfectly valid to have one method work on the file handle and then pass it to another method for further processing and have the second method test to see if the file has been rewound, i.e. check the position returns 0.

In that case, if position() returns 0, how do I tell if the file has been rewound or if there is an error?

Other FS libraries return -1, which, cast to size_t, will result in a very large number which can be used to detect the error.
On the other hand the Arduino String library uses an int return for indexOf() and returns -1 for errors.
int (32bits for ESP32) would probably be large enough to use for the position() return.

[me-no-dev]

why would you depend on position to distinguish valid from invalid file? We could return -1 but this IMHO is invalid, because result is given in unsigned int. Feel free to add a pull request with the change.

[drmpf]

As a user of this API, I would expect a return in the valid range, i.e. 0 to some high number, to indicate that the file is actually in that position.
As it is if 0 is returned I don't know if it is safe to let the code proceed.

The 'fixes' for this seem to be either
i) return an out of range result, e.g. -1, cast to the result type, on error
ii) return an error via an 'out of band process', such as throwing an exception or set something like ferror.
iii) fix it in the documenation

I think i) is the preferrable way and this is what the Seeed Arduino FS library does.
However you don't seem keen on this.
ii) seems to out of keeping with the 'Arduino' style
If you don't want to fix this in the code, then iii) should be chosen to at least fixed it in the documentation for this library.
That is the description for this call should be documented along the lines of

/** position() 
** returns unsigned int
** if >0 returned, then that is the current position in the file
** if 0 returned, then the file state is indeterminate.  The position may be zero or the file may be invalid.
*/

strtol seems use all three 'fixes' to give you the worst of all worlds

Return value -- long
    If successful, an integer value corresponding to the contents of str is returned.
    If the converted value falls out of range of corresponding return type, a range error occurs (setting errno to ERANGE) and LONG_MAX, LONG_MIN is returned.
    If no conversion can be performed, ​0​ is returned. 

For some errors it returns an extreme value
For some errors it indicates the error via the 'out of band' variable ERANGE
For some errors it just returns 0 and documents that that may or may not be a valid result.

How would the maintainers of this library like to proceed?

[me-no-dev]

@drmpf I checked the very latest Arduino SD library (not ours, official SD lib) and it returns -1 as you propose. So such change would be accepted here, since our FS is modeled after the Arduino SD library.

[TD-er]

See #10360

This should be reverted as it causes all kinds of issues, since position() is being used in functions like peek() , seekDir() and available().
Some of which return this as an int or hand it over to a function which expects a signed type.