fseek returns offset upon success instead of 0

[ragnarok2040]

I noticed this when I had fseek in an if statement, if fseek succeeds it is supposed to return 0 but it instead returns the offset of the file from the return of fioLseek. I fixed it by checking whether fioLseek returned with an error and setting ret to 0 if it had not. I'm not sure if that's the correct way to do it as I'm not familiar with fioLseek();, e.g. if it returns -1 when it doesn't succeed. This should fix fsetpos(); as well. I didn't have any problems building ps2sdk afterwards, but any code that relied on fseek returning an offset or code added afterwards that used fseek in a conditional statement might need fixing.

Edit:
I just took a look using codeblocks and there doesn't seem to be any code anywhere in ps2sdk that this fix would break. It should also fix the tests in test_fseek_ftell(); in stdio_tests.c.

More information:
http://www.opengroup.org/onlinepubs/007 ... fseek.html

Code: Select all

int fseek(FILE *stream, long offset, int origin)
{
  int ret;
  
  stream->has_putback = 0;

  switch(stream->type) {
    case STD_IOBUF_TYPE_NONE:
    case STD_IOBUF_TYPE_GS:
    case STD_IOBUF_TYPE_SIO:
    case STD_IOBUF_TYPE_STDOUTHOST:
      /* cannot seek stdout or stderr. */
      ret = -1;
      break;
    default:
      /* attempt to seek to offset from origin. */
      ret = fioLseek(stream->fd, (int)offset, origin);
  }
  return (ret);
}

should be

Code: Select all

int fseek(FILE *stream, long offset, int origin)
{
  int ret;
  
  stream->has_putback = 0;

  switch(stream->type) {
    case STD_IOBUF_TYPE_NONE:
    case STD_IOBUF_TYPE_GS:
    case STD_IOBUF_TYPE_SIO:
    case STD_IOBUF_TYPE_STDOUTHOST:
      /* cannot seek stdout or stderr. */
      ret = -1;
      break;
    default:
      /* attempt to seek to offset from origin. */
      if((fioLseek(stream->fd, (int)offset, origin)) != -1) {
        ret = 0;
      }
      else
        ret = -1;
  }
  return (ret);
}

[dlanor]

I noticed this when I had fseek in an if statement, if fseek succeeds it is supposed to return 0 but it instead returns the offset of the file from the return of fioLseek. I fixed it by checking whether fioLseek returned with an error and setting ret to 0 if it had not. I'm not sure if that's the correct way to do it as I'm not familiar with fioLseek();, e.g. if it returns -1 when it doesn't succeed.

I'm not 100% sure what the formal definition says about this, as I don't have the relevant Sony documents. But even without knowing this you can still modify your patch to make it 100% sure of compatibility.

You just need to change part of your patch from:

Code: Select all

      if((fioLseek(stream->fd, (int)offset, origin)) != -1) {
        ret = 0;
      }
      else
        ret = -1;

to:

Code: Select all

      if((fioLseek(stream->fd, (int)offset, origin)) >= 0) {
        ret = 0;
      }
      else
        ret = -1;

Thus any positive return value from fioLseek gets changed to a zero, while any negative return value gets changed to -1, so it works regardless of whether fioLseek can return detailed error codes or just -1. I think this flexibility may be important, as generic use of fioLseek really implies calling various unrelated device drivers that may differ in this regard. PS2SDK itself has no direct control over that, as such a device driver might not even be related to PS2SDK in any way.

but any code that relied on fseek returning an offset or code added afterwards that used fseek in a conditional statement might need fixing.

For non-error returns I don't see that as a problem, since any code usíng the standard C fseek function must also expect its standard return values. If it doesn't, then it is that code which is incorrect, and then it needs fixing on general principles anyway. The ANSI definition of fseek clearly states that the non-error return value should be zero and nothing else.

Interestingly enough, the same definition does NOT state that the return value for errors must be -1. Instead it simply states that the return value for those cases will be non-zero. This apparently implies that any non-zero values would be acceptable, so that we might as well pass on the PS2-style error codes (if any) from fioLseek.

For such implementation the critical part of the patch could then become:

Code: Select all

      ret = fioLseek(stream->fd, (int)offset, origin);
      if(ret >= 0)
        ret = 0;

This should fix the problem you ran into, just like your patch, while still holding a door open for detailed error return codes.

Best regards: dlanor

[ragnarok2040]

Nice, thanks dlanor. I created a patch from the implementation you suggested.

Code: Select all

diff -burN orig.ps2sdk/ee/libc/src/stdio.c ps2sdk/ee/libc/src/stdio.c
--- orig.ps2sdk/ee/libc/src/stdio.c	Sat Jul 28 12:00:45 2007
+++ ps2sdk/ee/libc/src/stdio.c	Sat Jul 28 12:11:54 2007
@@ -781,6 +781,8 @@
     default:
       /* attempt to seek to offset from origin. */
       ret = fioLseek(stream->fd, (int)offset, origin);
+      if (ret >= 0)
+        ret = 0;
   }
   return (ret);
 }

[ooPo]

Code: Select all

Sending        ee/libc/src/stdio.c
Transmitting file data .
Committed revision 1429.

Added to the repository.

[chp]

The lowlevel function returns a negative representation of what should be put in the errno variable when it fails (for example -EBADF). If you want to properly shape this up, you should also store that value in errno (converted into a positive value first of course) before returning -1.

[ragnarok2040]

Ahh, I see. I modified the fix so that it does that. It's kind of redundant now to return fseek with the negative errno value, but that shouldn't matter, heh.

Code: Select all

      ret = fioLseek(stream->fd, (int)offset, origin);
      if &#40;ret < 0&#41;
        errno = ret * &#40;-1&#41;;
      else
        ret = 0;