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

[#318] Create filter_commands.tab at most once while scanning multiple lines of restrict.txt #332

Merged
merged 1 commit into from
Aug 2, 2018

Conversation

nars1
Copy link
Collaborator

@nars1 nars1 commented Aug 2, 2018

We had a v63005/gtm8877 subtest failure where line 50 below issued a COMMFILTERERR error.

gtm8877.csh
-----------
     42 # FILTER RETURNING A STRING
     43 rm $ydb_dist/restrict.txt
     44 cat > $ydb_dist/restrict.txt << EOF
     45 ZSYSTEM_FILTER:filterfn2^gtm8877
     46 PIPE_FILTER:filterfn2^gtm8877
     47 EOF
     48 chmod -w $ydb_dist/restrict.txt
     49 echo "# FILTER RETURNING A STRING"
     50 $ydb_dist/mumps -run zsystemfn^gtm8877
     51 $ydb_dist/mumps -run pipeopenfn^gtm8877
     52 echo ""

Expected output
---------------
filtered output
filtered output

Actual output
-------------
%YDB-E-COMMFILTERERR, Error executing the command filter for gtmzsystem. 150379666,(Call-In),%YDB-E-CINOENTRY, No entry specified for gtmzsystem in the call-in table
filtered output

We clearly added ZSYSTEM_FILTER just before PIPE_FILTER in line 45 of gtm8877.csh above but that is
found missing when we actually run it.

This test passed almost always so this was some edge case.

Looking at the code, for each line in restrict.txt, restrict_init() checks if ZSYSTEM_FILTER or PIPE_FILTER
is defined and if so invokes the following macro.

restrict.c
----------
  69 #define PUT_FLNAME_IN_MAPPING_FILE(RPATH, FPATH, FP, C_CALL_NAME, M_REF_NAME, STAT_RM, SAVE_ERRNO, ERR_STR)     \
  70 {                                                                                                               \
  71         if (!ACCESS(FPATH, F_OK))               /* Filter file exists, now check modified time */               \
  72         {                                                                                                       \
  73                 Stat(RPATH, &rTime);                                                                            \
  74                 rmtime = rTime.st_mtim;                                                                         \
  75                 Stat(FPATH, &fTime);                                                                            \
  76                 fmtime = fTime.st_mtim;                                                                         \
  77                 /* Check if restrict.txt file modification time (rmtime) is newer than                          \
  78                  * filter_commands.tab file modification time (fmtime). If so, recreate filter_commands.tab.    \
  79                  */                                                                                             \
  80                 if ((rmtime.tv_sec > fmtime.tv_sec)                                                             \
  81                         || ((rmtime.tv_sec == fmtime.tv_sec) && (rmtime.tv_nsec >= fmtime.tv_nsec)))            \
  82                 {       /* Delete the older mapping file and recreate new if required */                        \
  83                         created_now = TRUE;                                                                     \
  84                         gtm_file_remove(FPATH, strlen(FPATH), &STAT_RM);                                        \
  85                         append_filter(FPATH, FP, C_CALL_NAME, M_REF_NAME, SAVE_ERRNO, ERR_STR);                 \
  86                 } else if (created_now) /* This process created a new file,append to it */                      \
  87                 {                                                                                               \
  88                         append_filter(FPATH, FP, C_CALL_NAME, M_REF_NAME, SAVE_ERRNO, ERR_STR);                 \
  89                 }                                                                                               \

The macro checks if the file filter_commands.tab exists and if so compares the file modification
time of that against restrict.txt. If the comparison shows restrict.txt is newer, it recreates
filter_commands.tab. Otherwise, it checks if we had created the file now ("created_now"
variable is TRUE) and if so appends the current filter line (ZSYSTEM_FILTER or PIPE_FILTER)
onto filter_commands.tab. And otherwise, it does nothing because the two are supposedly in sync.

And that makes the following scenario (and the test failure) possible.

  1. PUT_FLNAME_IN_MAPPING_FILE macro is called for ZSYSTEM_FILTER line in restrict.txt. It decides
    filter_commands.tab needs to be created and adds a line for ZSYSTEM_FILTER.
  2. PUT_FLNAME_IN_MAPPING_FILE macro is called for PIPE_FILTER line in restrict.txt. And it finds
    that the nanosecond-level timestamp matches between restrict.txt and filter_commands.tab.
    Therefore decides to recreate the file again while still processing the same restrict.txt.
    This means the line added to filter_commands.tab in Step 1 is lost.

The check at line 86 is done after the check at line 80 whereas it should be done before. If we
created this file because of a prior call to PUT_FLNAME_IN_MAPPING_FILE macro, we should append
to it unconditionally. Otherwise we should do the file modification check. This is now fixed.

In addition, once the PUT_FLNAME_IN_MAPPING_FILE macro is called once in restrict_init(),
there is no need to do any more system calls (ACCESS/access or Stat/stat calls) as we know
whether we need to append to a newly created file ("created_now" variable is TRUE) OR we need
to do nothing (because the first invocation of the macro found filter_commands.tab is newer
than restrict.txt. Whereas the current code makes unnecessary access() and stat() calls. That
is also fixed.

While at this, the macro has been enhanced to pass the "created_now" variable across instead
of silently using/modifying it. A new variable "created_now_initialized" indicates whether
"created_now" is usable or not (i.e. the PUT_FLNAME_IN_MAPPING_FILE macro has been called
once). And that is also passed to the macro.

…multiple lines of restrict.txt

We had a v63005/gtm8877 subtest failure where line 50 below issued a COMMFILTERERR error.

gtm8877.csh
-----------
     42 # FILTER RETURNING A STRING
     43 rm $ydb_dist/restrict.txt
     44 cat > $ydb_dist/restrict.txt << EOF
     45 ZSYSTEM_FILTER:filterfn2^gtm8877
     46 PIPE_FILTER:filterfn2^gtm8877
     47 EOF
     48 chmod -w $ydb_dist/restrict.txt
     49 echo "# FILTER RETURNING A STRING"
     50 $ydb_dist/mumps -run zsystemfn^gtm8877
     51 $ydb_dist/mumps -run pipeopenfn^gtm8877
     52 echo ""

Expected output
---------------
filtered output
filtered output

Actual output
-------------
%YDB-E-COMMFILTERERR, Error executing the command filter for gtmzsystem. 150379666,(Call-In),%YDB-E-CINOENTRY, No entry specified for gtmzsystem in the call-in table
filtered output

We clearly added ZSYSTEM_FILTER just before PIPE_FILTER in line 45 of gtm8877.csh above but that is
found missing when we actually run it.

This test passed almost always so this was some edge case.

Looking at the code, for each line in restrict.txt, restrict_init() checks if ZSYSTEM_FILTER or PIPE_FILTER
 is defined and if so invokes the following macro.

restrict.c
----------
  69 #define PUT_FLNAME_IN_MAPPING_FILE(RPATH, FPATH, FP, C_CALL_NAME, M_REF_NAME, STAT_RM, SAVE_ERRNO, ERR_STR)     \
  70 {                                                                                                               \
  71         if (!ACCESS(FPATH, F_OK))               /* Filter file exists, now check modified time */               \
  72         {                                                                                                       \
  73                 Stat(RPATH, &rTime);                                                                            \
  74                 rmtime = rTime.st_mtim;                                                                         \
  75                 Stat(FPATH, &fTime);                                                                            \
  76                 fmtime = fTime.st_mtim;                                                                         \
  77                 /* Check if restrict.txt file modification time (rmtime) is newer than                          \
  78                  * filter_commands.tab file modification time (fmtime). If so, recreate filter_commands.tab.    \
  79                  */                                                                                             \
  80                 if ((rmtime.tv_sec > fmtime.tv_sec)                                                             \
  81                         || ((rmtime.tv_sec == fmtime.tv_sec) && (rmtime.tv_nsec >= fmtime.tv_nsec)))            \
  82                 {       /* Delete the older mapping file and recreate new if required */                        \
  83                         created_now = TRUE;                                                                     \
  84                         gtm_file_remove(FPATH, strlen(FPATH), &STAT_RM);                                        \
  85                         append_filter(FPATH, FP, C_CALL_NAME, M_REF_NAME, SAVE_ERRNO, ERR_STR);                 \
  86                 } else if (created_now) /* This process created a new file,append to it */                      \
  87                 {                                                                                               \
  88                         append_filter(FPATH, FP, C_CALL_NAME, M_REF_NAME, SAVE_ERRNO, ERR_STR);                 \
  89                 }                                                                                               \

The macro checks if the file filter_commands.tab exists and if so compares the file modification
time of that against restrict.txt. If the comparison shows restrict.txt is newer, it recreates
filter_commands.tab. Otherwise, it checks if we had created the file now ("created_now"
variable is TRUE) and if so appends the current filter line (ZSYSTEM_FILTER or PIPE_FILTER)
onto filter_commands.tab.  And otherwise, it does nothing because the two are supposedly in sync.

And that makes the following scenario (and the test failure) possible.

1) PUT_FLNAME_IN_MAPPING_FILE macro is called for ZSYSTEM_FILTER line in restrict.txt. It decides
   filter_commands.tab needs to be created and adds a line for ZSYSTEM_FILTER.
2) PUT_FLNAME_IN_MAPPING_FILE macro is called for PIPE_FILTER line in restrict.txt. And it finds
   that the nanosecond-level timestamp matches between restrict.txt and filter_commands.tab.
   Therefore decides to recreate the file again while still processing the same restrict.txt.
   This means the line added to filter_commands.tab in Step 1 is lost.

The check at line 86 is done after the check at line 80 whereas it should be done before. If we
created this file because of a prior call to PUT_FLNAME_IN_MAPPING_FILE macro, we should append
to it unconditionally. Otherwise we should do the file modification check. This is now fixed.

In addition, once the PUT_FLNAME_IN_MAPPING_FILE macro is called once in restrict_init(),
there is no need to do any more system calls (ACCESS/access or Stat/stat calls) as we know
whether we need to append to a newly created file ("created_now" variable is TRUE) OR we need
to do nothing (because the first invocation of the macro found filter_commands.tab is newer
than restrict.txt. Whereas the current code makes unnecessary access() and stat() calls. That
is also fixed.

While at this, the macro has been enhanced to pass the "created_now" variable across instead
of silently using/modifying it. A new variable "created_now_initialized" indicates whether
"created_now" is usable or not (i.e. the PUT_FLNAME_IN_MAPPING_FILE macro has been called
once). And that is also passed to the macro.
@nars1 nars1 self-assigned this Aug 2, 2018
@nars1 nars1 requested a review from estess August 2, 2018 10:40
@nars1 nars1 merged commit 04efee2 into YottaDB:master Aug 2, 2018
@nars1 nars1 deleted the ydb318 branch August 2, 2018 14:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants