Opened 10 years ago

Closed 10 years ago

#4999 closed defect (fixed)

[Patch] VRTDatasets will lookup files with RelativeToVRT set from the location of a symlink

Reported by: ryanl Owned by: Robert Coup
Priority: normal Milestone: 1.10.0
Component: GDAL_Raster Version: svn-trunk
Severity: normal Keywords: VRT
Cc: Even Rouault

Description

If a vrt uses RelativeToVRT the current VRTDataset will lookup that file from the location of the specified file. If the file is a symlink this will cause errors unless every file referenced by the vrt using a relative path is symlinked to the correct place. ie. https://gist.github.com/rcoup/4991063

The attached patch will check if the vrt file is a symlink and if it is the driver will lookup the symlink's target and use that file to do the lookups. When the actual file is found the driver will proceed as normal using the path to the actual file. The patch also includes tests for:

  • Symlink to vrt specified absolutely (ln -s /path/to/file.vrt symlink.vrt)
  • Symlink to vrt specified relatively (ln -s to/file.vrt symlink.vrt)
  • Symlink to a symlink to a vrt (Both specified relatively)

Attachments (2)

vrt_symlinks.patch (5.6 KB ) - added by ryanl 10 years ago.
Patch for r25659
vrt_symlinks_v2.patch (6.1 KB ) - added by ryanl 10 years ago.
updated patch for r25659

Download all attachments as: .zip

Change History (6)

by ryanl, 10 years ago

Attachment: vrt_symlinks.patch added

Patch for r25659

comment:1 by Robert Coup, 10 years ago

Cc: Even Rouault added; Robert Coup removed
Owner: changed from warmerdam to Robert Coup
Status: newassigned

comment:2 by Even Rouault, 10 years ago

A few remarks :

  • CPLGetCurrentDir() might return NULL on some (exotic) platforms, which would cause CPLProjectRelativeFilename() to assert in debug mode --> the CPLAssert() in CPLProjectRelativeFilename should likely be modified to accept pszProjectDir being NULL, since a test afterwards accepts it.
  • the dynamic allocation in VSIStatBuf *statBuffer = new VSIStatBuf() seems useless. VSIStatBuf statBuffer; should do it
  • lstat() return code should be tested
  • the branch with the new CPLError() will leak fp and pszXML
  • the new tests will likely emit errors on the console on Windows because you cannot unlink a file that is still opened. And I'm not sure how os.symlink() will work on Windows.

by ryanl, 10 years ago

Attachment: vrt_symlinks_v2.patch added

updated patch for r25659

comment:3 by ryanl, 10 years ago

  • I hard coded NULL instead of CPLGetCurrentDir() and was unable to cause an assert in debug mode, looking at the code this would only assert if the address was between a pointer to a buffer and that pointer + some length, so NULL values will pass through fine.
  • Dynamic allocation: Agreed, changed
  • lstat() return code: Agreed, now it is checked and an error is thrown unless the error is ENOENT, as this will be thrown if the file is a virtual file and checks later in the open process are currently handling it.
  • Memory leak: Completely missed that, fixed
  • Tests on windows: Following the lead from gcore/misc.py the symlink tests will be skipped if symlinks are unsupported, the misc tests also follow the same link, test, remove process that the new vrt tests do.

comment:4 by Even Rouault, 10 years ago

Milestone: 1.10.0
Resolution: fixed
Status: assignedclosed

r25682 configure.in configure port/cpl_config.h.in frmts/vrt/vrtdataset.cpp ../autotest/gcore/vrt_read.py ../autotest/utilities/test_gdallocationinfo.py -m "VRT: solve issue when the VRT is a symlink and that the sources filenames are RelativeToVRT (patch by ryanl, #4999)"

I've just fixed a memleak with the return of CPLGetCurrentDir() and modified test_gdallocationinfo.py to adapt to the new behaviour.

Note: See TracTickets for help on using tickets.