NJS: loader should be registered using njs_vm_set_module_loader()

[andrey-zelenkov]

This change was introduced in NJS 0.8.3 here:
https://hg.nginx.com/njs/rev/ad1a7ad3c715

[hongzhidao]

It seems there is unused code left and sorry that it affected your job.

diff --git a/src/nxt_script.c b/src/nxt_script.c
index 70045a22..c77e5ee5 100644
--- a/src/nxt_script.c
+++ b/src/nxt_script.c
@@ -40,7 +40,7 @@ static nxt_lvlhsh_t  nxt_script_info;
 static njs_vm_ops_t  nxt_js_ops = {
     NULL,
     NULL,
-    nxt_js_module_loader,
+    NULL,
     NULL,
 };

nxt_js_module_loader() is unused in nxt_script.c which is for uploading js module.

[andrey-zelenkov]

Rebased and removed njs_vm_set_module_loader() call from src/nxt_script.c:

% git range-diff e3edba2a...5c00593
-:  -------- > 1:  c3af21e9 Docker: Switch to github
-:  -------- > 2:  baff936b Packages: Move dist target to git archive
-:  -------- > 3:  34b3a812 Add nxt_file_chown()
-:  -------- > 4:  b500c36d Allow to set the permissions of the Unix domain control socket
-:  -------- > 5:  2bd3b418 Docs: Update man page for new --control-* options
-:  -------- > 6:  1dca8602 Tools: disambiguate unitc control socket detection
1:  e3edba2a ! 7:  5c005939 NJS: loader should be registered using njs_vm_set_module_loader()
    @@ src/nxt_script.c: static void nxt_script_buf_completion(nxt_task_t *task, void *
      nxt_script_t *
      nxt_script_new(nxt_task_t *task, nxt_str_t *name, u_char *data, size_t size,
          u_char *error)
    - {
    --    u_char        *start;
    --    njs_vm_t      *vm;
    --    njs_str_t     mod_name;
    --    njs_mod_t     *mod;
    --    njs_vm_opt_t  opts;
    --    nxt_script_t  *script;
    -+    u_char         *start;
    -+    njs_vm_t       *vm;
    -+    nxt_mp_t       *mp;
    -+    njs_str_t      mod_name;
    -+    njs_mod_t      *mod;
    -+    njs_vm_opt_t   opts;
    -+    nxt_script_t   *script;
    -+    nxt_js_conf_t  *jcf;
    - 
    -     njs_vm_opt_init(&opts);
    - 
     @@ src/nxt_script.c: nxt_script_new(nxt_task_t *task, nxt_str_t *name, u_char *data, size_t size,
          opts.file.start = (u_char *) "default";
          opts.file.length = 7;
    @@ src/nxt_script.c: nxt_script_new(nxt_task_t *task, nxt_str_t *name, u_char *data
          vm = njs_vm_create(&opts);
          if (nxt_slow_path(vm == NULL)) {
              return NULL;
    -     }
    - 
    -+    mp = nxt_mp_create(1024, 128, 256, 32);
    -+    if (nxt_slow_path(mp == NULL)) {
    -+        njs_vm_destroy(vm);
    -+        return NULL;
    -+    }
    -+
    -+    jcf = nxt_js_conf_new(mp, 0);
    -+    if (nxt_slow_path(jcf == NULL)) {
    -+        goto fail;
    -+    }
    -+
    -+    njs_vm_set_module_loader(vm, nxt_js_module_loader, jcf);
    -+
    -     mod_name.length = name->length;
    -     mod_name.start = name->start;
    - 
    -@@ src/nxt_script.c: nxt_script_new(nxt_task_t *task, nxt_str_t *name, u_char *data, size_t size,
    - 
    -     nxt_memcpy(script->text.start, data, size);
    - 
    -+    nxt_mp_destroy(mp);
    -     njs_vm_destroy(vm);
    - 
    -     return script;
    - 
    - fail:
    - 
    -+    nxt_mp_destroy(mp);
    -     njs_vm_destroy(vm);
    - 
    -     return NULL;

[hongzhidao]

Add a check here.

if (nxt_fast_path(vm != NULL)) {
    njs_vm_set_module_loader(vm, nxt_js_module_loader, jcf);
}

Others look good.

[hongzhidao]

Hi Andrei, take a look again.

[andrey-zelenkov]

Rebased and added NULL check suggested by @hongzhidao:

% git range-diff 5c005939...324c2282
...
 1:  5c005939 ! 57:  324c2282 NJS: loader should be registered using njs_vm_set_module_loader()
    @@ src/nxt_js.c: nxt_js_vm_create(nxt_js_conf_t *jcf)
     -    return njs_vm_create(&opts);
     +    vm = njs_vm_create(&opts);
     +
    -+    njs_vm_set_module_loader(vm, nxt_js_module_loader, jcf);
    ++    if (nxt_fast_path(vm != NULL)) {
    ++        njs_vm_set_module_loader(vm, nxt_js_module_loader, jcf);
    ++    }
     +
     +    return vm;
      }

[andrey-zelenkov]

Configure version check bumped:

% git range-diff 324c2282...9055ff68
1:  324c2282 ! 1:  9055ff68 NJS: loader should be registered using njs_vm_set_module_loader()
    @@ Metadata
      ## Commit message ##
         NJS: loader should be registered using njs_vm_set_module_loader()
     
    +    This change makes NJS module incompatible with NJS older than 0.8.3.
    +    Therefore, the configuration version check has been adjusted accordingly.
    +
         This change was introduced in NJS 0.8.3 here:
         <https://hg.nginx.com/njs/rev/ad1a7ad3c715>
     
    + ## auto/njs ##
    +@@ auto/njs: nxt_feature_incs="$NXT_NJS_CFLAGS $NXT_NJS_AUX_CFLAGS"
    + nxt_feature_libs="$NXT_NJS_LIBS $NXT_NJS_AUX_LIBS"
    + nxt_feature_test="#include <njs.h>
    + 
    +-                  #if NJS_VERSION_NUMBER < 0x000800
    +-                  # error NJS < 0.8.0 is not supported.
    ++                  #if NJS_VERSION_NUMBER < 0x000803
    ++                  # error NJS < 0.8.3 is not supported.
    +                   #endif
    + 
    +                   int main(void) {
    +@@ auto/njs: nxt_feature_test="#include <njs.h>
    + 
    + if [ $nxt_found = no ]; then
    +     $echo
    +-    $echo $0: error: no NJS library \>= 0.8.0 found.
    ++    $echo $0: error: no NJS library \>= 0.8.3 found.
    +     $echo
    +     exit 1;
    + fi
    +
      ## src/nxt_js.c ##
     @@ src/nxt_js.c: nxt_js_module_loader(njs_vm_t *vm, njs_external_ptr_t external, njs_str_t *name)
      }

[andrey-zelenkov]

Rebase and bump NJS version

% git range-diff 9055ff68...3aec4bd5
 -:  -------- >  1:  4d25c612 Edited changes.xml for the 1.32.0 release
 -:  -------- >  2:  ace553dc Generated Dockerfiles for Unit 1.32.0
 -:  -------- >  3:  08811700 Added version 1.32.0 CHANGES
 -:  -------- >  4:  e67d7433 Version bump
 -:  -------- >  5:  23e807de Wasm-wc: use more common uname switch to get operating system name
 -:  -------- >  6:  8ff606fb Configuration: Fix check in nxt_conf_json_parse_value()
 -:  -------- >  7:  4eb008bb Remove unused nxt_vector_t API
 -:  -------- >  8:  353d2d05 Var: Remove a dead assignment in nxt_var_interpreter()
 -:  -------- >  9:  c2f7f296 Avoid potential NULL pointer dereference in nxt_router_temp_conf()
 -:  -------- > 10:  8032ce31 Test with root access in GitHub workflows
 -:  -------- > 11:  0cee7d1a Add GitHub workflow for wasm-wasi-component
 -:  -------- > 12:  63bc3882 .mailmap: Map Dylan's 2nd GitHub address
 -:  -------- > 13:  f6899af6 Var: Fix cacheable issue for njs variable access
 -:  -------- > 14:  5511593d Remove support for Microsoft's Visual C++ compiler
 -:  -------- > 15:  0c2d7786 Remove support for Intel's icc compiler
 -:  -------- > 16:  e79e4635 Remove support for IBM's XL C compiler
 -:  -------- > 17:  9cd11133 Remove support for Sun's Sun Studio/SunPro C compiler
 -:  -------- > 18:  806e209d Remove -W from compiler flags
 -:  -------- > 19:  1dcb5383 Expand the comment about -Wstrict-overflow on GCC
 -:  -------- > 20:  0b5223e1 Disable strict-aliasing in clang by default
 -:  -------- > 21:  c1e3f02f Compile with -fno-strict-overflow
 -:  -------- > 22:  280a978d Add initial infrastructure for pretty printing make output
 -:  -------- > 23:  5d831af0 Hook up make pretty printing to the Unit core and tests
 -:  -------- > 24:  da335bec Pretty print the Java language module compiler output
 -:  -------- > 25:  574528f7 Pretty print the Perl language module compiler output
 -:  -------- > 26:  0a0dcf91 Pretty print the PHP language module compiler output
 -:  -------- > 27:  caaa1d28 Pretty print the Python language module compiler output
 -:  -------- > 28:  133f75fd Pretty print the Ruby language module compiler output
 -:  -------- > 29:  b763ba7e Pretty print the wasm language module compiler output
 -:  -------- > 30:  15072fbd Enable optional 'debuggable' builds
 -:  -------- > 31:  d23812b8 Allow to disable -Werror at 'make' time
 -:  -------- > 32:  f55fa70c Add a help target to the root Makefile
 -:  -------- > 33:  a171b399 Add an EXTRA_CFLAGS make variable
 -:  -------- > 34:  6b138571 Wasm-wc: Bump the mio crate from 0.8.10 to 0.8.11
 -:  -------- > 35:  2e615250 Add dependabot.yml
 1:  9055ff68 ! 36:  3aec4bd5 NJS: loader should be registered using njs_vm_set_module_loader()
    @@ auto/njs: nxt_feature_test="#include <njs.h>
          exit 1;
      fi
     
    + ## docs/changes.xml ##
    +@@ docs/changes.xml: NGINX Unit updated to 1.33.0.
    +          date="" time=""
    +          packager="Nginx Packaging &lt;nginx-packaging@f5.com&gt;">
    + 
    ++<change type="change">
    ++<para>
    ++if building with NJS, version 0.8.3 or later is now required.
    ++</para>
    ++</change>
    ++
    + </changes>
    + 
    + 
    +
    + ## pkg/contrib/src/njs/SHA512SUMS ##
    +@@
    +-cc3110a0c6866dfc03d19c58745e5b75aa9792999db45bc55a752f7b04db8ae51322bfe0156b873109c8477c6c1a030c851c770697cf6791c6e89fb2fed0a2c5  njs-0.8.2.tar.gz
    ++1cec9a322c40aa2b4ec6eb5bea78d7442880b0cff3a41ad171a3dc3157a6990baec6c8b9eda99ee02a9e51c0b933f13ef17431079a5ff409aaf84b912c7f4df7  njs-0.8.3.tar.gz
    +
    + ## pkg/contrib/src/njs/version ##
    +@@
    +-NJS_VERSION := 0.8.2
    ++NJS_VERSION := 0.8.3
    +
      ## src/nxt_js.c ##
     @@ src/nxt_js.c: nxt_js_module_loader(njs_vm_t *vm, njs_external_ptr_t external, njs_str_t *name)
      }

[hongzhidao]

LGTM.