Created VSIError to report VSI errors. #98

Closed
wants to merge 17 commits into
from

Projects

None yet

3 participants

@lossyrob

Implements virtual file system error reporting, and sets errors on unix file IO errors and certain AWS S3 errors.

More documentation to follow.

  • Implement error reporting
  • Testing
  • Test against windows to check against correct file system error string.
@lossyrob

/cc @sgillies @rouault @perrygeo

This is the initial set of work, I couldn't get all the testing done before having to say Au Revoir at the OSGeo code sprint.

My editor strips endline whitespace, sorry for the diff noise; if you need that to not happen just let me know and I can fix it.

@rouault

cpl_vsi_error.obj will have to be added in port/makefile.vc too

@rouault

For robustness the pszMessage == NULL case should be handled. Possibly with if (pszMessage == NULL ) pszMessage = pszErrorMsg

@rouault

The deleted lines 268-289 dealing with CPL_ACCUM_ERROR_MSG should be restored

@rouault

This header file can be included from C files, where bool is invalid. int should be rather used here.

@rouault

See above: bool used in header possibly included from C files

@rouault

VSIErrorV could possibly be kept "static private" to cpl_vsi_error.cpp file

@lossyrob

Thanks for the notes Even. I'll make these changes when I can get back to this, and also figure out what's going with the autotests (I tried running on a clean checkout and getting the same segfault). I think my approach will be to try and have some docker images that can build and test, for OSX hacks like me. It'll be a couple of weeks because I'm under the gun for a GeoTrellis release.

Does GDAL have a release schedule, so I know when I'll have to get this in by?

@rouault
Member
rouault commented Feb 29, 2016

Does GDAL have a release schedule, so I know when I'll have to get this in by?
For now, the feature freeze is planned end of March, but there's also a chance/risk that it is advanced on March 11th to please a potential "sponsor" (should know very soon hopefully)

@sgillies

@rouault Which feature freeze is planned for the end of March? Are you talking about a 2.1 freeze? It's worth my time to get @lossyrob's work into 2.1.

@rouault
Member
rouault commented Feb 29, 2016

Are you talking about a 2.1 freeze?

Yes

@lossyrob

End of March is doable for me, but if it gets pushed up to March 11 it'll be a tight fit. @sgillies besides the code comments I just need to test, so if your willing to throw time at it it would be a matter of getting the tests running and passing.

@rouault
Member
rouault commented Feb 29, 2016

End of March is doable for me, but if it gets pushed up to March 11 it'll be a tight fit.

Update: forget my mention of March 11. End of March for feature freeze is still the current target.

@sgillies

Thanks, @rouault!

@lossyrob

@rouault My latest commits address your code review comments, thanks. I'm going to be working on getting the tests running and finish up any additional changes today.

@rouault rouault and 1 other commented on an outdated diff Mar 20, 2016
autotest/gcore/basic_test.py
@@ -40,11 +40,13 @@
# Nothing exciting here. Just trying to open non existing files,
# or empty names, or files that are not valid datasets...
+non_existing_error_msg = 'No such file or directory'
@rouault
rouault Mar 20, 2016 Member

I'm afraid this will not pass on Windows

@lossyrob
lossyrob Mar 20, 2016

True. I got the impression that the autotest was only running on Linux (it still fails my mac in some ways, even trunk, whereas Travis passes). Are the autotests targeted to work across all platforms? If so, do you have suggestions on how to test on Windows?

@rouault
rouault Mar 20, 2016 Member

The autotest also run on Windows and Mac.
For Windows, there's an AppVeyor config in this branch : https://github.com/rouault/gdal_coverage/blob/trunk_vc12_full/appveyor.yml . It could/should probably be merged into main repo. For now, master is merged regularly in this branch.
For Mac, this is https://github.com/rouault/gdal_coverage/blob/trunk_travis_macosx/.travis.yml . Could probably be unified into the main travis.yml with some matrix logic.
With the current setup, I guess you could probably fork that repo and merge your branch into the 2 above branches, and create PR to trigger the builds ?

@lossyrob
lossyrob Mar 20, 2016

Ok, I'll give that a shot once I get the travis build to pass in this repo.

@lossyrob
lossyrob Mar 20, 2016

Actually, the way you have the branches set up, I don't believe that will work. I'll try to find another way to test on Windows.

@rouault rouault and 1 other commented on an outdated diff Mar 20, 2016
gdal/port/cpl_vsi_error.cpp
@@ -61,6 +61,7 @@ typedef struct {
/* Do not add anything here. szLastErrMsg must be the last field. See CPLRealloc() below */
} VSIErrorContext;
+static void CPL_DLL VSIErrorV(VSIErrorNum, const char *, va_list );
@rouault
rouault Mar 20, 2016 Member

CPL_DLL doesn't make sense for a static function

@lossyrob
lossyrob Mar 20, 2016

Thanks, I'm addressing these in a commit; I rebased the travis change so this commit got lost, but I will address the two comments you made on cpl_vsi_error.cpp

@rouault rouault commented on an outdated diff Mar 20, 2016
gdal/port/cpl_vsi_error.cpp
@@ -256,14 +257,14 @@ const char* CPL_STDCALL VSIGetLastErrorMsg()
* been cleared by VSIErrorReset(). The returned pointer is to an internal
* string that should not be altered or freed.
*
- * @return true if a CPLError was issued, or false if not.
+ * @return 1 if a CPLError was issued, or 0 if not.
@rouault
rouault Mar 20, 2016 Member

TRUE and FALSE (which are integers, and not C++ bool) would be clearer

@lossyrob

@rouault tests are passing on Travis, and I set up a window build that passed here: https://ci.appveyor.com/project/lossyrob/gdal-coverage/build/job/hr5k8fm2n36j4sya. I believe this PR is ready for merge. Thanks!

@lossyrob lossyrob changed the title from [Work in progress] Created VSIError to report VSI errors. to Created VSIError to report VSI errors. Mar 21, 2016
@rouault rouault added a commit to rouault/gdal that referenced this pull request Mar 21, 2016
@rouault rouault Add VSIError mechanism to store errors related to filesystem calls, a…
…nd use it for /vsis3/. Add new CPLE_ error numbers. (patch by Rob Emanuele, OSGeo/gdal#98)

git-svn-id: https://svn.osgeo.org/gdal/trunk@33758 f0d54148-0727-0410-94bb-9a71ac55c965
5f293d1
@rouault
Member
rouault commented Mar 21, 2016

Committed in r33758

@kwrobot kwrobot pushed a commit to aashish24/gdal-svn that referenced this pull request Mar 21, 2016
rouault Add VSIError mechanism to store errors related to filesystem calls, a…
…nd use it for /vsis3/. Add new CPLE_ error numbers. (patch by Rob Emanuele, OSGeo/gdal#98)

git-svn-id: https://svn.osgeo.org/gdal/trunk/gdal@33758 f0d54148-0727-0410-94bb-9a71ac55c965
78c798c
@rouault
Member
rouault commented Mar 21, 2016

Everything looks good with the buildbots. Closing

@rouault rouault closed this Mar 21, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment