modules/content: simplify content.mmap-add RPC

Problem: the content.mmap-add RPC includes parameters that
are no longer needed given its reduced role in supporting archives.

Drop 'disable_mmap'.
Drop 'small_file_threshold'.
Drop 'fullpath' and drop the fullpath parameter from fileref_create_ex()
Accept only one tag, not an array of them.

Fail the mmap request if the path is not fully qualified or not a
regular file.

Change content.mmap-remove to only accept one tag also.
Update flux-archive.

Author: garlick

src/cmd/builtin/archive.c

@@ -72,28 +72,25 @@ struct create_ctx {
  */
 static void mmap_fileref_data (struct create_ctx *ctx, const char *path)
 {
+    char *fullpath;
     flux_future_t *f;
-    char *fpath;
 
     // relative path is preserved in the archive, but broker needs full path
-    if (!(fpath = realpath (path, NULL)))
+    if (!(fullpath = realpath (path, NULL)))
         log_err_exit ("%s", path);
 
     if (!(f = flux_rpc_pack (ctx->h,
                              "content.mmap-add",
                              0,
                              0,
-                             "{s:s s:s s:b s:i s:i s:[s]}",
-                             "path", path,
-                             "fullpath", fpath,
-                             "disable_mmap", 0,
-                             "threshold", 0, // disable small file handling
+                             "{s:s s:i s:s}",
+                             "path", fullpath,
                              "chunksize", ctx->param.chunksize,
-                             "tags", ctx->name))
+                             "tag", ctx->name))
         || flux_rpc_get (f, NULL))
         log_msg_exit ("%s: %s", path, future_strerror (f, errno));
     flux_future_destroy (f);
-    free (fpath);
+    free (fullpath);
 }
 
 /* Store the blobs of an RFC 37 blobvec-encoded fileref to the content store.
@@ -168,7 +165,6 @@ static void add_archive_file (struct create_ctx *ctx, const char *path)
     json_t *fileref;
 
     if (!(fileref = fileref_create_ex (path,
-                                       NULL,
                                        &ctx->param,
                                        &mapinfo,
                                        &error)))
@@ -394,8 +390,8 @@ static void unmap_archive (flux_t *h, const char *name)
                              "content.mmap-remove",
                              0,
                              0,
-                             "{s:[s]}",
-                             "tags", name))
+                             "{s:s}",
+                             "tag", name))
         || flux_rpc_get (f, NULL) < 0) {
         log_msg ("unmap %s: %s", name, future_strerror (f, errno));
     }

src/common/libfilemap/fileref.c

@@ -324,7 +324,6 @@ static json_t *fileref_create_symlink (const char *path,
 }
 
 json_t *fileref_create_ex (const char *path,
-                           const char *fullpath,
                            struct blobvec_param *param,
                            struct blobvec_mapinfo *mapinfop,
                            flux_error_t *error)
@@ -344,8 +343,6 @@ json_t *fileref_create_ex (const char *path,
             goto inval;
         }
     }
-    if (!fullpath)
-        fullpath = path;
     /* Store a relative path in the object so that extraction can specify a
      * destination directory, like tar(1) default behavior.
      */
