Skip to content
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

Ctrl-Z (the suspend signal) is honored in all cases #215

Closed
nars1 opened this issue Apr 27, 2018 · 0 comments
Closed

Ctrl-Z (the suspend signal) is honored in all cases #215

nars1 opened this issue Apr 27, 2018 · 0 comments
Assignees
Milestone

Comments

@nars1
Copy link
Collaborator

nars1 commented Apr 27, 2018

Final Release Note

Ctrl-Z (the suspend signal) is honored by YottaDB processes in all cases. Previously, if the signal was delivered while the process was executing a section of code where it was not safe to suspend it, the signal was ignored and the user had to retry the Ctrl-Z. Note that as handling the signal is deferred when the process is in sensitive areas of code, it is possible for the response to be momentarily delayed. (#215)

Description

Ctrl-Z of a YottaDB process is normally honored by suspending the process. But if the process is executing some critical section of the code where it cannot be interrupted (for example if the process is allocation memory or displaying the the YDB> or MUPIP> or LKE> prompt etc.) , and the Ctrl-Z comes in during that time window, the Ctrl-Z is currently ignored. This is undesirable.

It is okay to defer suspending the process until the process leaves the critical code section but the suspend should take effect soon after.

Draft Release Note

Ctrl-Z is honored by YottaDB processes in all cases. Previously, if the Ctrl-Z came in while the process is executing some critical section of the code where it cannot be interrupted (for example if the process is allocation memory or displaying the the YDB> or MUPIP> or LKE> prompt etc.), it was ignored and the user had to retry the Ctrl-Z until it worked.

@nars1 nars1 added this to the r130 milestone Apr 27, 2018
@nars1 nars1 self-assigned this Apr 27, 2018
nars1 added a commit to nars1/YottaDB that referenced this issue Apr 27, 2018
…t just deferred signals that terminate the process but also those that suspend the process (Ctrl-Z)

In the v63004/gtm8791 subtest, it was noticed that a Ctrl-Z was not handled
(i.e. the process was not suspended) when the test ran on slower systems.
Turns out it was because when the LKE> prompt was being displayed, interrupts
are deferred in the process and if a Ctrl-Z is done at that time, we set
the global variable "suspend_status" to DEFER_SUSPEND to record this fact
and continue but once the interrupts are enabled, we only do checks of deferred
exit handling signals, not of the suspend signal. This means the Ctrl-Z is
effectively ignored. At least until the next rel_crit/rel_lock happens in this
process since that is when the "suspend_status" global is checked again. This
is unfriendly so the DEFERRED_EXIT_HANDLING_CHECK macro (which is invoked every
time we come out of an interrupt deferred zone) is renamed as
DEFERRED_SIGNAL_HANDLING_CHECK and Ctrl-Z deferral is also checked as part of
this macro.
nars1 added a commit to nars1/YottaDBtest that referenced this issue Apr 27, 2018
… run on slow systems, Ctrl-Z was not honored)
nars1 added a commit to nars1/YottaDBtest that referenced this issue Apr 30, 2018
…s in YottaDB/YDB#215)

The test was doing a SET ^a=1 followed by a HANG 1 and another SET ^a=2 in order
to reach jnl_sub_qio_start() while we were not holding crit lock so we would suspend
ourselves (due to the white-box test case) and cause another process to issue a
JNLPROCSTUCK message.

