SHPRestoreSHX(): add extra sanity checks

[rouault]

No description provided.

[rouault]

CC @agiudiceandrea

[agiudiceandrea]

Thank you very much for this improvement! Is there a way to catch the SHPRestoreSHX() failure and display the emitted errors when using the "Repair shapefile" processing algorithm in QGIS https://github.com/qgis/QGIS/blob/master/src/analysis/processing/qgsalgorithmrepairshapefile.cpp#L81C6-L90?

[rouault]

Is there a way to catch the SHPRestoreSHX() failure

Perhaps calling CPLGetLastErrorType() + CPLGetLastErrorMsg(), but I haven't verified if there are not other subsequent errors that are emitted. Otherwise installing a custom error handler with CPLPushErrorHandlerEx() / CPLPopErrorHandler() is a way of catching all emitted errors, not just the last one

[agiudiceandrea]

I see another little issue here

gdal/ogr/ogrsf_frmts/shape/shpopen.c

Lines 685 to 698 in c92b22d

	SHPHandle SHPAPI_CALL SHPOpenLLEx(const char *pszLayer, const char *pszAccess,
	SAHooks *psHooks, int bRestoreSHX)
	{
	if (!bRestoreSHX)
	return SHPOpenLL(pszLayer, pszAccess, psHooks);
	else
	{
	if (SHPRestoreSHX(pszLayer, pszAccess, psHooks))
	{
	return SHPOpenLL(pszLayer, pszAccess, psHooks);
	}
	}
	return SHPLIB_NULLPTR;

If SHAPE_RESTORE_SHX=YES, then SHPRestoreSHX is executed. If SHPRestoreSHX fails, then SHPOpenLLEx always returns SHPLIB_NULLPTR even if the layer, after the SHX restoration, could be read by SHPOpenLL.
This could lead to a bit misleading behaviour (using gdal-dev at c92b22d):

>set SHAPE_RESTORE_SHX=YES
>ogrinfo corrupted.shp
ERROR 4: Error parsing .shp to restore .shx
ERROR 4: Error parsing .shp to restore .shx
ogrinfo failed - unable to open 'corrupted.shp'.
>set SHAPE_RESTORE_SHX=
>ogrinfo corrupted.shp
INFO: Open of `corrupted.shp'
      using driver `ESRI Shapefile' successful.
1: corrupted (Line String)

(Why I see two times the same error?)

I would also suggest add here

gdal/ogr/ogrsf_frmts/shape/shpopen.c

Lines 612 to 613 in c92b22d

	"Failed to read all values for %d records in .shx file: %s.",
	psSHP->nRecords, strerror(errno));

a similar advice as the one at

gdal/ogr/ogrsf_frmts/shape/shpopen.c

Lines 417 to 418 in c92b22d

	"Set SHAPE_RESTORE_SHX config option to YES to restore or "
	"create it.",

[rouault]

If SHPRestoreSHX fails, then SHPOpenLLEx always returns SHPLIB_NULLPTR

I see your point, but that seems appropriate to me, as a way to indicate something went wrong in the restoration.

(Why I see two times the same error?)

probably because it first tries in update mode, and then in read-only mode

I would also suggest add here

I'm a bit ambivalent about that. Suggesting SHAPE_RESTORE_SHX when there is no .shx is totally reasonable, but when the .shx exists and there is a disagreement with the .shp, this might be a corruption in the .shp itself. If you wanted to add a hint about SHAPE_RESTORE_SHX, words of caution should be added that it will override the existing .shx, which may prevent further forensic attempts, and people should probably backup the .shx before.