| 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,
On 12/15/18 12:43 PM, Nicolas Pomarède wrote:
I tested this patch and it correctly saved/restored the opened
filehandler when streaming an MP2 file from disk, so this looks good to
go in main repo.
but I had some warning at compilation :
....
My system is linux 64 bit, maybe "offset" size differs on 32 bit system
if that's what you're using ?
More serious problem is when compiling for windows : the mtime part of
the save/restore does not compile :
....> I'm not sure this can be fixed easily if we want to maintain a common
code for linux/windows (and mayb osx too, I don't know if your patch
compiles OK under OSX).
Does the (attached) updated patch fix the warnings?
* modification test doesn't need nanosecond accuracy, so I can
use backwards compatible "st_mtime" seconds field
* GCC "PRId64" macro handles the %lld issue between 32-/64-bit
compiles, but I don't know whether it works with your Windows
build
Also, I don't think saving / checking mtime at restore is really needed.
If someone decides to modify the file on disk after it was part of a
memory snapshot it would still work if filesze doesn't change for example.
It would load, but whether it works correctly depends on whether
the parts of the file that program loads after restore were changed.
I considered checking file size, but modification time covers that
too:
-------------
The field st_mtime is changed by file modifications, for example, by
mknod(2), truncate(2), utime(2), and write(2) (of more than zero
bytes). Moreover, st_mtime of a directory is changed by the creation
or deletion of files in that directory. The st_mtime field is not
changed for changes in owner, group, hard link count, or mode.
-------------
I see your point to check this and warn the user, but I'm not sure this
part of the patch should be kept if it causes too many compilation
difference on various OSes.
- Eero
diff -r 202cf7c4f230 src/gemdos.c
--- a/src/gemdos.c Fri Dec 14 02:14:11 2018 +0200
+++ b/src/gemdos.c Sun Dec 16 01:30:35 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,96 @@
/*-----------------------------------------------------------------------*/
/**
+ * 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 stat fstat;
+ time_t mtime;
+ 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_mtime; /* modification time */
+ }
+ else
+ {
+ /* avoid warnings about access to unset memory */
+ offset = 0;
+ stat("/", &fstat);
+ mtime = fstat.st_mtime;
+ }
+ 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 stat fstat;
+ time_t mtime;
+ 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));
+ handle->FileHandle = NULL;
+
+ 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_mtime), &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 %"PRId64" 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 +943,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 +1974,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 +2072,7 @@
strcpy(szActualFileName, pszFileName);
FileHandles[Index].FileHandle = OverrideHandle;
RealMode = "read-only";
+ ModeStr = "rb";
}
else
{
@@ -2014,6 +2123,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 Sun Dec 16 01:30:35 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