Errors in FormatCRLF function to idrisi driver.

[lucafibbi]

The function FormatCRLF (in the file IdrisiDataset.cpp) creates a temporary file with the extension $$$ and the name equal to the rdc file. When the variable pszTempfile is calculated (with the name of the temporay file) even the variable pszFilename (with the name of the original rdc file) undergoes a change, so the rdc file can't be open, the temporary file remain open when the function return and the line feed of the rdc file remain unchanged. Here, I show a possible solution of this bug:

original

void FormatCRLF( const char *pszFilename ) {

const char *pszTempfile; FILE *fpIn; FILE *fpOut; char ch;

char* pszFilenameDup = CPLStrdup( pszFilename ); pszTempfile = CPLResetExtension( pszFilenameDup, "$$$" ); CPLFree( pszFilenameDup );

fpIn = VSIFOpen( pszFilename, "r" ); fpOut = VSIFOpen( pszTempfile, "w" );

if ( fpIn == NULL ) {

return;

}

if ( fpOut == NULL ) {

VSIFClose( fpIn ); return;

}

Copy data

ch = VSIFGetc( fpIn );

while( VSIFEof( fpIn ) == FALSE ) {

VSIFPutc( ch, fpOut ); ch = VSIFGetc( fpIn );

}

VSIFClose( fpIn ); VSIFClose( fpOut );

Convert format

fpIn = VSIFOpen( pszTempfile, "r" ); fpOut = VSIFOpen( pszFilename, "w" );

if ( fpIn == NULL ) {

return;

}

if ( fpOut == NULL ) {

VSIFClose( fpIn ); return;

}

ch = VSIFGetc( fpIn );

while( VSIFEof( fpIn ) == FALSE ) {

if( ch == '\012' ) {

VSIFPutc( '\015', fpOut );

} VSIFPutc( ch, fpOut ); ch = VSIFGetc( fpIn );

}

VSIFClose( fpIn ); VSIFClose( fpOut );

VSIUnlink( pszTempfile );

}

new

void FormatCRLF( const char *pszFilename ) {

char *pszTempfile = NULL; FILE *fpIn = NULL; FILE *fpOut = NULL; char ch;

fpIn = VSIFOpen( pszFilename, "r" ); if ( fpIn == NULL ) {

return;

}

pszTempfile = (char *)CPLMalloc(strlen(pszFilename)+5); sprintf(pszTempfile,"%s.$$$",pszFilename);

fpOut = VSIFOpen( pszTempfile, "w" );

if ( fpOut == NULL ) {

CPLFree( pszTempfile );

VSIFClose( fpIn ); return;

}

Copy data

ch = VSIFGetc( fpIn );

while( VSIFEof( fpIn ) == FALSE ) {

VSIFPutc( ch, fpOut ); ch = VSIFGetc( fpIn );

}

VSIFClose( fpIn ); VSIFClose( fpOut );

Convert format

fpIn = VSIFOpen( pszTempfile, "r" ); if ( fpIn == NULL ) {

CPLFree( pszTempfile );

return;

} fpOut = VSIFOpen( pszFilename, "w" );

if ( fpOut == NULL ) {

CPLFree( pszTempfile );

VSIFClose( fpIn ); return;

}

ch = VSIFGetc( fpIn );

while( VSIFEof( fpIn ) == FALSE ) {

if( ch == '\012' ) {

VSIFPutc( '\015', fpOut );

} VSIFPutc( ch, fpOut ); ch = VSIFGetc( fpIn );

}

VSIFClose( fpIn ); VSIFClose( fpOut );

VSIUnlink( pszTempfile );

CPLFree( pszTempfile );

}

[ilucena]

The function "FormatCRLF()" was created to produce DOS text format when running on UNIX. That was in GDAL 1.3. Now the CSLSave() is producing UNIX format even when running on Windows.

http://trac.osgeo.org/gdal/browser/trunk/gdal/frmts/idrisi/IdrisiDataset.cpp#L405

Does anybody knows why?

But is independent from the problem you mentioned. I will take a look. Thanks for reporting that.

[ilucena]

There was a change on CSLSave() on port/cpl_string.cpp:

http://trac.osgeo.org/gdal/browser/branches/1.4/gdal/port/cpl_string.cpp#L294 http://trac.osgeo.org/gdal/browser/trunk/gdal/port/cpl_string.cpp#L395

Since that driver is going to need that function in any operational system, I copied CSLSave over FormatCRLF and renamed it to SaveAsCRLF. I tested in Windows and Linux and it look fine. gdal_autotest passed OK too.

The question now is: Is that a bug in CSLSave? I will explain.

When you run that sentence printf(fp,"line1\nline2"); in Windows, you would expect it to write two characters at the end of each line, 0x0d and 0x0a, Carriage Return and Line Feed. Right?

[Even Rouault]

It looks like it was an unintented effect of the switch of CSLSave() to using VSIF Large API. Before, we wrote the file by using the C library file API (fopen/fputc/fprintf/fwrite etc) that on Windows takes care about translation "\n" to "\r\n" when the file is opened in text mode ("wt"). But now on Windows, the API used by VSIF Large API are CreateFile()/WriteFile() and the 't' indicator in "wt" mode is just ignored, so the content is written as binary and the translation of "\n" to "\r\n" is not done anymore.