jnl_sub_qio_start() was invoked a lot of times in the above sequence of commands
and the first invocation itself resulted in a call to suspsigs_handler() to suspend
but it did not suspend because we were holding crit. The invocation that caused the
process to suspend was a little later when jnl_sub_qio_start() was invoked through
t_end() -> wcs_timer_start() -> jnl_qio_start() -> jnl_sub_qio_start(). But before
t_end() invokes wcs_timer_start(), it does a REVERT which now causes a check of
whether suspend() was deferred (due to YottaDB#215 code changes) and so we suspend the
process right there (because at this point, we have released crit and are out
of the phase2 commit logic). And so the process does not ever reach wcs_timer_start()
and never gets suspended while holding the jnl buffer qio lock like the test expects
causing the test to eventually fail (because no JNLPROCSTUCK message is later seen).

To work around this, the sequence of commands done by the test have now been simplified
to a SET ^a=1 followed by a VIEW "JNLWAIT". The latter invokes jnl_wait() which call
jnl_sub_qio_start() while we do not hold crit and so the process suspends right there
recreating the same situation that the test wants for a later JNLPROCSTUCK message.
nars1 added a commit that referenced this issue Apr 30, 2018
…deferred signals that terminate the process but also those that suspend the process (Ctrl-Z)

In the v63004/gtm8791 subtest, it was noticed that a Ctrl-Z was not handled
(i.e. the process was not suspended) when the test ran on slower systems.
Turns out it was because when the LKE> prompt was being displayed, interrupts
are deferred in the process and if a Ctrl-Z is done at that time, we set
the global variable "suspend_status" to DEFER_SUSPEND to record this fact
and continue but once the interrupts are enabled, we only do checks of deferred
exit handling signals, not of the suspend signal. This means the Ctrl-Z is
effectively ignored. At least until the next rel_crit/rel_lock happens in this
process since that is when the "suspend_status" global is checked again. This
is unfriendly so the DEFERRED_EXIT_HANDLING_CHECK macro (which is invoked every
time we come out of an interrupt deferred zone) is renamed as
DEFERRED_SIGNAL_HANDLING_CHECK and Ctrl-Z deferral is also checked as part of
this macro.
@nars1 nars1 closed this as completed Apr 30, 2018
nars1 added a commit to YottaDB/YDBTest that referenced this issue Apr 30, 2018
… run on slow systems, Ctrl-Z was not honored)
nars1 added a commit to YottaDB/YDBTest that referenced this issue Apr 30, 2018
…ttaDB/YDB#215)

The test was doing a SET ^a=1 followed by a HANG 1 and another SET ^a=2 in order
to reach jnl_sub_qio_start() while we were not holding crit lock so we would suspend
ourselves (due to the white-box test case) and cause another process to issue a
JNLPROCSTUCK message.

jnl_sub_qio_start() was invoked a lot of times in the above sequence of commands
and the first invocation itself resulted in a call to suspsigs_handler() to suspend
but it did not suspend because we were holding crit. The invocation that caused the
process to suspend was a little later when jnl_sub_qio_start() was invoked through
t_end() -> wcs_timer_start() -> jnl_qio_start() -> jnl_sub_qio_start(). But before
t_end() invokes wcs_timer_start(), it does a REVERT which now causes a check of
whether suspend() was deferred (due to #215 code changes) and so we suspend the
process right there (because at this point, we have released crit and are out
of the phase2 commit logic). And so the process does not ever reach wcs_timer_start()
and never gets suspended while holding the jnl buffer qio lock like the test expects
causing the test to eventually fail (because no JNLPROCSTUCK message is later seen).

To work around this, the sequence of commands done by the test have now been simplified
to a SET ^a=1 followed by a VIEW "JNLWAIT". The latter invokes jnl_wait() which call
jnl_sub_qio_start() while we do not hold crit and so the process suspends right there
recreating the same situation that the test wants for a later JNLPROCSTUCK message.
@ksbhaskar ksbhaskar changed the title Ctrl-Z of YottaDB executables (mumps/mupip/dse/lke etc.) is ignored in some cases Ctrl-Z (the suspend signal) is honored in all cases May 8, 2018
nars1 added a commit to nars1/YottaDB that referenced this issue May 11, 2018
…d in prior YottaDB#215 commit

As part of a prior YottaDB#215 commit, the DEFERRED_SIGNAL_HANDLING_CHECK macro was enhanced
to check for Ctrl-Z deferral. Turns out that change caused libyottadb.so size to bloat
from 5.00Mb (in r1.20) to 5.25Mb. This is because this macro is used in heavyweight
macros like REVERT which meant all the code inside this macro gets duplicated across
various parts of the compiled .o files thereby bloating the size of libyottadb.so (which
is just an aggregate of all the individual .o files).

To fix this size bloat, the macro was made minimalistic so it does a simple if check of
a global variable and if that is non-zero, in turn invoke a function that does most of
the code that was previously included in this macro. Thereby we centralize the code in
one place instead of inlining it everywhere the macro is used.

But we also do not want to slow down every macro invocation in the common case (which
is that we do not have any signals deferred). This meant that we cannot fold all the
logic in the macro into the function (as a function call is expensive and we don't want
to do it in every invocation of this macro).

The macro did 3 checks of deferred events
	a) forced_exit
	b) timers
	c) ctrl-z

