add consistent character lengths for files and paths #1567
Conversation
There was a problem hiding this comment.
I looked at the platform files and that looked alright, and very briefly skimmed the rest. Will defer to @bensonr because I am not going to be available for a more thorough review.
|
@climbfuji @mathomp4 @Dooruk Does this work sufficiently fix your issue? |
mpp/include/mpp_util.inc
Outdated
| @@ -1348,8 +1348,7 @@ end function rarray_to_char | |||
| integer, dimension(2) :: get_ascii_file_num_lines_and_length !< number of lines (1) and | |||
| !! max line length (2) | |||
| integer :: num_lines, max_length | |||
| integer, parameter :: LENGTH=1024 | |||
| character(len=LENGTH) :: str_tmp | |||
| character(len=FMS_FILE_LEN) :: str_tmp | |||
There was a problem hiding this comment.
This is not a filename, but the length of a string being read from an ascii file for conversion to a character variable. Using FMS_FILE_LEN would now impose a limit of 255 from the original 1024.
There was a problem hiding this comment.
Fixed with f9cac03, all the changes for that routine should be reverted.
mpp/include/mpp_util.inc
Outdated
| if ( len_trim(str_tmp) == FMS_FILE_LEN ) then | ||
| write(UNIT=text, FMT='(I5)') FMS_FILE_LEN |
There was a problem hiding this comment.
I think we want to revert this to the original code here as the LENGTH here is not referring to a file length or pathname.
mpp/include/mpp_util.inc
Outdated
| call mpp_error(FATAL, 'get_ascii_file_num_lines: Length of output string ('//trim(text)//& | ||
| & ' is too small. Increase the LENGTH value.') | ||
| & ' is too small. Increase the FMS_MAX_FILE_LEN cpp value.') |
There was a problem hiding this comment.
I think we want to revert this as well.
| @@ -126,7 +126,7 @@ module time_interp_external2_mod | |||
| !> Holds filename and file object | |||
| !> @ingroup time_interp_external2_mod | |||
| type, private :: filetype | |||
| character(len=128) :: filename = '' | |||
| character(len=FMS_PATH_LEN) :: filename = '' | |||
There was a problem hiding this comment.
This is a filename, shouldn't it be limited using FMS_FILE_LEN? If so, need to change the platform module use statement as well.
There was a problem hiding this comment.
Yeah I think this one should be a file length, fixed with f9cac03
…h to file instead of path
@bensonr I skipped the tests because most file/path names used in the tests are typically set once and not modified so there's not really a chance of one being too short. There's a couple cases where a filename can be set by a namelist, but then the values are just set in the test scripts so it would always be set to a value we control. Do you think they should be updated as well? |
@rem1776 - hopefully we'll find out during testing or via the automated ci/cd |
Description
FMS_PATH_LENandFMS_FILE_LENthat are set by theFMS_MAX_PATH_LENandFMS_MAX_FILE_LENcpp macros in the fms_platform.h file and default to 1024 and 255 respectively.fm_path_name_len. Since it is used externally, I removed internal usage and set the value to FMS_PATH_LEN so we can remove it later along with updating the components, see issue remove path length parameter from field_manager #1564.len=*instead of a set length for any filename related character argumentsIt can be somewhat ambiguous what is intended as a filename vs a full path name, so let me know if you think a length should be one instead of the other or if anything was missed.
Fixes #1518
How Has This Been Tested?
gnu 13 + mpich on the amd box
Checklist:
make distcheckpasses