The documentation of CSLSave() is not explicit about its behaviour regarding to line terminator, so I don't know if it's a big issue or not. I don't think the previous behaviour was really that great as the same code did not generate the same result whether is was run under Unix or WIN32 (hence the need for the quite ugly line terminator convertor that you had to write previously in Idrisi code).

--> You would rather expect 2 different functions - CSLSaveUnix() and CSLSaveWindows() (or one with some option flag) - that should be called according to the specifications of the format you want to write and that would behave exactly the same way from any OS. Using "wt" should be in fact banned in order to get the same behaviour on all OS.

A grep in GDAL source tree shows the following callers for CSLSave():

frmts/raw/hkvdataset.cpp:        CSLSave( papszGeoref, pszFilename );
frmts/raw/pauxdataset.cpp:        CSLSave( papszAuxLines, pszAuxFilename );
ogr/ogrfeaturestyle.cpp:    if (CSLSave(m_papszStyleTable,pszFilename) == 0)
ogr/ogrsf_frmts/avc/avc_binwr.c:    CSLSave(papszPrj, psFile->pszFilename);

And there are far more places where people write a text file "at hand" :

frmts/aaigrid/aaigriddataset.cpp:    fpImage = VSIFOpenL( pszFilename, "wt" );
frmts/aaigrid/aaigriddataset.cpp:        fp = VSIFOpenL( pszPrjFilename, "wt" );
frmts/bsb/bsb2raw.c:    fp = VSIFOpen( CPLResetExtension( papszArgv[2], "aux"), "wt" );
frmts/raw/btdataset.cpp:    fp = VSIFOpenL( pszPrjFile, "wt" );
frmts/raw/ehdrdataset.cpp:        FILE *fp = VSIFOpenL( osCLRFilename, "wt" );
frmts/raw/ehdrdataset.cpp:    fp = VSIFOpenL( osPrjFilename.c_str(), "wt" );
frmts/raw/ehdrdataset.cpp:    fp = VSIFOpenL( osHDRFilename, "wt" );
frmts/raw/ehdrdataset.cpp:    fp = VSIFOpenL( osSTXFilename, "wt" );
frmts/raw/ehdrdataset.cpp:    fp = VSIFOpenL( pszHdrFilename, "wt" );
frmts/raw/envidataset.cpp:    fp = VSIFOpen( pszHDRFilename, "wt" );
frmts/raw/hkvdataset.cpp:    fp = VSIFOpen( pszFilename, "wt" );
frmts/raw/mffdataset.cpp:    fp = VSIFOpen( pszFilename, "wt" );
frmts/raw/pauxdataset.cpp:    fp = VSIFOpenL( pszAuxFilename, "wt" );
frmts/xpm/xpmdataset.cpp:    fpPBM = VSIFOpen( pszFilename, "wt+" );
ogr/ogrsf_frmts/avc/avc_binwr.c:        fpOut = VSIFOpen(pszFname, "wt");
ogr/ogrsf_frmts/fme/ogrfmedatasource.cpp:    fp = VSIFOpen( CPLResetExtension( pszFilename, "fdd" ), "wt" );
ogr/ogrsf_frmts/gml/ogrgmldatasource.cpp:            && (fp = VSIFOpen( pszGFSFilename, "wt" )) != NULL )
ogr/ogrsf_frmts/gml/ogrgmldatasource.cpp:        fpOutput = VSIFOpen( pszFilename, "wt+" );
ogr/ogrsf_frmts/gml/ogrgmldatasource.cpp:        fpSchema = VSIFOpen( pszXSDFilename, "wt" );
ogr/ogrsf_frmts/kml/ogrkmldatasource.cpp:        fpOutput_ = VSIFOpen( pszName, "wt+" );
ogr/ogrsf_frmts/mitab/mitab_tabfile.cpp:    if ( (fp = VSIFOpen(m_pszFname, "wt")) != NULL)
ogr/ogrsf_frmts/mitab/mitab_tabview.cpp:    if ( (fp = VSIFOpen(m_pszFname, "wt")) != NULL)
ogr/ogrsf_frmts/oci/ogrociloaderlayer.cpp:    fpLoader = VSIFOpen( pszLoaderFilename, "wt" );
ogr/ogrsf_frmts/shape/ogrshapedatasource.cpp:            && (fp = VSIFOpen( osPrjFile, "wt" )) != NULL )
gcore/gdal_misc.cpp:    fpTFW = VSIFOpenL( pszTFW, "wt" );

[ilucena]

I traced the calls that are leading to Windows API CreateFile ​http://msdn.microsoft.com/en-us/library/aa363858(VS.85).aspx and I understand that it would requires a portability decision ("wt" should be in fact banned; create two version of CSLSave; just document it; global solution for other functions; etc.). I would point out that CSLSave takes a stringlist that could be in the form of name=value entries and writing it "by hand" would means making a copy of the CSLSave code to the driver. Exactly what I did. Thanks for your suggestion.