@@ -359,8 +356,8 @@ json_t *fileref_create_ex (const char *path,
      * Avoid open(2) blocking on a FIFO with O_NONBLOCK, but restore blocking
      * behavior after open(2) succeeds.
      */
-    if ((fd = open (fullpath, O_RDONLY | O_NOFOLLOW | O_NONBLOCK)) < 0) {
-        if (errno != ELOOP || lstat (fullpath, &sb) < 0) {
+    if ((fd = open (path, O_RDONLY | O_NOFOLLOW | O_NONBLOCK)) < 0) {
+        if (errno != ELOOP || lstat (path, &sb) < 0) {
             errprintf (error, "%s: %s", path, strerror (errno));
             goto error;
         }
@@ -419,7 +416,7 @@ json_t *fileref_create_ex (const char *path,
     /* symlink
      */
     else if (S_ISLNK (sb.st_mode)) {
-        if (!(o = fileref_create_symlink (relative_path, fullpath, &sb, error)))
+        if (!(o = fileref_create_symlink (relative_path, path, &sb, error)))
             goto error;
     }
     /* directory
@@ -454,7 +451,7 @@ json_t *fileref_create_ex (const char *path,
 
 json_t *fileref_create (const char *path, flux_error_t *error)
 {
-    return fileref_create_ex (path, NULL, NULL, NULL, error);
+    return fileref_create_ex (path, NULL, NULL, error);
 }
 
 void fileref_pretty_print (json_t *fileref,

src/common/libfilemap/fileref.h

@@ -30,15 +30,13 @@ struct blobvec_mapinfo {
 
 
 /* Variant of fileref_create() with extra parameters to allow for 'blobvec'
- * encoding.  This is intended to be called from the broker.
- * - If non-NULL 'fullpath' is used in place of 'path' for open/stat/mmap.
+ * encoding.
  * - If 'param' is non-NULL, blobvec encoding is enabled with the specified
  *   params.
  * - If 'mapinfo' is non-NULL, and the file meets conditions for blobvec
  *   encoding, the file remains mapped in memory and its address is returned.
  */
 json_t *fileref_create_ex (const char *path,
-                           const char *fullpath,
                            struct blobvec_param *param,
                            struct blobvec_mapinfo *mapinfo,
                            flux_error_t *error);

src/common/libfilemap/test/fileref.c

@@ -45,7 +45,7 @@ static json_t *xfileref_create_vec (const char *path,
         .small_file_threshold = 4096,
     };
 
-    o = fileref_create_ex (path, NULL, &blobvec_param, NULL, &error);
+    o = fileref_create_ex (path, &blobvec_param, NULL, &error);
     if (!o)
         diag ("%s", error.text);
     return o;
@@ -650,7 +650,7 @@ void test_expfail (void)
     param.chunksize = 1024;
     param.hashtype = "smurfette";
     param.small_file_threshold = 0;
-    o = fileref_create_ex (mkpath ("test"), NULL, &param, NULL, &error);
+    o = fileref_create_ex (mkpath ("test"), &param, NULL, &error);
     if (!o)
         diag ("%s", error.text);
     ok (o == NULL && errno == EINVAL,
@@ -660,7 +660,7 @@ void test_expfail (void)
     param.chunksize = -1;
     param.hashtype = "sha1";
     param.small_file_threshold = 0;
-    o = fileref_create_ex (mkpath ("test"), NULL, &param, NULL, &error);
+    o = fileref_create_ex (mkpath ("test"), &param, NULL, &error);
     if (!o)
         diag ("%s", error.text);
     ok (o == NULL && errno == EINVAL,
@@ -670,7 +670,7 @@ void test_expfail (void)
     param.chunksize = 1024;
     param.hashtype = "sha1";
     param.small_file_threshold = -1;
-    o = fileref_create_ex (mkpath ("test"), NULL, &param, NULL, &error);
+    o = fileref_create_ex (mkpath ("test"), &param, NULL, &error);
     if (!o)
         diag ("%s", error.text);
     ok (o == NULL && errno == EINVAL,

src/modules/content/mmap.c

@@ -380,10 +380,14 @@ struct content_region *content_mmap_region_lookup (struct content_mmap *mm,
 static struct content_region *content_mmap_region_create (
                                                  struct content_mmap *mm,
                                                  const char *path,
-                                                 const char *fpath,
-                                                 struct blobvec_param *param,
+                                                 int chunksize,
                                                  flux_error_t *error)
 {
+    struct blobvec_param param = {
+        .hashtype = mm->hash_name,
+        .chunksize = chunksize,
+        .small_file_threshold = 0, // always choose blobvec encoding here
+    };
     struct content_region *reg;
 
     if (!(reg = calloc (1, sizeof (*reg)))) {
@@ -393,15 +397,22 @@ static struct content_region *content_mmap_region_create (
     reg->refcount = 1;
     reg->mm = mm;
     reg->mapinfo.base = MAP_FAILED;
-    if (!(reg->fullpath = strdup (fpath)))
+    if (!(reg->fullpath = strdup (path)))
         goto error;
 
     if (!(reg->fileref = fileref_create_ex (path,
-                                            fpath,
-                                            param,
+                                            &param,
                                             &reg->mapinfo,
                                             error)))
         goto error;
+    /* fileref_create_ex() accepts all file types, but flux-archive(1) should
+     * not be requesting that files be mapped which do not meet criteria.
+     */
+    if (reg->mapinfo.base == MAP_FAILED) {
+        errprintf (error, "%s: not suitable for mapping", path);
+        errno = EINVAL;
+        goto error;
+    }
     if (region_cache_add (reg) < 0) {
         errprintf (error,
                    "%s: error caching region blobrefs: %s",
@@ -415,70 +426,41 @@ static struct content_region *content_mmap_region_create (
     return NULL;
 }
 
-static int check_string_array (json_t *o, int min_count)
-{
-    size_t index;
-    json_t *entry;
-
-    if (!json_is_array (o)
-        || json_array_size (o) < min_count)
-        goto error;
-    json_array_foreach (o, index, entry) {
-        if (!json_is_string (entry))
-            goto error;
-    }
-    return 0;
-error:
-    errno = EPROTO;
-    return -1;
-}
-
 static void content_mmap_add_cb (flux_t *h,
                                  flux_msg_handler_t *mh,
                                  const flux_msg_t *msg,
                                  void *arg)
 {
     struct content_mmap *mm = arg;
     const char *path;
-    const char *fpath = NULL;
-    int disable_mmap;
-    struct blobvec_param param;
-    json_t *tags;
+    int chunksize;
+    const char *tag;
     struct content_region *reg = NULL;
     flux_error_t error;
     const char *errmsg = NULL;
-    size_t index;
-    json_t *entry;
 
     if (flux_request_unpack (msg,
                              NULL,
-                             "{s:s s?s s:b s:i s:i s:o}",
+                             "{s:s s:i s:s}",
                              "path", &path,
-                             "fullpath", &fpath,
-                             "disable_mmap", &disable_mmap,
-                             "chunksize", &param.chunksize,
-                             "threshold", &param.small_file_threshold,
-                             "tags", &tags) < 0
-        || check_string_array (tags, 1) < 0)
+                             "chunksize", &chunksize,
+                             "tag", &tag) < 0)
         goto error;
     if (mm->rank != 0) {
         errmsg = "content may only be mmapped on rank 0";
         goto inval;
     }
-    param.hashtype = mm->hash_name;
-    if (!(reg = content_mmap_region_create (mm,
-                                            path,
-                                            fpath,
-                                            disable_mmap ? NULL : &param,
-                                            &error))) {
+    if (path[0] != '/') {
+        errmsg = "path must be fully qualified";
+        goto inval;
+    }
+    if (!(reg = content_mmap_region_create (mm, path, chunksize, &error))) {
         errmsg = error.text;
         goto error;
     }
-    json_array_foreach (tags, index, entry) {
-        // takes a reference on region
-        if (!hola_list_add_end (mm->tags, json_string_value (entry), reg))
-            goto error;
-    }
+    // takes a reference on region
+    if (!hola_list_add_end (mm->tags, tag, reg))
+        goto error;
     if (flux_respond (h, msg, NULL) < 0)
         flux_log_error (h, "error responding to content.mmap-add request");
     content_mmap_region_decref (reg);
@@ -498,22 +480,17 @@ static void content_mmap_remove_cb (flux_t *h,
 {
     struct content_mmap *mm = arg;
     const char *errmsg = NULL;
-    json_t *tags;
-    size_t index;
-    json_t *entry;
+    const char *tag;
     int unmap_count = 0;
 
-    if (flux_request_unpack (msg, NULL, "{s:o}", "tags", &tags) < 0
-        || check_string_array (tags, 1) < 0)
+    if (flux_request_unpack (msg, NULL, "{s:s}", "tag", &tag) < 0)
         goto error;
     if (mm->rank != 0) {
         errmsg = "content can only be mmapped on rank 0";
         goto inval;
     }
-    json_array_foreach (tags, index, entry) {
-        if (hola_hash_delete (mm->tags, json_string_value (entry)) == 0)
-            unmap_count++;
-    }
+    if (hola_hash_delete (mm->tags, tag) == 0)
+        unmap_count++;
     if (unmap_count > 0) {
         if (plug_cache_holes (mm) < 0) {
             errmsg = "error filling missing cache entries after unmap";