New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

System error code handling and utiltity function cleanup #238

Merged
merged 5 commits into from Oct 11, 2017

Conversation

Projects
None yet
5 participants
@saschazelzer
Member

saschazelzer commented Sep 3, 2017

This PR addresses issues #191 and #220 and also contains further cleanup.

The first commit contains the actual fixes:

  • Correctly dinstinguish between C runtime library errors and Win32 API errors
  • Set errno to 0 before calling C runtime library functions
  • Use thread-safe strerrror variants if available

It also improves code sharing between the legacy and the new GTest test executable:

  • Pull out TestUtils.cpp for re-use in legacy and GTest tests
  • TestUtils.cpp changes:
    • Use temporary directory instead of "ProgramData" path on Windows
    • Added cross-platform mkdtemp and mkstemp functions for creating unique files and directories (originated from public domain and contributed by me with slight changes to another project a couple of years ago; now just copied here)
    • Added helper functions for creating temporary files and directories

The commit also adds unit tests for our internal filesystem related functions:

  • Create tests for cppmicroservices::fs functions. The tests are compiled for static builds only to avoid exporting internal functions.
  • Make the gtest-based test executable a proper bundle, in order to compile TestUtils.cpp.

For the above changes, I had to duplicate a little more code from the internal file-system code. Seeing that we then had almost the same content duplicated in the framework and the unit test code I wanted to get rid of that and make it reusable.

This is what the second commit contains:

  • Create an object library called "util" that contains framework independent utility code chared between bundles and tests. This includes file-system related functions, error handling routines, string manipulation routines.
  • Add the object library to all bundles and the two test drivers by default

Please note that virtually no new code was written, except for disentangling our C / Win32 error handling, using strerror_r etc. where appropriate, and writing new unit tests (GTest based).

@codecov-io

This comment has been minimized.

Show comment
Hide comment
@codecov-io

codecov-io Sep 3, 2017

Codecov Report

Merging #238 into development will increase coverage by 5.15%.
The diff coverage is 78.92%.

Impacted file tree graph

@@               Coverage Diff               @@
##           development     #238      +/-   ##
===============================================
+ Coverage        77.58%   82.73%   +5.15%     
===============================================
  Files              117      188      +71     
  Lines             8011    13298    +5287     
===============================================
+ Hits              6215    11002    +4787     
- Misses            1796     2296     +500
Impacted Files Coverage Δ
framework/test/driver/ZipFile.h 94.11% <ø> (ø)
framework/src/service/ServiceObjects.cpp 67.7% <0%> (ø) ⬆️
framework/src/bundle/BundleThread.cpp 75.16% <0%> (+1.32%) ⬆️
framework/src/service/ServiceListeners.cpp 98.63% <100%> (ø) ⬆️
framework/test/bundles/libC1/TestBundleC1.cpp 88.29% <100%> (ø)
framework/src/bundle/BundleResourceContainer.cpp 95.41% <100%> (ø) ⬆️
...work/test/driver/BundleRegistryPerformanceTest.cpp 93.15% <100%> (ø)
framework/test/driver/ResourceCompilerTest.cpp 97.68% <100%> (ø)
framework/src/bundle/BundleUtils.cpp 100% <100%> (+13.04%) ⬆️
framework/test/driver/FrameworkTest.cpp 95.62% <100%> (ø)
... and 101 more

codecov-io commented Sep 3, 2017

Codecov Report

Merging #238 into development will increase coverage by 5.15%.
The diff coverage is 78.92%.

Impacted file tree graph

@@               Coverage Diff               @@
##           development     #238      +/-   ##
===============================================
+ Coverage        77.58%   82.73%   +5.15%     
===============================================
  Files              117      188      +71     
  Lines             8011    13298    +5287     