As part of this commit, all the different checks now update a new global variable
"deferred_signal_handling_needed". It is a bitmask that has one bit set for each type
of deferred event (a, b or c above). So the macro can just do a check of this global
variable and if it is non-zero, invoke a new function "handle_deferred_signal()".

To achieve that, the following was done to the above 3 deferred event scenarios.

a) forced_exit was a multi-state variable so it was not easily replaced by the new bit.
   Therefore "deferred_signal_handling_needed & DEFERRED_SIGNAL_HANDLING_EXIT" was
   set to 1 if ever forced_exit was set to 1 and cleared if forced_exit was 0 or 2.
   And the two were maintained in sync.

b) deferred_timers_check_needed was a boolean variable so it was easily replaced by
   the new bit "deferred_signal_handling_needed & DEFERRED_SIGNAL_HANDLING_TIMERS".

c) suspend_status was a tri-state variable so it was not easily replaced by
   the new bit. Instead it was split into two new variables. One is the new bit
   "deferred_signal_handling_needed & DEFERRED_SIGNAL_HANDLING_CTRLZ" which indicates
   whether a ctrlz event got deferred or not. Another is a new global "is_suspended"
   which is TRUE if the process is currently suspended. This took case of the NOW_SUSPEND
   scenario from before.

With the above changes, the size of libyottadb.so came down from 5.25Mb down to 4.68Mb
which is better than even the 5.00Mb size in r1.20.

While at this, a use of the DEFERRED_SIGNAL_HANDLING_CHECK macro in t_end.c and tp_tend.c
is now removed since it occurs right after a REVERT which has the same check anyways
(REVERT -> ENABLE_INTERRUPTS -> DEFERRED_SIGNAL_HANDLING_CHECK_TRIMMED).
nars1 added a commit that referenced this issue May 11, 2018
…ior #215 commit

As part of a prior #215 commit, the DEFERRED_SIGNAL_HANDLING_CHECK macro was enhanced
to check for Ctrl-Z deferral. Turns out that change caused libyottadb.so size to bloat
from 5.00Mb (in r1.20) to 5.25Mb. This is because this macro is used in heavyweight
macros like REVERT which meant all the code inside this macro gets duplicated across
various parts of the compiled .o files thereby bloating the size of libyottadb.so (which
is just an aggregate of all the individual .o files).

To fix this size bloat, the macro was made minimalistic so it does a simple if check of
a global variable and if that is non-zero, in turn invoke a function that does most of
the code that was previously included in this macro. Thereby we centralize the code in
one place instead of inlining it everywhere the macro is used.

But we also do not want to slow down every macro invocation in the common case (which
is that we do not have any signals deferred). This meant that we cannot fold all the
logic in the macro into the function (as a function call is expensive and we don't want
to do it in every invocation of this macro).

The macro did 3 checks of deferred events
	a) forced_exit
	b) timers
	c) ctrl-z

As part of this commit, all the different checks now update a new global variable
"deferred_signal_handling_needed". It is a bitmask that has one bit set for each type
of deferred event (a, b or c above). So the macro can just do a check of this global
variable and if it is non-zero, invoke a new function "handle_deferred_signal()".

To achieve that, the following was done to the above 3 deferred event scenarios.

