From 260e49e7c8eabde6ddd9c078fb17d72b6545ef3e Mon Sep 17 00:00:00 2001 From: Teemu Murtola Date: Thu, 9 Jul 2015 19:52:54 +0300 Subject: [PATCH 1/1] More gmxfio simplification - Remove gmx_fio_setdebug() and all implementation related to it; getting the source code location printed to stderr is not really useful for errors that typically originate from the file system. - Move gmx_fio_setprecision() to gmxfio-xdr, since it logically belongs there. - Remove bOpen, as the only case where any code could observe bOpen==FALSE requires a use-after-free, and possibly also an ugly race condition on top of that. - Reduce use of iFTP member within gmxfio, where the purpose is to check whether the fio->xdr member is not NULL. - Remove checks from gmx_fio_open() that overlapped with those in gmx_ffopen(). There are some corner cases that now work differently: gmxfio no longer bypasses the check for .Z/.gz files for xdr files, but lets things through to gmx_ffopen(), and with GMX_FAHCORE there is no longer any kind of existence check for xdr files (it is questionable whether the previous behavior was intentionally different for xdr and non-xdr files, anyways). - Remove dead code: fio->fp is always non-NULL if fio->xdr is. Change-Id: Icd1588bfbb510543a55f57fb86e0e1e2a1fce236 --- src/contrib/anaf.c | 1 - src/gromacs/fileio/gmxfio-impl.h | 4 +- src/gromacs/fileio/gmxfio-xdr.cpp | 12 +++-- src/gromacs/fileio/gmxfio-xdr.h | 3 ++ src/gromacs/fileio/gmxfio.cpp | 77 +++---------------------------- src/gromacs/fileio/gmxfio.h | 6 --- src/gromacs/fileio/tpxio.c | 2 - 7 files changed, 17 insertions(+), 88 deletions(-) diff --git a/src/contrib/anaf.c b/src/contrib/anaf.c index 89971946ac..044d987048 100644 --- a/src/contrib/anaf.c +++ b/src/contrib/anaf.c @@ -70,7 +70,6 @@ static void list_trn(char *fn) printf("Going to open %s\n",fn); fpread = open_trn(fn,"r"); fpwrite = open_tpx(NULL,"w"); - gmx_fio_setdebug(fpwrite,TRUE); mmm=mass[0]+2*mass[1]; for(i=0; (i<5); i++) diff --git a/src/gromacs/fileio/gmxfio-impl.h b/src/gromacs/fileio/gmxfio-impl.h index f42facdc52..0b455979dd 100644 --- a/src/gromacs/fileio/gmxfio-impl.h +++ b/src/gromacs/fileio/gmxfio-impl.h @@ -61,10 +61,8 @@ struct t_fileio { FILE *fp; /* the file pointer */ - gmx_bool bOpen, /* the file is open */ - bRead, /* the file is open for reading */ + gmx_bool bRead, /* the file is open for reading */ bDouble, /* write doubles instead of floats */ - bDebug, /* the file ops should come with debug info */ bReadWrite; /* the file is open for reading and writing */ char *fn; /* the file name */ XDR *xdr; /* the xdr data pointer */ diff --git a/src/gromacs/fileio/gmxfio-xdr.cpp b/src/gromacs/fileio/gmxfio-xdr.cpp index 2c6f98fef3..b2876d0566 100644 --- a/src/gromacs/fileio/gmxfio-xdr.cpp +++ b/src/gromacs/fileio/gmxfio-xdr.cpp @@ -62,6 +62,13 @@ static const char *eioNames[eioNR] = "IVEC", "STRING" }; +void gmx_fio_setprecision(t_fileio *fio, gmx_bool bDouble) +{ + gmx_fio_lock(fio); + fio->bDouble = bDouble; + gmx_fio_unlock(fio); +} + XDR *gmx_fio_getxdr(t_fileio *fio) { XDR *ret = NULL; @@ -336,11 +343,6 @@ static gmx_bool do_xdr(t_fileio *fio, void *item, int nitem, int eio, default: gmx_fio_fe(fio, eio, desc, srcfile, line); } - if ((res == 0) && (fio->bDebug)) - { - fprintf(stderr, "Error in xdr I/O %s %s to file %s (source %s, line %d)\n", - eioNames[eio], desc, fio->fn, srcfile, line); - } return (res != 0); } diff --git a/src/gromacs/fileio/gmxfio-xdr.h b/src/gromacs/fileio/gmxfio-xdr.h index 17c3fbc60b..982724d64f 100644 --- a/src/gromacs/fileio/gmxfio-xdr.h +++ b/src/gromacs/fileio/gmxfio-xdr.h @@ -48,6 +48,9 @@ extern "C" { struct t_fileio; +void gmx_fio_setprecision(struct t_fileio *fio, gmx_bool bDouble); +/* Select the floating point precision for reading and writing files */ + XDR *gmx_fio_getxdr(struct t_fileio *fio); /* Return the file pointer itself */ diff --git a/src/gromacs/fileio/gmxfio.cpp b/src/gromacs/fileio/gmxfio.cpp index 67ab811570..a094435c73 100644 --- a/src/gromacs/fileio/gmxfio.cpp +++ b/src/gromacs/fileio/gmxfio.cpp @@ -95,10 +95,6 @@ static int gmx_fio_int_flush(t_fileio* fio) { rc = fflush(fio->fp); } - else if (fio->xdr) - { - rc = fflush((FILE *) fio->xdr->x_private); - } return rc; } @@ -309,11 +305,7 @@ t_fileio *gmx_fio_open(const char *fn, const char *mode) /* Check if it should be opened as a binary file */ if (!ftp_is_text(fn2ftp(fn))) { - /* Not ascii, add b to file mode */ - if ((strchr(newmode, 'b') == NULL) && (strchr(newmode, 'B') == NULL)) - { - strcat(newmode, "b"); - } + strcat(newmode, "b"); } snew(fio, 1); @@ -331,30 +323,10 @@ t_fileio *gmx_fio_open(const char *fn, const char *mode) fio->iFTP = fn2ftp(fn); fio->fn = gmx_strdup(fn); + fio->fp = gmx_ffopen(fn, newmode); /* If this file type is in the list of XDR files, open it like that */ if (ftp_is_xdr(fio->iFTP)) { - /* First check whether we have to make a backup, - * only for writing, not for read or append. - */ - if (newmode[0] == 'w') - { -#ifndef GMX_FAHCORE - /* only make backups for normal gromacs */ - make_backup(fn); -#endif - } - else - { - /* Check whether file exists */ - if (!gmx_fexist(fn)) - { - gmx_open(fn); - } - } - /* Open the file */ - fio->fp = gmx_ffopen(fn, newmode); - /* determine the XDR direction */ if (newmode[0] == 'w' || newmode[0] == 'a') { @@ -364,15 +336,9 @@ t_fileio *gmx_fio_open(const char *fn, const char *mode) { fio->xdrmode = XDR_DECODE; } - snew(fio->xdr, 1); xdrstdio_create(fio->xdr, fio->fp, fio->xdrmode); } - else - { - /* If it is not, open it as a regular file */ - fio->fp = gmx_ffopen(fn, newmode); - } /* for appending seek to end of file to make sure ftell gives correct position * important for checkpointing */ @@ -384,8 +350,6 @@ t_fileio *gmx_fio_open(const char *fn, const char *mode) fio->bRead = bRead; fio->bReadWrite = bReadWrite; fio->bDouble = (sizeof(real) == sizeof(double)); - fio->bDebug = FALSE; - fio->bOpen = TRUE; /* and now insert this file into the list of open files. */ gmx_fio_insert(fio); @@ -396,12 +360,7 @@ static int gmx_fio_close_locked(t_fileio *fio) { int rc = 0; - if (!fio->bOpen) - { - gmx_fatal(FARGS, "File %s closed twice!\n", fio->fn); - } - - if (ftp_is_xdr(fio->iFTP)) + if (fio->xdr != NULL) { xdr_destroy(fio->xdr); sfree(fio->xdr); @@ -412,7 +371,6 @@ static int gmx_fio_close_locked(t_fileio *fio) rc = gmx_ffclose(fio->fp); /* fclose returns 0 if happy */ } - fio->bOpen = FALSE; return rc; } @@ -425,10 +383,6 @@ int gmx_fio_close(t_fileio *fio) /* We don't want two processes operating on the list at the same time */ tMPI_Thread_mutex_lock(&open_file_mutex); - if (fio->iFTP == efTNG) - { - gmx_incons("gmx_fio_close should not be called on a TNG file"); - } gmx_fio_lock(fio); /* first remove it from the list */ gmx_fio_remove(fio); @@ -448,7 +402,7 @@ int gmx_fio_fp_close(t_fileio *fio) { int rc = 0; gmx_fio_lock(fio); - if (!ftp_is_xdr(fio->iFTP)) + if (fio->xdr == NULL) { rc = gmx_ffclose(fio->fp); /* fclose returns 0 if happy */ fio->fp = NULL; @@ -643,7 +597,7 @@ int gmx_fio_get_output_file_positions(gmx_file_position_t **p_outputfiles, { /* Skip the checkpoint files themselves, since they could be open when we call this routine... */ - if (cur->bOpen && !cur->bRead && cur->iFTP != efCPT) + if (!cur->bRead && cur->iFTP != efCPT) { /* This is an output file currently open for writing, add it */ if (nfiles == nalloc) @@ -674,20 +628,6 @@ int gmx_fio_get_output_file_positions(gmx_file_position_t **p_outputfiles, } -void gmx_fio_setprecision(t_fileio *fio, gmx_bool bDouble) -{ - gmx_fio_lock(fio); - fio->bDouble = bDouble; - gmx_fio_unlock(fio); -} - -void gmx_fio_setdebug(t_fileio *fio, gmx_bool bDebug) -{ - gmx_fio_lock(fio); - fio->bDebug = bDebug; - gmx_fio_unlock(fio); -} - char *gmx_fio_getname(t_fileio *fio) { char *ret; @@ -748,11 +688,6 @@ static int gmx_fio_int_fsync(t_fileio *fio) { rc = gmx_fsync(fio->fp); } - else if (fio->xdr) /* this should normally not happen */ - { - rc = gmx_fsync((FILE*) fio->xdr->x_private); - /* ^ is this actually OK? */ - } return rc; } @@ -779,7 +714,7 @@ t_fileio *gmx_fio_all_output_fsync(void) cur = gmx_fio_get_first(); while (cur) { - if (cur->bOpen && !cur->bRead) + if (!cur->bRead) { /* if any of them fails, return failure code */ int rc = gmx_fio_int_fsync(cur); diff --git a/src/gromacs/fileio/gmxfio.h b/src/gromacs/fileio/gmxfio.h index dabf39c7eb..d08249e4a1 100644 --- a/src/gromacs/fileio/gmxfio.h +++ b/src/gromacs/fileio/gmxfio.h @@ -97,9 +97,6 @@ int gmx_fio_fclose(FILE *fp); * Change properties of the open file ********************************************************/ -void gmx_fio_setprecision(t_fileio *fio, gmx_bool bDouble); -/* Select the floating point precision for reading and writing files */ - char *gmx_fio_getname(t_fileio *fio); /* Return the filename corresponding to the fio index */ @@ -109,9 +106,6 @@ int gmx_fio_getftp(t_fileio *fio); was opened as a specific file type and changing that midway is most likely an evil hack. */ -void gmx_fio_setdebug(t_fileio *fio, gmx_bool bDebug); -/* Set the debug mode */ - gmx_bool gmx_fio_getread(t_fileio *fio); /* Return whether read mode is on in fio */ diff --git a/src/gromacs/fileio/tpxio.c b/src/gromacs/fileio/tpxio.c index 63f76d6f1c..509aa4a15d 100644 --- a/src/gromacs/fileio/tpxio.c +++ b/src/gromacs/fileio/tpxio.c @@ -3222,8 +3222,6 @@ static void do_tpxheader(t_fileio *fio, gmx_bool bRead, t_tpxheader *tpx, int idum = 0; real rdum = 0; - gmx_fio_setdebug(fio, bDebugMode()); - /* XDR binary topology file */ precision = sizeof(real); if (bRead) -- 2.22.0