===============================================
+ Hits              6215    11002    +4787     
- Misses            1796     2296     +500
Impacted Files Coverage Δ
framework/test/driver/ZipFile.h 94.11% <ø> (ø)
framework/src/service/ServiceObjects.cpp 67.7% <0%> (ø) ⬆️
framework/src/bundle/BundleThread.cpp 75.16% <0%> (+1.32%) ⬆️
framework/src/service/ServiceListeners.cpp 98.63% <100%> (ø) ⬆️
framework/test/bundles/libC1/TestBundleC1.cpp 88.29% <100%> (ø)
framework/src/bundle/BundleResourceContainer.cpp 95.41% <100%> (ø) ⬆️
...work/test/driver/BundleRegistryPerformanceTest.cpp 93.15% <100%> (ø)
framework/test/driver/ResourceCompilerTest.cpp 97.68% <100%> (ø)
framework/src/bundle/BundleUtils.cpp 100% <100%> (+13.04%) ⬆️
framework/test/driver/FrameworkTest.cpp 95.62% <100%> (ø)
... and 101 more

@saschazelzer saschazelzer requested a review from CppMicroServices/developers Sep 3, 2017

@saschazelzer saschazelzer self-assigned this Sep 3, 2017

Show outdated Hide outdated cmake/usCTestScript_travis.cmake Outdated
Show outdated Hide outdated framework/src/bundle/CoreBundleContext.cpp Outdated
Show outdated Hide outdated util/include/cppmicroservices/util/FileSystem.h Outdated
#else
#ifndef WIN32_LEAN_AND_MEAN
#define WIN32_LEAN_AND_MEAN

This comment has been minimized.

@jeffdiclemente

jeffdiclemente Sep 28, 2017

Contributor

this #ifndef/#define WIN32_LEAN_AND_MEAN is in multiple places. it would be good to put it in a central location. perhaps in GlobalConfig.h?

@jeffdiclemente

jeffdiclemente Sep 28, 2017

Contributor

this #ifndef/#define WIN32_LEAN_AND_MEAN is in multiple places. it would be good to put it in a central location. perhaps in GlobalConfig.h?

This comment has been minimized.

@saschazelzer

saschazelzer Oct 4, 2017

Member

I think we need #126 resolved and have an internal global .h file (which is not included in public .h files) where we can place such defines.

@saschazelzer

saschazelzer Oct 4, 2017

Member

I think we need #126 resolved and have an internal global .h file (which is not included in public .h files) where we can place such defines.

This comment has been minimized.

@jeffdiclemente

jeffdiclemente Oct 4, 2017

Contributor

that sounds good to me.

@jeffdiclemente

jeffdiclemente Oct 4, 2017

Contributor

that sounds good to me.

Show outdated Hide outdated util/src/Error.cpp Outdated
Show outdated Hide outdated util/src/Error.cpp Outdated
Show outdated Hide outdated util/src/Error.cpp Outdated
return std::string();
}
static const std::string s_CurrWorkingDir = InitCurrentWorkingDirectory();

This comment has been minimized.

@karthikreddy09

karthikreddy09 Oct 3, 2017

Contributor

Why not make this magic static inside the function?

@karthikreddy09

karthikreddy09 Oct 3, 2017

Contributor

Why not make this magic static inside the function?

This comment has been minimized.

@saschazelzer

saschazelzer Oct 4, 2017

Member

This code was just moved from a different file and not changed. We had a discussion about the actual code in the original PR. Basically, we need to call InitCurrentWorkingDirectory at a time when there is only the main thread running (during static init).

@saschazelzer

saschazelzer Oct 4, 2017

Member

This code was just moved from a different file and not changed. We had a discussion about the actual code in the original PR. Basically, we need to call InitCurrentWorkingDirectory at a time when there is only the main thread running (during static init).

This comment has been minimized.

@jeffdiclemente

jeffdiclemente Oct 4, 2017

Contributor

@karthikreddy09 take a look at PR #216 for more information.

@jeffdiclemente

jeffdiclemente Oct 4, 2017

Contributor

@karthikreddy09 take a look at PR #216 for more information.

This comment has been minimized.

@karthikreddy09

karthikreddy09 Oct 4, 2017

Contributor

Ah ... now I understand. Thanks for pointing to the previous discussion.

@karthikreddy09

karthikreddy09 Oct 4, 2017

