Merge GT.M V6.3-005 into YottaDB mainline #317
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…elevant in YottaDB mainline
…05 into YottaDB mainline In general, most of the conflicts were resolved by incorporating the V6.3-005 changes as long as they can co-exist with concurrent YottaDB changes. In case they cannot co-exist, the V6.3-005 changes were discarded. In a lot of cases, the only conflict was because of gtm/GTM name usage vs ydb/YDB name usage. These conflicts were resolved in a straightforward manner. Some of the conflict resolution though was not straightforward and would benefit from some more comments which are described below. * sr_port/gtm_string.h and sr_unix/util_output.c STRNCPY_LIT and STRNCPY_LIT_FULL macros are now removed as they are no longer used. This is because most of those usages were a source of buffer overflow warnings which meant fixing them to use something else. After incorporating V6.3-005 changes, the only remaining usage of STRNCPY_LIT in util_output.c is replaced with a MEMCPY_LIT (which is equivalent) but does not generate any buffer overflow warning like STRNCPY_LIT did. * sr_port/have_crit.h deferred_exit_handling_check() is an inline function introduced in V63005. But since conflicting changes were made to the equivalent DEFERRED_SIGNAL_HANDLING_CHECK macro in YottaDB and it was hard to retrofit, the new inline function was discarded. * sr_port/hd.mpt With V6.3-005 (GTM-5574), most of the numeric conversion functions use a new percent utility M program %CONVBASEUTIL. Since the changes involve enhancements to the range of numbers supported as well as some correctness fixes those were chosen instead of a conflicting YottaDB change (commit id afbb06e) without giving up the YottaDB-specific enhancement that hexadecimal numbers with a 0x or 0X prefix are supported. * sr_port/line.c In my understanding, TREF(compile_time) is guaranteed to be TRUE while we are in line() so it was not clear to me why a check of whether it was 0 was done in V6.3-005 so that conflicting part has been removed and instead an assert has been added to ensure this understanding is correct. Otherwise the V6.3-005 change was picked so we call show_source_line() only if the CQ_WARNINGS flag is ON instead of calling it with a "warn" parameter that is set to TRUE or FALSE depending on this flag in prior YottaDB versions. I don't think it makes a difference to the user either ways but it seems better to go with the V6.3-005 change for show_source_line() invocation. * sr_port/msg.m Every *errors_ctl.c file (e.g. merrors_ctl.c) now has a new section to list all undocumented messages in that file. This is printed as a LITDEF section with array names as gdeerrors_undocarr[], merrors_undocarr[] etc. This is now incorporated into the YottaDB version of msg.m (which generates the files differently than GT.M since r1.20). * sr_port/mstack_size_init.c and sr_port/mstack_size_init.h V6.3-005 adds a new env var gtm_mstack_crit_threshold. This is now folded into a new entry in sr_port/ydb_logicals_tab.h. And corresponding changes made to this file to use the index of the new entry here. Macro names MSTACK_CRIT_MIN_RANGE, MSTACK_CRIT_MAX_RANGE, MSTACK_CRIT_DEF_RANGE have been renamed to use THRESHOLD instead of RANGE to be in sync with the env var name which has "threshold" (not range) in it. * sr_port/objlabel.h V6.3-005 bumps the GT.M object file format. Now that those changes are incorporated into YottaDB, the YottaDB object file format is bumped from 4 to 5. * sr_port/op_zg1.c The change in V6.3-005 to this module seems to be from GTM-8943 to ensure a ZGOTO O inside a call-in returns to the parent C caller. This was already taken care of in YottaDB#32. So the V6.3-005 change is discarded. * sr_port/parm_pool.c and sr_port/push_parm_src.h The V6.3-005 changes included added comments on function parameters of push_parm(). But since this function had previously been moved to sr_port/push_parm_src.h in YottaDB, the new comments have been moved there instead. * sr_port/view_arg_convert.c The V6.3-005 change to this module used the stringpool as a temporary storage before calling format2zwr(). It had the following reasoning. * here & 2 other places use stringpool because we use format2zwr to ensure the message is graphic * & we don't return from the rts_error, so a fixed or malloc'd location seems even less attractive But I do not understand the above reasoning as we currently use a stack variable (which is neither fixed like a global variable, or malloced) and that storage goes away after the rts_error. And since YottaDB had fixed a lot more issues with VIEW and $VIEW (YottaDB#224) in r1.22, those change are considered and the V6.3-005 changes are discarded. * sr_unix/cli_parse.c V6.3-005 changed this file to use strncpy and pass the entire allocated buffer length which meant the buffer will be zero filled after the input string is copied. Since the allocated buffer length is MAX_LINE (greater than 32Kb), and since input command lines are less than a 100 bytes, that is a lot of zero filling done (almost 32Kb of zero filling) every time the strncpy() call. So the V6.3-005 change was discarded. YottaDB had already addressed the gcc 8.1 compiler warning differently (which is probably what prompted the V6.3-005 change) and that did not have this unnecessary overhead so that was kept as is. * sr_unix/gt_timers.c Conflicts were resolved by changing any place where deferred_timers_check_needed was set to FALSE or TRUE in V6.3-005 to instead use CLEAR_DEFERRED_TIMERS_CHECK_NEEDED or SET_DEFERRED_TIMERS_CHECK_NEEDED macros. * sr_unix/gtmci.c, sr_port/tp_restart.c and sr_unix/gtm_filter_command.c Any new usages of "frame_pointer->flags & SFF_CI" in V6.3-005 were replaced with "frame_pointer->type & SFT_CI" since SFF_CI is no longer present in YottaDB (nixed as part of YottaDB#32). Not yet sure if there are any subtle issues with this change. Will investigate as we find any issues while coming up with test cases for GTM-8877 (the ability to invoke an M filter from a C function) which is what triggered the SFF_CI changes in V6.3-005. * sr_unix/gtmci.c V6.3-005 had some changes to CITPNESTED error. But since this error no longer issued in YottaDB (since nested call-ins are allowed in TP due to YottaDB#188), those changes were discarded. * sr_unix/gtmrecv.c and sr_unix/gtmsource.c The V6.3-005 changes (due to GTM-8954) were discarded as similar changes were already made in YottaDB#210. In fact, with the GT.M V63005 changes, the r110/srcsrv_extfilter_sig11 subtest failed because stdout in source server was still pointing to /dev/null instead of source server log file resulting in messages related to the internal filter stopping not showing up in source server log. No such issues show up with YottaDB#210 so that is kept. * sr_unix/gtmsource.h and sr_unix/gtmsource_inline.h The JPL_PHASE2_WRITE_COMPLETE macro is now changed to an inline function jpl_phase2_write_complete() in V6.3-005. That change is incorporated with minor changes in names of variables etc. (gtm to ydb). * sr_unix/gvcst_init_sysops.c and sr_unix/mu_rndwn_file.c V6.3-005 changed this module to ensure we never use gtmsecshr to flush the db file header in case db has the READ_ONLY characteristic turned ON. This is similar to what was already done (amongst many other changes) in YottaDB#150. So the V6.3-005 change was discarded. * sr_unix/jnl_prc_vector.c V6.3-005 had changed SNPRINTF to use macros JPV_LEN_USER and JPV_LEN_PRCNAM. In YottaDB though, the same code was changed to address a gcc 8.1 compiler warning and that used SIZEOF(pv->jpv_user) and SIZEOF(pv->jpv_prcnam). Both usages are equivalent right now but could become different if the macros are changed inadvertently. The SIZEOF usage is safer since it ensures we never go beyond the allocated memory (i.e. no buffer overflows) so the YottaDB change is kept and the V6.3-005 change is discarded. * sr_unix/jobchild_init.c and sr_x86_64/opp_ciret.s V6.3-005 introduced changes to invoke opp_ciret() (a new assembly routine) in case of x86_64 else invoke ci_ret_code_exit(). Not sure why this was necessary. YottaDB#32 removed the need for ci_ret_code_exit() altogether so it is suspected that makes the new opp_ciret() also unnecessary. Therefore the V6.3-005 change is discarded and the new .s file nixed. * sr_unix/mupip_dump_fhead.c The V6.3-005 change was accepted/incorporated with a small name change (GTM_PATH_MAX -> YDB_PATH_MAX). * sr_unix/mupip_set_file.c The V6.3-005 change was due to GTM-8957. This change was already done (and in a more comprehensive fashion) by YottaDB#150. So the V6.3-005 change was discarded. * sr_unix/op_fnzsearch.c The V6.3-005 change was due to GTM-8990. But YottaDB#228 addresses the same issue in a better fashion in my understanding so the V6.3-005 change was discarded. * sr_unix/rel_crit.c and sr_unix/rel_lock.c DEFERRED_EXIT_HANDLING_CHECK macro in GT.M translates to DEFERRED_SIGNAL_HANDLING_CHECK macro in YottaDB. So if the GT.M macro was removed, the corresponding YottaDB macro was removed etc. The GT.M changes were accepted. * sr_unix/relinkctl.c V6.3-005 used OBJ_PLATFORM_LABEL to generate the relinkctl key file name. Since this macro was long obsoleted in YottaDB, that particular line was removed and the rest of the V6.3-005 changes were accepted. * sr_unix/sleep.h The V6.3-005 change was accepted except E_9 usages were changed to the more descriptive NANOSECS_IN_SEC. * sr_unix/suspend.c The V6.3-005 change was discarded as it moved the set of "suspend_status" before the send_msg_csa. But this variable had been nixed in YottaDB#215 and replaced with a "is_suspended" global variable which is never used anywhere but is there only to indicate the process is suspended. Moving this global variable before the send_msg_csa() should not affect anything (i.e. no recursion because this variable is never used anywhere).
Starting V6.3-005, msg.m expects the phrase "known undocumented" in each *errors.msg file. So all *.msg files are now fixed to conform to this expectation.
… as it can instead of returning 0 This ensures mpt_extra/conv subtest passes for cases added as part of YottaDB#44. As part of GTM-5574 changes in GT.M V6.3-005, the conversion function was changed to return 0 in case only a substring of the input number was valid. Whereas YottaDB had previously enhanced all the numeric conversion routines to convert as much of the input number as possible. Continuing in that trend, the new %CONVBASEUTIL M program is now enhanced to convert as much as it can.
… $VIEW(GDS2GVN) not working correctly due to regression introduced in V6.3-005 Not sure why this regression did not get detected by GT.M testing. I do not see any mention of this change in the V63005 release notes either. So undoing that change to let the $VIEW commands work correctly like before.
…5, for reasons unknown, to no longer accept a value) The unicode/dse subtest fails because of the V63005 change. Since there is no mention of this change in the V63005 release notes, this change is undone to let the test continue to run.
The scantypedefs script that we have access to expects every character array member in a structure to have a size of at least 1 byte. In GT.M V6.3-005, a char array size was changed from 1 to 0 bytes and so this is reverted back to 1 byte to let the scantypedefs script work fine without any more changes.
…nitDebug.m for sr_x86_64 and sr_armv7l
….3-005 where NOPRINCIO error no longer showed up in syslog when MUPIP LOAD process lost its terminal/principal-device The primary issue was in GTM_FWRITE macro where rc was being assigned to the return value of ferror(). I think this was done with the intent that ferror() will return the "errno" of the fwrite() call. But ferror() returns a non-zero value in case of an error and the actual value it returned was 1 in case of an ENO5/EIO. So rc had the value of 1 which to the caller gtm_fprintf() incorrectly signaled a successful call to fprintf() resulting in the NOPRINCIO error not being written to syslog.
…3-005) The unicode/dse subtest (with dbg) failed as follows only on the arm boxes (not on x86_64). DSE> %YDB-F-ASSERT, Assert failed in sr_port/gtm_malloc_src.h line 904 for expression (0 == memcmp(trailerMarker, markerChar, SIZEOF(markerChar))) It is an off-by-one buffer overflow in DSE and is a portable issue. Fixes in this area were done in GT.M V6.3-005 as part of GTM-9001 but look like they were incomplete. The off-by-one buffer overrun is now fixed in 2 more functions (cli_str_setup and cli_lex_setup). GTM-9001 did it only in 1 function (cli_lex_in_expand).
estess
reviewed
Jul 30, 2018
| #define JNL_HANG_TRIGGER 500 | ||
| #define JNL_LSEEKWRITE_HANG(CSA) jnl_lseekwrite_hang(CSA) | ||
|
|
||
| static inline boolean_t jnl_lseekwrite_hang(struct sgmnt_addrs_struct *csa) |
There was a problem hiding this comment.
Should these static routines be STATICFNDEFs instead with their declarations likewise tagged STATICFNDCL? Else the routine names won't show up in a pro core.
Steve
There was a problem hiding this comment.
Ah, I see, these are static INLINE so nevermind for now. Just hope they show up correctly.
estess
reviewed
Jul 30, 2018
| @@ -1329,8 +1330,8 @@ void gvcst_init(gd_region *reg, gd_addr *addr) | |||
| if (NO_STATS_OPTIN != TREF(statshare_opted_in)) | |||
| { | |||
| if (!is_statsDB) | |||
| { /* This is a baseDB - so long as NOSTATS is *not* turned on, we should initialize the statsDB */ | |||
| if (!(RDBF_NOSTATS & reg->reservedDBFlags)) | |||
| { /* This is a baseDB - so long if all in, we should initialize the statsDB */ | |||
There was a problem hiding this comment.
Remove the "so long" part as the phrase doesn't make much sense. Probably should hyphenate "all-in" to make it more clear this is a "state".
estess
reviewed
Jul 30, 2018
| $ZQUIT=0 | ||
| $ZREALSTOR=694280 | ||
| $ZRELDATE="20180614 00:33" | ||
| $ZROUTINES=". /usr/lib/fis-gtm/V6.3-005_x86_64 /usr/lib/fis-gtm/V6.3-005_x86_64/plugin/o(/usr/lib/fis-gtm/V6.3-005_x86_64/plugin/r)" |
There was a problem hiding this comment.
Should $ZRELEASE be added in here along with any other SVNs we have added and changing the fis-gtm to something more yotta-ish?
There was a problem hiding this comment.
Note there are several fis-gtm references in the various help files.
There was a problem hiding this comment.
I meant all the fis-gtm references are now made yotta-ish.
estess
reviewed
Jul 30, 2018
|
|
||
| GBLREF stack_frame *frame_pointer; | ||
|
|
||
| void gtm_init_env(rhdtyp *base_addr, unsigned char *transfer_addr) | ||
| { | ||
| assert(CURRENT_RHEAD_ADR(base_addr) == base_addr); | ||
| ESTABLISH(mdb_condition_handler); |
There was a problem hiding this comment.
Are there any tests known to drive this code? I'm concerned about mdb_condition_handler being driven at this point before everything is set up. Certainly we should NOT be driving an M code error handler at this point (possible if $gtm_etrap is set) as there is as of yet no save area since dm_start has not yet run. Not sure what this is an alleged cure for.
There was a problem hiding this comment.
I will investigate this further.
There was a problem hiding this comment.
I don't see any way to trigger an error inside gtm_init_env.c. The only error possibilities are STACKCRIT and STACKOFLOW. And because this function is invoked only when the process starts, the M stack is empty at that time AND the M-stack size is a minimum of 25KiB, there is no way of getting a STACKCRIT error for the one or two frames that we allocate at startup. So not sure why this change was necessary. Maybe it was done as part of general cleanup. I don't see any way having the ESTABLISH/REVERT makes any difference because of this. Given this, would you like removing this or keep it as is? My inclination is to leave it as is.
There was a problem hiding this comment.
Posting @estess response in the right thread.
I would like to remove it as I think it is dangerous as it sits in case
someone looks it over and figures it can deal with a newly added error
or something. I'm really curious as to the origin of that addition and
what it allegedly solved. Oh well.
Steve
There was a problem hiding this comment.
The ESTABLISH/REVERT of mdb_condition_handler in gtm_init_env.c is now removed.
estess
reviewed
Jul 30, 2018
| @@ -71,6 +71,7 @@ void gtm_free(void *); | |||
| void gtm_hiber_start(ydb_uint_t mssleep); | |||
| void gtm_hiber_start_wait_any(ydb_uint_t mssleep); | |||
| void gtm_start_timer(ydb_tid_t tid, ydb_int_t time_to_expir, void (*handler)(), ydb_int_t hdata_len, void *hdata); | |||
| gtm_status_t gtm_ci_filter(const char *c_rtn_name, ...); | |||
There was a problem hiding this comment.
Looks like GTM added a new externally accessible routine (who does I know not what yet) that likely needs a ydb counterpart. Might be a new utility routine. Depending on what it does, might need a Golang flavor as well if that turns out to be useful.
There was a problem hiding this comment.
gtm_ci_filter() is not externally accessible. It is currently only used for invoking M code when running ZSYSTEM or OPEN command for a PIPE device. So no need for any Golang flavor in my understanding.
There was a problem hiding this comment.
If it is not actually exported, then the declaration should be moved from gtmxc_types.h (which IS user accessible) to some other internal header file.
There was a problem hiding this comment.
Done. Now moved to sr_unix/gtmci.h.
estess
reviewed
Jul 30, 2018
| @@ -323,6 +328,7 @@ short iopi_open(io_log_name *dev_name, mval *pp, int fd, mval *mspace, int4 time | |||
| #endif | |||
| char dev_name_buf[LOGNAME_LEN]; | |||
| mstr dev_mstr; | |||
| gtm_string_t filtered_command; | |||
There was a problem hiding this comment.
We have almost banned gtm_string_t from the YDB code base so this one too should likely be changed to ydb_string_t.
There was a problem hiding this comment.
Done. That said, there are still a couple dozen usages of gtm_string_t in the encryption plugin.
estess
reviewed
Jul 31, 2018
There was a problem hiding this comment.
I did not call out numerous coding standards violations the most prevalent of which is not putting the "else" statement on the same line as the closing brace. Others include opening brace not starting a new line and a very odd syntax for documenting parameters of routines I personally found very difficult to read.
While I had some comments (and made those), since this is already committed, I'm marking it Approved.
|
I addressed all your comments. If you know of any specific modules that need "else" or other coding standards issues fixed let me know and I can do it. |
|
I didn't keep track of them so I guess we'll fix them as we run into
them again.
…On 08/01/2018 02:24 PM, Narayanan Iyer wrote:
I addressed all your comments. If you know of any specific modules
that need "else" or other coding standards issues fixed let me know
and I can do it.
—
You are receiving this because your review was requested.
Reply to this email directly, view it on GitHub
<#317 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/Abx8ONqVq5lfyodNmGZ3HfOPlWcq4zHVks5uMfJZgaJpZM4Vbhdr>.
|
nars1
added a commit
to nars1/YottaDB
that referenced
this pull request
Aug 1, 2018
|
I would like to remove it as I think it is dangerous as it sits in case
someone looks it over and figures it can deal with a newly added error
or something. I'm really curious as to the origin of that addition and
what it allegedly solved. Oh well.
Steve
…On 08/01/2018 04:08 PM, Narayanan Iyer wrote:
***@***.**** commented on this pull request.
------------------------------------------------------------------------
In sr_unix/gtm_init_env.c
<#317 (comment)>:
>
GBLREF stack_frame *frame_pointer;
void gtm_init_env(rhdtyp *base_addr, unsigned char *transfer_addr)
{
assert(CURRENT_RHEAD_ADR(base_addr) == base_addr);
+ ESTABLISH(mdb_condition_handler);
I don't see any way to trigger an error inside gtm_init_env.c. The
only error possibilities are STACKCRIT and STACKOFLOW. And because
this function is invoked only when the process starts, the M stack is
empty at that time AND the M-stack size is a minimum of 25KiB, there
is no way of getting a STACKCRIT error for the one or two frames that
we allocate at startup. So not sure why this change was necessary.
Maybe it was done as part of general cleanup. I don't see any way
having the ESTABLISH/REVERT makes any difference because of this.
Given this, would you like removing this or keep it as is? My
inclination is to leave it as is.
—
You are receiving this because your review was requested.
Reply to this email directly, view it on GitHub
<#317 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/Abx8OE8VtXH5fzSllDao4jYhO7SG94g0ks5uMgq1gaJpZM4Vbhdr>.
|
nars1
added a commit
that referenced
this pull request
Aug 1, 2018
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.