a) forced_exit was a multi-state variable so it was not easily replaced by the new bit.
   Therefore "deferred_signal_handling_needed & DEFERRED_SIGNAL_HANDLING_EXIT" was
   set to 1 if ever forced_exit was set to 1 and cleared if forced_exit was 0 or 2.
   And the two were maintained in sync.

b) deferred_timers_check_needed was a boolean variable so it was easily replaced by
   the new bit "deferred_signal_handling_needed & DEFERRED_SIGNAL_HANDLING_TIMERS".

c) suspend_status was a tri-state variable so it was not easily replaced by
   the new bit. Instead it was split into two new variables. One is the new bit
   "deferred_signal_handling_needed & DEFERRED_SIGNAL_HANDLING_CTRLZ" which indicates
   whether a ctrlz event got deferred or not. Another is a new global "is_suspended"
   which is TRUE if the process is currently suspended. This took case of the NOW_SUSPEND
   scenario from before.

With the above changes, the size of libyottadb.so came down from 5.25Mb down to 4.68Mb
which is better than even the 5.00Mb size in r1.20.

While at this, a use of the DEFERRED_SIGNAL_HANDLING_CHECK macro in t_end.c and tp_tend.c
is now removed since it occurs right after a REVERT which has the same check anyways
(REVERT -> ENABLE_INTERRUPTS -> DEFERRED_SIGNAL_HANDLING_CHECK_TRIMMED).
nars1 added a commit to nars1/YottaDB that referenced this issue May 12, 2018
* An "assert(deferred_signal_handling_needed);" in sr_port/handle_deferred_signal.c was
  removed since it can cause test failures in some cases (timer pops etc.).

* sr_unix/deferred_signal_handler.c was renamed to sr_unix/deferred_exit_handler.c
  since this function is invoked when we need to handle deferred signals that initiate
  exit processing. Likewise, sr_unix/deferred_signal_handler.h was renamed to
  sr_unix/deferred_exit_handler.h.

* Similar to the previous bullet, sr_port/dbcertify_deferred_signal_handler.c was renamed
  to sr_port/dbcertify_deferred_exit_handler.c.

* sr_port/handle_defered_signal.c was renamed to sr_port/deferred_signal_handler.c since
  this routine handled deferred signals that do not terminate the process (distinction
  from the newly renamed deferred_exit_handler.c where the deferred signals do terminate
  the process). And the function handle_deferred_signal() (which had a slightly different
  name than the module handle_defered_signal.c) was renamed to deferred_signal_handler().

* The type of "deferred_signal_handling_needed" was changed from boolean_t to uint4.
nars1 added a commit that referenced this issue May 12, 2018
* An "assert(deferred_signal_handling_needed);" in sr_port/handle_deferred_signal.c was
  removed since it can cause test failures in some cases (timer pops etc.).

* sr_unix/deferred_signal_handler.c was renamed to sr_unix/deferred_exit_handler.c
  since this function is invoked when we need to handle deferred signals that initiate
  exit processing. Likewise, sr_unix/deferred_signal_handler.h was renamed to
  sr_unix/deferred_exit_handler.h.

* Similar to the previous bullet, sr_port/dbcertify_deferred_signal_handler.c was renamed
  to sr_port/dbcertify_deferred_exit_handler.c.

* sr_port/handle_defered_signal.c was renamed to sr_port/deferred_signal_handler.c since
  this routine handled deferred signals that do not terminate the process (distinction
  from the newly renamed deferred_exit_handler.c where the deferred signals do terminate
  the process). And the function handle_deferred_signal() (which had a slightly different
  name than the module handle_defered_signal.c) was renamed to deferred_signal_handler().

* The type of "deferred_signal_handling_needed" was changed from boolean_t to uint4.
nars1 added a commit to nars1/YottaDB that referenced this issue Jul 23, 2018
…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).
nars1 added a commit that referenced this issue Jul 23, 2018
…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 #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 (#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 #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 #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 #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 #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 #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. #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
  #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 #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 #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).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

1 participant