Contributor

Ah ... now I understand. Thanks for pointing to the previous discussion.

Show outdated Hide outdated framework/src/bundle/CoreBundleContext.cpp Outdated
}
// A cross-platform version of the POSIX mkdtemp function
char* mkdtemps_compat(char *tmpl, int suffixlen)

This comment has been minimized.

@ksubramz

ksubramz Oct 9, 2017

Contributor

It looks like there's a lot of common code between mkdtemps_compat and mkstemps_compat that can be factored out to remove the duplication?

@ksubramz

ksubramz Oct 9, 2017

Contributor

It looks like there's a lot of common code between mkdtemps_compat and mkstemps_compat that can be factored out to remove the duplication?

This comment has been minimized.

@saschazelzer

saschazelzer Oct 10, 2017

Member

Mostly for the same reason as above. Laziness, working on bugs or features, afraid of introducing regressions, ...

@saschazelzer

saschazelzer Oct 10, 2017

Member

Mostly for the same reason as above. Laziness, working on bugs or features, afraid of introducing regressions, ...

static const char validLetters[] = "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789";
// A cross-platform version of the mkstemps function

This comment has been minimized.

@ksubramz

ksubramz Oct 9, 2017

Contributor

Sascha, curious about why we need to be compatible with mkstemp (or mkdtemp), AFAIU, these are only called by our tests and if we drop the compatibility criterion, we could avoid passing "char *" and avoid using strlen() etc.

i.e. I think it's better to operate on std::string's instead of (less safe) C-style strings, use std::string methods and throw an exception from this function itself in-case of any error.

@ksubramz

ksubramz Oct 9, 2017

Contributor

Sascha, curious about why we need to be compatible with mkstemp (or mkdtemp), AFAIU, these are only called by our tests and if we drop the compatibility criterion, we could avoid passing "char *" and avoid using strlen() etc.

i.e. I think it's better to operate on std::string's instead of (less safe) C-style strings, use std::string methods and throw an exception from this function itself in-case of any error.

This comment has been minimized.

@saschazelzer

saschazelzer Oct 10, 2017

Member

The answer is a historical one. The code originates from the public domain on the Internet. As such it was C code, written to be compatible with mkstemp and friends by the original author. I only did very small changes initially (many years ago) and used the code as is in another project. It did its job well enough and I thought I would spend my time elsewhere instead of making it prettier for CppMicroServices.

Feel free to send a PR for C++ification or other improvements, but I think there is not much gain unless there is a bug of course.

@saschazelzer

saschazelzer Oct 10, 2017

Member

The answer is a historical one. The code originates from the public domain on the Internet. As such it was C code, written to be compatible with mkstemp and friends by the original author. I only did very small changes initially (many years ago) and used the code as is in another project. It did its job well enough and I thought I would spend my time elsewhere instead of making it prettier for CppMicroServices.

Feel free to send a PR for C++ification or other improvements, but I think there is not much gain unless there is a bug of course.

This comment has been minimized.

@ksubramz

ksubramz Oct 10, 2017

Contributor

Thanks. Sounds good.

@ksubramz

ksubramz Oct 10, 2017

Contributor

Thanks. Sounds good.

static const char validLetters[] = "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789";
// A cross-platform version of the mkstemps function

This comment has been minimized.

@ksubramz

ksubramz Oct 10, 2017

Contributor

Thanks. Sounds good.

@ksubramz

ksubramz Oct 10, 2017

Contributor

Thanks. Sounds good.

@saschazelzer saschazelzer merged commit 4fb25d9 into development Oct 11, 2017

0 of 5 checks passed

codecov/patch CI failed.
Details
codecov/project CI failed.
Details
continuous-integration/travis-ci/push The Travis CI build could not complete due to an error
Details
continuous-integration/appveyor/pr AppVeyor was unable to build non-mergeable pull request
Details
continuous-integration/appveyor/branch Waiting for AppVeyor build to complete
Details

@saschazelzer saschazelzer deleted the system-error-code-handling-cleanup branch Oct 29, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment