| Re: [hatari-devel] opened filehandles not saved/restored in memory snapshot with gemdos hd ? |
[ Thread Index |
Date Index
| More lists.tuxfamily.org/hatari-devel Archives
]
Hi Nicolas,
Attached is a patch showing what I think GEMDOS file handle
save & restore (with dummy DTA handling) would look like.
I've only tested that it builds, and that things don't explode when
using save & restore when there are *no* open file handles
(according to "info gemdos" in debugger).
Can you test the rest?
- Eero
On 12/14/18 2:33 AM, Eero Tamminen wrote:
Hi,
On 12/10/18 10:25 AM, Nicolas Pomarède wrote:
Le 09/12/2018 à 23:00, Eero Tamminen a écrit :
On 12/9/18 2:41 PM, Nicolas Pomarède wrote:
while debugging a program, I noticed that this program didn't work
correctly anymore once it was started from a gemdos hd, saved and
restored from a memory snapshot.
This program is opening an MP2 music file at start and read blocks
of 64K bytes in a loop to send them to the dsp, using GEMDOS FREAD $3F.
From what I see, when memory is saved during playback, the content
of FileHandles[] is not saved for all opened files having
bUsed=true. So on restore, even if the file handle is still opened
from TOS' point of view, it's not opened anymore from the host OS'
point of view and FREAD returns error -37 "invalid file handle"
Shouldn't the content of FileHandles[] be saved and then when
restoring reopen all "szActualName" files' path for which bUsed is
true ? Or could this have some bad side effects ?
I can't think of any outright.
A problem of course is if file cannot be found any more from
the same host path. I'm not sure whether Hatari should:
* Give user error about restore failing and either abort, or reset
emulation as it's in inconsistent state
* Warn user about missing file(s) and tell that GEMDOS HD state
doesn't match restored state, but still continue
I'd rather display a warning and continue the restore anyway. In all
cases, gemdos.c will report an error to TOS (as it's already the
case), so nothing should crash, just a bad filehandle error that the
emulated program should handle.
the seek/f_pos of each host's filehandles should also be saved along
with the path to be sure the file is reopened and restored at the same
position.
It's might not be a common case that programs keep files open for
such a long time, but saving/restoring this state would be handy.
Can you propose a patch?
not at the moment, sorry. But I will gladly test any patch that could
be sent :)
Ok, I started looking into it.
It will need to handle also forced file handles and cached DTA
entries.
Former can be saved/restored as-is, but I think I'll just invalidate
info for all (256) DTA entries as they can contain arbitrary number of
dynamic string allocations, which would make DTAs use "random" amount
of space from memory snapshot.
I.e. Atari program needs to call Fsfirst() before further Fsnext()
calls will provide matches. I don't think that's a problem unless
one makes a snapshot in middle of file selector or desktop directory
window refreshing.
Current DTA handling can do something bogus because memory snapshot
restore will restore Hatari magic values to Atari side DTAs, but their
DTA index can currently refer to something unused in internal Hatari
DTAs.
-> I added a check for whether the DTA index is actually in use
by Hatari.
- Eero
diff -r 202cf7c4f230 src/gemdos.c
--- a/src/gemdos.c Fri Dec 14 02:14:11 2018 +0200
+++ b/src/gemdos.c Sat Dec 15 02:43:52 2018 +0200
@@ -125,6 +125,7 @@
typedef struct
{
bool bUsed;
+ char szMode[4]; /* enough for all used fopen() modes: rb/rb+/wb+ */
Uint32 Basepage;
FILE *FileHandle;
/* TODO: host path might not fit into this */
@@ -340,7 +341,7 @@
/*-----------------------------------------------------------------------*/
/**
- * Clear given DTA structure.
+ * Clear given DTA cache structure.
*/
static void ClearInternalDTA(int idx)
{
@@ -358,6 +359,20 @@
InternalDTAs[idx].bUsed = false;
}
+/*-----------------------------------------------------------------------*/
+/**
+ * Clear all DTA cache structures.
+ */
+static void GemDOS_ClearAllInternalDTAs(void)
+{
+ int i;
+ for(i = 0; i < ARRAY_SIZE(InternalDTAs); i++)
+ {
+ ClearInternalDTA(i);
+ }
+ DTAIndex = 0;
+}
+
/*-----------------------------------------------------------------------*/
/**
@@ -509,17 +524,11 @@
*/
void GemDOS_Init(void)
{
- int i;
bInitGemDOS = false;
GemDOS_ClearAllFileHandles();
-
- /* Clear DTAs */
- for(i = 0; i < ARRAY_SIZE(InternalDTAs); i++)
- {
- ClearInternalDTA(i);
- }
- DTAIndex = 0;
+ GemDOS_ClearAllInternalDTAs();
+
}
/*-----------------------------------------------------------------------*/
@@ -799,11 +808,95 @@
/*-----------------------------------------------------------------------*/
/**
+ * Save file handle info. If handle is used, save valid file modification
+ * timestamp and file position, otherwise dummies.
+ */
+static void save_file_handle_info(FILE_HANDLE *handle)
+{
+ struct timespec mtime;
+ struct stat fstat;
+ off_t offset;
+
+ MemorySnapShot_Store(&handle->bUsed, sizeof(handle->bUsed));
+ MemorySnapShot_Store(&handle->szMode, sizeof(handle->szMode));
+ MemorySnapShot_Store(&handle->Basepage, sizeof(handle->Basepage));
+ MemorySnapShot_Store(&handle->szActualName, sizeof(handle->szActualName));
+ if (handle->bUsed)
+ {
+ offset = ftello(handle->FileHandle);
+ stat(handle->szActualName, &fstat);
+ mtime = fstat.st_mtim; /* modification time */
+ }
+ else
+ {
+ /* avoid warnings about access to unset memory */
+ offset = 0;
+ stat("/", &fstat);
+ mtime = fstat.st_mtim;
+ }
+ MemorySnapShot_Store(&mtime, sizeof(mtime));
+ MemorySnapShot_Store(&offset, sizeof(offset));
+}
+
+/*-----------------------------------------------------------------------*/
+/**
+ * Restore saved file handle info. If handle is used, open file,
+ * validate that file modification timestamp matches, then seek to saved
+ * position. Things done on disk must match order in save_file_info()
+ */
+static void restore_file_handle_info(int i, FILE_HANDLE *handle)
+{
+ struct timespec mtime;
+ struct stat fstat;
+ off_t offset;
+ FILE *fp;
+
+ if (handle->bUsed)
+ fclose(handle->FileHandle);
+
+ /* read all to proceed correctly in snapshot */
+ MemorySnapShot_Store(&handle->bUsed, sizeof(handle->bUsed));
+ MemorySnapShot_Store(&handle->szMode, sizeof(handle->szMode));
+ MemorySnapShot_Store(&handle->Basepage, sizeof(handle->Basepage));
+ MemorySnapShot_Store(&handle->szActualName, sizeof(handle->szActualName));
+ MemorySnapShot_Store(&mtime, sizeof(mtime));
+ MemorySnapShot_Store(&offset, sizeof(offset));
+
+ if (!handle->bUsed)
+ return;
+
+ if (stat(handle->szActualName, &fstat) != 0)
+ {
+ handle->bUsed = false;
+ Log_Printf(LOG_WARN, "GEMDOS handle %d cannot be restored, file missing: %s\n",
+ i, handle->szActualName);
+ return;
+ }
+ if (memcmp(&(fstat.st_mtim), &mtime, sizeof(mtime)) != 0)
+ {
+ Log_Printf(LOG_WARN, "restored GEMDOS handle %d points to a file that has been modified in meanwhile: %s\n",
+ i, handle->szActualName);
+ }
+ fp = fopen(handle->szActualName, handle->szMode);
+ if (fp == NULL || fseeko(fp, offset, SEEK_SET) != 0)
+ {
+ handle->bUsed = false;
+ Log_Printf(LOG_WARN, "GEMDOS '%s' handle %d cannot be restored, seek to saved offset %lld failed for: %s\n",
+ handle->szMode, i, offset, handle->szActualName);
+ fclose(fp);
+ return;
+ }
+ handle->FileHandle = fp;
+}
+
+/*-----------------------------------------------------------------------*/
+/**
* Save/Restore snapshot of local variables('MemorySnapShot_Store' handles type)
*/
void GemDOS_MemorySnapShot_Capture(bool bSave)
{
- int i, dummy;
+ FILE_HANDLE *finfo;
+ int i, handles = ARRAY_SIZE(FileHandles);
bool bEmudrivesAvailable;
/* Save/Restore the emudrives structure */
@@ -849,18 +942,31 @@
}
}
- /* Save/Restore details */
- MemorySnapShot_Store(&DTAIndex,sizeof(DTAIndex));
+ /* misc information */
MemorySnapShot_Store(&bInitGemDOS,sizeof(bInitGemDOS));
MemorySnapShot_Store(&act_pd, sizeof(act_pd));
- MemorySnapShot_Store(&dummy, sizeof(dummy)); /* redundant DTA_Gemdos */
- MemorySnapShot_Store(&CurrentDrive,sizeof(CurrentDrive));
- /* Don't save file handles as files may have changed which makes
- * it impossible to get a valid handle back
- */
- if (!bSave)
+ MemorySnapShot_Store(&CurrentDrive, sizeof(CurrentDrive));
+
+ /* File handle related information */
+ MemorySnapShot_Store(&ForcedHandles, sizeof(ForcedHandles));
+ if (bSave)
{
- GemDOS_ClearAllFileHandles();
+ MemorySnapShot_Store(&handles, sizeof(handles));
+
+ for (finfo = FileHandles, i = 0; i < handles; i++, finfo++)
+ save_file_handle_info(finfo);
+ }
+ else
+ {
+ int saved_handles;
+ MemorySnapShot_Store(&saved_handles, sizeof(saved_handles));
+ assert(saved_handles == handles);
+
+ for (finfo = FileHandles, i = 0; i < handles; i++, finfo++)
+ restore_file_handle_info(i, finfo);
+
+ /* DTA file name cache isn't valid anymore */
+ GemDOS_ClearAllInternalDTAs();
}
}
@@ -1867,6 +1973,7 @@
}
/* Tag handle table entry as used in this process and return handle */
FileHandles[Index].bUsed = true;
+ strcpy(FileHandles[Index].szMode, "wb+");
FileHandles[Index].Basepage = STMemory_ReadLong(act_pd);
snprintf(FileHandles[Index].szActualName,
sizeof(FileHandles[Index].szActualName),
@@ -1964,6 +2071,7 @@
strcpy(szActualFileName, pszFileName);
FileHandles[Index].FileHandle = OverrideHandle;
RealMode = "read-only";
+ ModeStr = "rb";
}
else
{
@@ -2014,6 +2122,7 @@
{
/* Tag handle table entry as used in this process and return handle */
FileHandles[Index].bUsed = true;
+ strcpy(FileHandles[Index].szMode, ModeStr);
FileHandles[Index].Basepage = STMemory_ReadLong(act_pd);
snprintf(FileHandles[Index].szActualName,
sizeof(FileHandles[Index].szActualName),
diff -r 202cf7c4f230 src/memorySnapShot.c
--- a/src/memorySnapShot.c Fri Dec 14 02:14:11 2018 +0200
+++ b/src/memorySnapShot.c Sat Dec 15 02:43:52 2018 +0200
@@ -58,7 +58,7 @@
#include "hatari-glue.h"
-#define VERSION_STRING "2.1.0" /* Version number of compatible memory snapshots - Always 6 bytes (inc' NULL) */
+#define VERSION_STRING "2.2.0" /* Version number of compatible memory snapshots - Always 6 bytes (inc' NULL) */
#define SNAPSHOT_MAGIC 0xDeadBeef
#if HAVE_LIBZ