Handle HAVE_FSEEKO only in one place.
Versatile Commodore Emulator
Brought to you by:
blackystardust,
gpz
When compiling my port for Windows Embedded, I discovered that there are several sources that implements an #ifdef HAVE_FSEEKO ... #endif
and other sources that are using fseeko()
without testing if it's really supported (on Windows Embedded and Windows Mobile, it is not).
So, I was wondering what you think about moving these #ifdef HAVE_FSEEKO ... #endif
blocks from the sources into a common include file, so that it will exists only in one place and it will be used for all other files. I can also do a patch if you want, but unfortunately it is not exactly clear to me what is the best include file to be used for this purpose.
This is an example about what I'm talking about:
https://sourceforge.net/p/vice-emu/code/HEAD/tree/trunk/vice/src/core/ata.c#l53
I dont understand what this macro is trying to do - to me it looks like that either:
or
what am i missing there?
If you want to support disks larger than 2GB -- which is common with ATA drives I think -- the code must use fseeko()/ftello(). So we must disable ATA and thus IDE64 support if there is no fseeko()/ftello(), since we cannot just write our own version of fseeko()/ftello().
As an alternative, there's
fpos_t
for stream positions, using fgetpos()/fsetpos(). But that again depends on what thefpos_t
type can represent, might be a typedef forint
for all we know, which then won't help either.sounds like
a) this should be a configure option, else you get surprise compiling
b) that macro should indeed be at a central place and exist exactly once
I'd go the
fpos_t
route, fseek()/ftell() uselong int
as their offset argument, which on 64-bit Windows would still be 32-bit, so no fseek() beyond 2GB.fpos_t
is the type meant for file positions since C89.if that makes it work everywhere - sure. but does it? don't we exchange one set of functions we need to test for for another?
fgetpos()/fsetpos() are C89 functions, so no need to test. If those don't exists your compiler/libc is broken.
Problem is we use fseeko() in more than just ata.c, it's also in fsimage-probe.c, scsi.c and even util.c. So all those need updating, combined with the fact that
fpos_t
is an opaque type, you cannot print it or serialize it easily (for snapshots).off_t util_file_length(FILE *fp)
uses fseeko() without any macros, so that'll break anyway without fseeko(). And usingfpos_t
there cannot be done portably, so it'll need to be changed into using a stat() call.Ugh.
I've moved the macros into
vice.h
and removed all traces of individual #defines from other files. Please check if this now works.A proper fix for the various things I mentioned in this thread will have to wait until after the 3.6 release. So I'll keep this bug open.
I'm working on some archdep wrappers for
fseeko()
/ftello()
. On Windows when they aren't available (they are using msys2, but not with MSVC 2019), I use_fseeki64()
/_ftelli64()
.Now the question is: are those winapi functions available for CE/Mobile? Some googling indicates they might not be.
Another thing is
off_t
, which appears to live in<sys/types.h>
on Windows, I wonder if that is also true for CE/Mobile?The replacement for
util_file_length()
isarchdep_file_size()
. The attached patch replaces all calls toutil_file_length()
(except inutil.c
itself, that'll come once I removeutil_file_length()
.To the other devs: please check if I made any mistakes or missed something. (this should be a pull request...)
Never mind, commit r41912 broke the patch.
Actually, no, they are not available.
My port for Windows CE / Mobile / Embedded requires an extra test like this one:
although it would be better that configure script should detect
_fseeki64()
and_ftelli64()
functions too.However, I noticed that it would be better to put that code only in one place for easier maintenance.
Yes, on Windows CE / Mobile / Embedded
off_t
exists into<sys/types.h>
.However, I have to say that there is one source that gave to me an error because off_t was undeclared since
<sys/types.h>
was not included. Adding the check and the inclusion of this file solved my issue. But again, there several sources with code like this:https://sourceforge.net/p/vice-emu/code/HEAD/tree/trunk/vice/src/core/ata.c#l29
So, I'm thinking: how about moving this piece of code into
vice/src/types.h
?Afterall, this file is already included where the check on
HAVE_SYS_TYPES_H
exists.Last edit: Carlo Bramini 2022-03-10
I agree it's better to have configure detect
_ftelli64
and_fseeki64
and finally fall back to usingftell
andfseek
, and indeed havesrc/types.h
handleoff_t
, using a typedef for it ifoff_t
isn't available.That way we properly test for features instead of using assumptions about operating systems and libraries.
Meanwhile I've implemented using configure to detect
_fseeki64
/_ftelli64
and provide a fallback if even those don't exist, using oldfseek
/ftell
. A typedef foroff_t
is provided, if it doesn't exist, insrc/types.h
.what is the status of this?
I fixed it as well as I could. Should work in all cases and when there's no 64-bit
fseek()
/ftell()
variant we fall back to the "old" versions. If the C library for a specific OS doesn't have 64-bit fseek what more can we do?so this can be closed/fixed? :)
Yeah, no activity in a year, so I'll assume it now works =)
Closing.