From 9a478edb06684cf663e2fda4b96a1a92aa9e8fb4 Mon Sep 17 00:00:00 2001 From: Dennis Heimbigner Date: Mon, 20 May 2024 19:15:19 -0600 Subject: [PATCH 1/3] Fix duplicate definition when using aws-sdk-cpp. re: Issue https://github.com/Unidata/netcdf-c/issues/2927 The NC_s3sdkinitialize NC_s3sdkfinalize functions were misplaced. They should have been moved from ds3util.c to ncs3sdk_h5.c. When using ncs3sdl_aws.cpp, this resulted in a duplicate definition. Also, found and fixed a memory leak in the NCZarr S3 code. --- include/ncs3sdk.h | 4 ++-- libdispatch/ds3util.c | 48 +++++++++++-------------------------- libdispatch/ncs3sdk_aws.cpp | 9 +++---- libdispatch/ncs3sdk_h5.c | 31 ++++++++++++++++++++++++ libnczarr/zmap_s3sdk.c | 11 +++++---- nczarr_test/run_corrupt.sh | 5 +++- 6 files changed, 62 insertions(+), 46 deletions(-) diff --git a/include/ncs3sdk.h b/include/ncs3sdk.h index 7be09da337..50f2ad2708 100644 --- a/include/ncs3sdk.h +++ b/include/ncs3sdk.h @@ -45,6 +45,7 @@ struct NCglobalstate; extern "C" { #endif +/* API for ncs3sdk_XXX.[c|cpp] */ EXTERNL int NC_s3sdkinitialize(void); EXTERNL int NC_s3sdkfinalize(void); EXTERNL void* NC_s3sdkcreateclient(NCS3INFO* context); @@ -60,8 +61,7 @@ EXTERNL int NC_s3sdksearch(void* s3client0, const char* bucket, const char* pref EXTERNL int NC_s3sdkdeletekey(void* client0, const char* bucket, const char* pathkey, char** errmsgp); /* From ds3util.c */ -EXTERNL int NC_s3sdkinitialize(void); -EXTERNL int NC_s3sdkfinalize(void); +EXTERNL void NC_s3sdkenvironment(void); EXTERNL int NC_getdefaults3region(NCURI* uri, const char** regionp); EXTERNL int NC_s3urlprocess(NCURI* url, NCS3INFO* s3, NCURI** newurlp); diff --git a/libdispatch/ds3util.c b/libdispatch/ds3util.c index 8db08d2e82..aab8fe987b 100644 --- a/libdispatch/ds3util.c +++ b/libdispatch/ds3util.c @@ -43,9 +43,6 @@ enum URLFORMAT {UF_NONE=0, UF_VIRTUAL=1, UF_PATH=2, UF_S3=3, UF_OTHER=4}; static const char* awsconfigfiles[] = {".aws/config",".aws/credentials",NULL}; #define NCONFIGFILES (sizeof(awsconfigfiles)/sizeof(char*)) -static int ncs3_initialized = 0; -static int ncs3_finalized = 0; - /**************************************************/ /* Forward */ @@ -56,38 +53,21 @@ static int awsparse(const char* text, NClist* profiles); /**************************************************/ /* Capture environmental Info */ -EXTERNL int -NC_s3sdkinitialize(void) -{ - if(!ncs3_initialized) { - ncs3_initialized = 1; - ncs3_finalized = 0; - } - { - /* Get various environment variables as defined by the AWS sdk */ - NCglobalstate* gs = NC_getglobalstate(); - if(getenv("AWS_REGION")!=NULL) - gs->aws.default_region = nulldup(getenv("AWS_REGION")); - else if(getenv("AWS_DEFAULT_REGION")!=NULL) - gs->aws.default_region = nulldup(getenv("AWS_DEFAULT_REGION")); - else if(gs->aws.default_region == NULL) - gs->aws.default_region = nulldup(AWS_GLOBAL_DEFAULT_REGION); - gs->aws.access_key_id = nulldup(getenv("AWS_ACCESS_KEY_ID")); - gs->aws.config_file = nulldup(getenv("AWS_CONFIG_FILE")); - gs->aws.profile = nulldup(getenv("AWS_PROFILE")); - gs->aws.secret_access_key = nulldup(getenv("AWS_SECRET_ACCESS_KEY")); - } - return NC_NOERR; -} - -EXTERNL int -NC_s3sdkfinalize(void) +EXTERNL void +NC_s3sdkenvironment(void) { - if(!ncs3_finalized) { - ncs3_initialized = 0; - ncs3_finalized = 1; - } - return NC_NOERR; + /* Get various environment variables as defined by the AWS sdk */ + NCglobalstate* gs = NC_getglobalstate(); + if(getenv("AWS_REGION")!=NULL) + gs->aws.default_region = nulldup(getenv("AWS_REGION")); + else if(getenv("AWS_DEFAULT_REGION")!=NULL) + gs->aws.default_region = nulldup(getenv("AWS_DEFAULT_REGION")); + else if(gs->aws.default_region == NULL) + gs->aws.default_region = nulldup(AWS_GLOBAL_DEFAULT_REGION); + gs->aws.access_key_id = nulldup(getenv("AWS_ACCESS_KEY_ID")); + gs->aws.config_file = nulldup(getenv("AWS_CONFIG_FILE")); + gs->aws.profile = nulldup(getenv("AWS_PROFILE")); + gs->aws.secret_access_key = nulldup(getenv("AWS_SECRET_ACCESS_KEY")); } /**************************************************/ diff --git a/libdispatch/ncs3sdk_aws.cpp b/libdispatch/ncs3sdk_aws.cpp index 806edae8a8..e1bceb7835 100644 --- a/libdispatch/ncs3sdk_aws.cpp +++ b/libdispatch/ncs3sdk_aws.cpp @@ -133,10 +133,9 @@ NC_s3sdkinitialize(void) if(!ncs3_initialized) { ncs3_initialized = 1; ncs3_finalized = 0; - #ifdef DEBUG - //ncs3options.loggingOptions.logLevel = Aws::Utils::Logging::LogLevel::Debug; + //ncs3options.loggingOptions.logLevel = Aws::Utils::Logging::LogLevel::Debug; ncs3options.loggingOptions.logLevel = Aws::Utils::Logging::LogLevel::Trace; ncs3options.httpOptions.installSigPipeHandler = true; ncs3options.loggingOptions.logger_create_fn = [] { return std::make_shared(Aws::Utils::Logging::LogLevel::Trace); }; @@ -144,6 +143,9 @@ NC_s3sdkinitialize(void) #endif Aws::InitAPI(ncs3options); + /* Get environment information */ + NC_s3sdkenvironment(); + } return NCUNTRACE(NC_NOERR); } @@ -500,7 +502,6 @@ NC_s3sdkwriteobject(void* s3client0, const char* bucket, const char* pathkey, s int stat = NC_NOERR; const char* key = NULL; - const char* mcontent = (char*)content; NCTRACE(11,"bucket=%s pathkey=%s count=%lld content=%p",bucket,pathkey,count,content); AWSS3CLIENT s3client = (AWSS3CLIENT)s3client0; @@ -535,7 +536,7 @@ NC_s3sdkwriteobject(void* s3client0, const char* bucket, const char* pathkey, s put_request.SetContentLength((long long)count); std::shared_ptr data = std::shared_ptr(new Aws::StringStream()); - data->rdbuf()->pubsetbuf((char*)content,count); + data->rdbuf()->pubsetbuf((char*)content,(std::streamsize)count); put_request.SetBody(data); auto put_result = AWSS3GET(s3client)->PutObject(put_request); if(!put_result.IsSuccess()) { diff --git a/libdispatch/ncs3sdk_h5.c b/libdispatch/ncs3sdk_h5.c index 5f7f223db9..f8263293b7 100644 --- a/libdispatch/ncs3sdk_h5.c +++ b/libdispatch/ncs3sdk_h5.c @@ -108,6 +108,37 @@ static int queryinsert(NClist* list, char* ekey, char* evalue); #define NT(x) ((x)==NULL?"null":x) +/**************************************************/ + +static int ncs3_initialized = 0; +static int ncs3_finalized = 0; + +EXTERNL int +NC_s3sdkinitialize(void) +{ + if(!ncs3_initialized) { + ncs3_initialized = 1; + ncs3_finalized = 0; + } + + /* Get environment information */ + NC_s3sdkenvironment(void); + + return NC_NOERR; +} + +EXTERNL int +NC_s3sdkfinalize(void) +{ + if(!ncs3_finalized) { + ncs3_initialized = 0; + ncs3_finalized = 1; + } + return NC_NOERR; +} + +/**************************************************/ + #if 0 static void dumps3info(NCS3INFO* s3info, const char* tag) diff --git a/libnczarr/zmap_s3sdk.c b/libnczarr/zmap_s3sdk.c index 0acdaf8e99..552a73473d 100644 --- a/libnczarr/zmap_s3sdk.c +++ b/libnczarr/zmap_s3sdk.c @@ -499,20 +499,21 @@ s3clear(void* s3client, const char* bucket, const char* rootkey) { int stat = NC_NOERR; char** list = NULL; - char** p; size_t nkeys = 0; if(s3client && bucket && rootkey) { if((stat = NC_s3sdksearch(s3client, bucket, rootkey, &nkeys, &list, NULL))) goto done; if(list != NULL) { - for(p=list;*p;p++) { + size_t i; + for(i=0;i Date: Fri, 24 May 2024 16:48:04 -0600 Subject: [PATCH 2/3] Cleanup handling of NETCDF_ENABLE_SET_LOG_LEVEL and NETCDF_ENABLE_SET_LOG_LEVEL_FUNC The NETCDF_ENABLE_SET_LOG_LEVEL_FUNC option is apparently not used, but is effectively used to set NETCDF_ENABLE_SET_LOG_LEVEL. This is not clear from the build files CMakeLists.txt and configure.ac. So this PR cleanups the code to make it more clear what is going on. --- CMakeLists.txt | 14 +++++++------- config.h.cmake.in | 5 +---- configure.ac | 26 ++++++++++++++++++-------- include/nc_logging.h | 6 ++---- libsrc4/nc4internal.c | 4 ++-- 5 files changed, 30 insertions(+), 25 deletions(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index 4bac1aad30..cfd2e0daa2 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -532,20 +532,20 @@ else() set(NETCDF_ENABLE_HDF4 OFF) endif() -# Option Logging, only valid for netcdf4. +# Option Logging, only valid for netcdf4 dispatchers. option(NETCDF_ENABLE_LOGGING "Enable Logging." OFF) if(NOT NETCDF_ENABLE_NETCDF4) set(NETCDF_ENABLE_LOGGING OFF) endif() if(NETCDF_ENABLE_LOGGING) - target_compile_definitions(netcdf PRIVATE LOGGING NETCDF_ENABLE_SET_LOG_LEVEL) - set(LOGGING ON) - set(NETCDF_ENABLE_SET_LOG_LEVEL ON) + set(LOGGING ON CACHE BOOL "") # Alias endif() + option(NETCDF_ENABLE_SET_LOG_LEVEL_FUNC "Enable definition of nc_set_log_level()." ON) -if(NETCDF_ENABLE_NETCDF4 AND NOT NETCDF_ENABLE_LOGGING AND NETCDF_ENABLE_SET_LOG_LEVEL_FUNC) - target_compile_definitions(netcdf PRIVATE -DNETCDF_ENABLE_SET_LOG_LEVEL) - set(NETCDF_ENABLE_SET_LOG_LEVEL ON) +if(LOGGING AND NETCDF_ENABLE_SET_LOG_LEVEL_FUNC) + set(NETCDF_ENABLE_SET_LOG_LEVEL ON CACHE BOOL "") +else() + set(NETCDF_ENABLE_SET_LOG_LEVEL OFF CACHE BOOL "") endif() # Option to allow for strict null file padding. diff --git a/config.h.cmake.in b/config.h.cmake.in index 759b13a852..9de5a071d7 100644 --- a/config.h.cmake.in +++ b/config.h.cmake.in @@ -475,10 +475,7 @@ with zip */ #cmakedefine LOGGING 1 /* If true, define nc_set_log_level. */ -#cmakedefine ENABLE_SET_LOG_LEVEL 1 - -/* If true, define nc_set_log_level_func */ -#cmakedefine NETCDF_ENABLE_SET_LOG_LEVEL_FUNC 1 +#cmakedefine NETCDF_ENABLE_SET_LOG_LEVEL 1 /* min blocksize for posixio. */ #cmakedefine NCIO_MINBLOCKSIZE ${NCIO_MINBLOCKSIZE} diff --git a/configure.ac b/configure.ac index 90def36716..2254c2c50a 100644 --- a/configure.ac +++ b/configure.ac @@ -451,16 +451,26 @@ AC_ARG_ENABLE([logging], test "x$enable_logging" = xyes || enable_logging=no AC_MSG_RESULT([$enable_logging]) -# Does the user want to turn off nc_set_log_level() function? (It will -# always be defined if --enable-logging is used.) -AC_MSG_CHECKING([whether nc_set_log_level() function is included (will do nothing unless enable-logging is also used)]) -AC_ARG_ENABLE([set_log_level_func], [AS_HELP_STRING([--disable-set-log-level-func], +# Does the user want to disable the nc_set_log_level() function? (It will +# always be defined (possibly as a no-op) if --enable-logging is used.) +AC_MSG_CHECKING([whether nc_set_log_level() function is enabled (will do nothing unless enable-logging is also used)]) +AC_ARG_ENABLE([set-log-level-func], [AS_HELP_STRING([--disable-set-log-level-func], [disable the nc_set_log_level function])]) -test "x$enable_set_log_level_func" = xno -a "x$enable_logging" = xno || enable_set_log_level_func=yes -if test "x$enable_set_log_level_func" = xyes -a "x$enable_hdf5" = xyes; then - AC_DEFINE([ENABLE_SET_LOG_LEVEL], 1, [If true, define nc_set_log_level.]) +test "x$enable_set_log_level_func" = xno || enable_set_log_level_func=yes +if test "x$enable_logging" = xno; then +enable_set_log_level_func=no +fi + +# alias to enable_set_log_level +if test "x$enable_set_log_level_func" = xyes; then +enable_set_log_level=yes +else +enable_set_log_level=no +fi +AC_MSG_RESULT($enable_set_log_level) +if test "x$enable_set_log_level" = xyes ; then + AC_DEFINE([NETCDF_ENABLE_SET_LOG_LEVEL], 1, [If true, enable nc_set_log_level function.]) fi -AC_MSG_RESULT($enable_set_log_level_func) # CURLOPT_USERNAME is not defined until curl version 7.19.1 # CURLOPT_PASSWORD is not defined until curl version 7.19.1 diff --git a/include/nc_logging.h b/include/nc_logging.h index 663e2c7b74..7d1f5f45e2 100644 --- a/include/nc_logging.h +++ b/include/nc_logging.h @@ -52,12 +52,10 @@ void nc_log(int severity, const char *fmt, ...); #define BAIL_QUIET BAIL -#ifdef USE_NETCDF4 -#ifndef ENABLE_SET_LOG_LEVEL +#ifndef NETCDF_ENABLE_SET_LOG_LEVEL /* Define away any calls to nc_set_log_level(), if its not enabled. */ #define nc_set_log_level(e) -#endif /* ENABLE_SET_LOG_LEVEL */ -#endif +#endif /* NETCDF_ENABLE_SET_LOG_LEVEL */ #endif /* LOGGING */ diff --git a/libsrc4/nc4internal.c b/libsrc4/nc4internal.c index fda731d003..f7f32c7ca8 100644 --- a/libsrc4/nc4internal.c +++ b/libsrc4/nc4internal.c @@ -1711,7 +1711,7 @@ nc4_normalize_name(const char *name, char *norm_name) return NC_NOERR; } -#ifdef ENABLE_SET_LOG_LEVEL +#ifdef NETCDF_ENABLE_SET_LOG_LEVEL /** * Initialize parallel I/O logging. For parallel I/O builds, open log @@ -1812,7 +1812,7 @@ nc_set_log_level(int new_level) return NC_NOERR; } -#endif /* ENABLE_SET_LOG_LEVEL */ +#endif /* NETCDF_ENABLE_SET_LOG_LEVEL */ #if LOGGING #define MAX_NESTS 10 From ddb0043eda176d256b0485d277143b142d1846f2 Mon Sep 17 00:00:00 2001 From: Dennis Heimbigner Date: Fri, 24 May 2024 18:16:21 -0600 Subject: [PATCH 3/3] Update release notes --- RELEASE_NOTES.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/RELEASE_NOTES.md b/RELEASE_NOTES.md index be114b328d..623c0bc853 100644 --- a/RELEASE_NOTES.md +++ b/RELEASE_NOTES.md @@ -7,6 +7,8 @@ This file contains a high-level description of this package's evolution. Release ## 4.9.3 - TBD +* Cleanup the option code for NETCDF_ENABLE_SET_LOG_LEVEL\[_FUNC\] See [Github #2931](https://github.com/Unidata/netcdf-c/issues/2931) for more information. +* Fix duplicate definition when using aws-sdk-cpp. See [Github #2928](https://github.com/Unidata/netcdf-c/issues/2928) for more information. * Cleanup various obsolete options and do some code refactoring. See [Github #2926](https://github.com/Unidata/netcdf-c/issues/2926) for more information. * Convert the Zarr-related ENABLE_XXX options to NETCDF_ENABLE_XXX options (part of the cmake overhaul). See [Github #2923](https://github.com/Unidata/netcdf-c/issues/2923) for more information. * Refactor macro `_FillValue` to `NC_FillValue` to avoid conflict with libc++ headers. See [Github #2858](https://github.com/Unidata/netcdf-c/issues/2858) for more information.