Review of the memfile.c API and small refactorings

- Create `mf_free_fnames()` that frees and nullifies `mf_[f]fname`
- Create `mf_set_fnames()` to set the `mf_fname` and the `mf_ffname`
  altoghether
- Have `mf_do_open` return a bool to indicate success so that calles
	don't have to check `memfile_T::mf_fd` (file descriptor)
- Inline `mf_write_block`

2 files changed, 73 insertions, 77 deletions

diff --git a/src/nvim/memfile.c b/src/nvim/memfile.c
index 8cf5642a80..6599db787f 100644
--- a/src/nvim/memfile.c
+++ b/src/nvim/memfile.c
@@ -79,8 +79,6 @@ static size_t total_mem_used = 0;    /// total memory used for memfiles
 ///         - NULL, on failure.
 memfile_T *mf_open(char_u *fname, int flags)
 {
-  off_t size;
-
   memfile_T *mfp = xmalloc(sizeof(memfile_T));
 
   if (fname == NULL) {               // no file, use memory only
@@ -88,11 +86,9 @@ memfile_T *mf_open(char_u *fname, int flags)
     mfp->mf_ffname = NULL;
     mfp->mf_fd = -1;
   } else {                           // try to open the file
-    mf_do_open(mfp, fname, flags);
-
-    if (mfp->mf_fd < 0) {            // fail if file could not be opened
+    if (!mf_do_open(mfp, fname, flags)) {
       xfree(mfp);
-      return NULL;
+      return NULL;                   // fail if file could not be opened
     }
   }
 
@@ -115,6 +111,8 @@ memfile_T *mf_open(char_u *fname, int flags)
     }
   }
 
+  off_t size;
+
   // When recovering, the actual block size will be retrieved from block 0
   // in ml_recover(). The size used here may be wrong, therefore mf_blocknr_max
   // must be rounded up.
@@ -171,13 +169,12 @@ memfile_T *mf_open(char_u *fname, int flags)
 ///         FAIL  If file could not be opened.
 int mf_open_file(memfile_T *mfp, char_u *fname)
 {
-  mf_do_open(mfp, fname, O_RDWR|O_CREAT|O_EXCL);  // try to open the file
-
-  if (mfp->mf_fd < 0)
-    return FAIL;
+  if (mf_do_open(mfp, fname, O_RDWR | O_CREAT | O_EXCL)) {
+    mfp->mf_dirty = true;
+    return OK;
+  }
 
-  mfp->mf_dirty = true;
-  return OK;
+  return FAIL;
 }
 
 /// Close a memory file and optionally delete the associated file.
@@ -185,28 +182,28 @@ int mf_open_file(memfile_T *mfp, char_u *fname)
 /// @param del_file  Whether to delete associated file.
 void mf_close(memfile_T *mfp, bool del_file)
 {
-  bhdr_T      *hp, *nextp;
-
   if (mfp == NULL) {                    // safety check
     return;
   }
   if (mfp->mf_fd >= 0 && close(mfp->mf_fd) < 0) {
       EMSG(_(e_swapclose));
   }
-  if (del_file && mfp->mf_fname != NULL)
+  if (del_file && mfp->mf_fname != NULL) {
     os_remove((char *)mfp->mf_fname);
+  }
+
   // free entries in used list
-  for (hp = mfp->mf_used_first; hp != NULL; hp = nextp) {
+  for (bhdr_T *hp = mfp->mf_used_first, *nextp; hp != NULL; hp = nextp) {
     total_mem_used -= hp->bh_page_count * mfp->mf_page_size;
     nextp = hp->bh_next;
     mf_free_bhdr(hp);
   }
-  while (mfp->mf_free_first != NULL)    // free entries in free list
+  while (mfp->mf_free_first != NULL) {  // free entries in free list
     xfree(mf_rem_free(mfp));
+  }
   mf_hash_free(&mfp->mf_hash);
   mf_hash_free_all(&mfp->mf_trans);     // free hashtable and its items
-  xfree(mfp->mf_fname);
-  xfree(mfp->mf_ffname);
+  mf_free_fnames(mfp);
   xfree(mfp);
 }
 
@@ -216,28 +213,28 @@ void mf_close(memfile_T *mfp, bool del_file)
 void mf_close_file(buf_T *buf, bool getlines)
 {
   memfile_T *mfp = buf->b_ml.ml_mfp;
-  if (mfp == NULL || mfp->mf_fd < 0)     // nothing to close
+  if (mfp == NULL || mfp->mf_fd < 0) {   // nothing to close
     return;
+  }
 
   if (getlines) {
     // get all blocks in memory by accessing all lines (clumsy!)
     mf_dont_release = true;
-    for (linenr_T lnum = 1; lnum <= buf->b_ml.ml_line_count; ++lnum)
+    for (linenr_T lnum = 1; lnum <= buf->b_ml.ml_line_count; ++lnum) {
       (void)ml_get_buf(buf, lnum, false);
+    }
     mf_dont_release = false;
     // TODO(elmart): should check if all blocks are really in core
   }
 
-  if (close(mfp->mf_fd) < 0)             // close the file
+  if (close(mfp->mf_fd) < 0) {           // close the file
     EMSG(_(e_swapclose));
+  }
   mfp->mf_fd = -1;
 
   if (mfp->mf_fname != NULL) {
     os_remove((char *)mfp->mf_fname);    // delete the swap file
-    xfree(mfp->mf_fname);
-    xfree(mfp->mf_ffname);
-    mfp->mf_fname = NULL;
-    mfp->mf_ffname = NULL;
+    mf_free_fnames(mfp);
   }
 }
 
@@ -390,11 +387,11 @@ void mf_put(memfile_T *mfp, bhdr_T *hp, bool dirty, bool infile)
 /// Signal block as no longer used (may put it in the free list).
 void mf_free(memfile_T *mfp, bhdr_T *hp)
 {
-  xfree(hp->bh_data);            // free data
+  xfree(hp->bh_data);           // free data
   mf_rem_hash(mfp, hp);         // get *hp out of the hash list
   mf_rem_used(mfp, hp);         // get *hp out of the used list
   if (hp->bh_bnum < 0) {
-    xfree(hp);                   // don't want negative numbers in free list
+    xfree(hp);                  // don't want negative numbers in free list
     mfp->mf_neg_count--;
   } else {
     mf_ins_free(mfp, hp);       // put *hp in the free list
@@ -475,10 +472,11 @@ int mf_sync(memfile_T *mfp, int flags)
 /// These are blocks that need to be written to a newly created swapfile.
 void mf_set_dirty(memfile_T *mfp)
 {
-  bhdr_T *hp;
-  for (hp = mfp->mf_used_last; hp != NULL; hp = hp->bh_prev)
-    if (hp->bh_bnum > 0)
+  for (bhdr_T *hp = mfp->mf_used_last; hp != NULL; hp = hp->bh_prev) {
+    if (hp->bh_bnum > 0) {
       hp->bh_flags |= BH_DIRTY;
+    }
+  }
   mfp->mf_dirty = true;
 }
 
@@ -506,10 +504,11 @@ static void mf_ins_used(memfile_T *mfp, bhdr_T *hp)
   hp->bh_next = mfp->mf_used_first;
   mfp->mf_used_first = hp;
   hp->bh_prev = NULL;
-  if (hp->bh_next == NULL)      // list was empty, adjust last pointer
+  if (hp->bh_next == NULL) {    // list was empty, adjust last pointer
     mfp->mf_used_last = hp;
-  else
+  } else {
     hp->bh_next->bh_prev = hp;
+  }
   mfp->mf_used_count += hp->bh_page_count;
   total_mem_used += hp->bh_page_count * mfp->mf_page_size;
 }
@@ -615,9 +614,10 @@ bool mf_release_all(void)
   FOR_ALL_BUFFERS(buf) {
     memfile_T *mfp = buf->b_ml.ml_mfp;
     if (mfp != NULL) {
-      // If no swap file yet, may open one.
-      if (mfp->mf_fd < 0 && buf->b_may_swap)
+      // If no swap file yet, try to open one.
+      if (mfp->mf_fd < 0 && buf->b_may_swap) {
         ml_open_file(buf);
+      }
 
       // Flush as many blocks as possible, only if there is a swapfile.
       if (mfp->mf_fd >= 0) {
@@ -752,7 +752,8 @@ static int mf_write(memfile_T *mfp, bhdr_T *hp)
     else
       page_count = hp2->bh_page_count;
     size = page_size * page_count;
-    if (mf_write_block(mfp, hp2 == NULL ? hp : hp2, offset, size) == FAIL) {
+    void *data = (hp2 == NULL) ? hp->bh_data : hp2->bh_data;
+    if ((unsigned)write_eintr(mfp->mf_fd, data, size) != size) {
       /// Avoid repeating the error message, this mostly happens when the
       /// disk is full. We give the message again only after a successful
       /// write or when hitting a key. We keep on trying, in case some
@@ -773,20 +774,6 @@ static int mf_write(memfile_T *mfp, bhdr_T *hp)
   return OK;
 }
 
-/// Write block to memfile's file.
-///
-/// @return  OK    On success.
-///          FAIL  On failure.
-static int mf_write_block(memfile_T *mfp, bhdr_T *hp,
-                          off_t offset, unsigned size)
-{
-  void *data = hp->bh_data;
-  int result = OK;
-  if ((unsigned)write_eintr(mfp->mf_fd, data, size) != size)
-    result = FAIL;
-  return result;
-}
-
 /// Make block number positive and add it to the translation list.
 ///
 /// @return  OK    On success.
@@ -856,13 +843,23 @@ blocknr_T mf_trans_del(memfile_T *mfp, blocknr_T old_nr)
   return new_bnum;
 }
 
-/// Set full file name of memfile's swapfile, out of simple file name and some
-/// other considerations.
+/// Frees mf_fname and mf_ffname.
+void mf_free_fnames(memfile_T *mfp)
+{
+  xfree(mfp->mf_fname);
+  xfree(mfp->mf_ffname);
+  mfp->mf_fname = NULL;
+  mfp->mf_ffname = NULL;
+}
+
+/// Set the simple file name and the full file name of memfile's swapfile, out
+/// of simple file name and some other considerations.
 ///
 /// Only called when creating or renaming the swapfile. Either way it's a new
 /// name so we must work out the full path name.
-void mf_set_ffname(memfile_T *mfp)
+void mf_set_fnames(memfile_T *mfp, char_u *fname)
 {
+  mfp->mf_fname = fname;
   mfp->mf_ffname = (char_u *)FullName_save((char *)mfp->mf_fname, false);
 }
 
@@ -878,7 +875,7 @@ void mf_fullname(memfile_T *mfp)
   }
 }
 
-/// Return TRUE if there are any translations pending for memfile.
+/// Return true if there are any translations pending for memfile.
 bool mf_need_trans(memfile_T *mfp)
 {
   return mfp->mf_fname != NULL && mfp->mf_neg_count > 0;
@@ -889,11 +886,11 @@ bool mf_need_trans(memfile_T *mfp)
 /// "fname" must be in allocated memory, and is consumed (also when error).
 ///
 /// @param  flags  Flags for open().
-static void mf_do_open(memfile_T *mfp, char_u *fname, int flags)
+/// @return A bool indicating success of the `open` call.
+static bool mf_do_open(memfile_T *mfp, char_u *fname, int flags)
 {
   // fname cannot be NameBuff, because it must have been allocated.
-  mfp->mf_fname = fname;
-  mf_set_ffname(mfp);
+  mf_set_fnames(mfp, fname);
 
   /// Extra security check: When creating a swap file it really shouldn't
   /// exist yet. If there is a symbolic link, this is most likely an attack.
@@ -904,26 +901,26 @@ static void mf_do_open(memfile_T *mfp, char_u *fname, int flags)
     EMSG(_("E300: Swap file already exists (symlink attack?)"));
   } else {
     // try to open the file
-    flags |= O_NOFOLLOW;
-    mfp->mf_fd = mch_open_rw((char *)mfp->mf_fname, flags);
+    mfp->mf_fd = mch_open_rw((char *)mfp->mf_fname, flags | O_NOFOLLOW);
   }
 
   // If the file cannot be opened, use memory only
   if (mfp->mf_fd < 0) {
-    xfree(mfp->mf_fname);
-    xfree(mfp->mf_ffname);
-    mfp->mf_fname = NULL;
-    mfp->mf_ffname = NULL;
-  } else {
+    mf_free_fnames(mfp);
+    return false;
+  }
+
 #ifdef HAVE_FD_CLOEXEC
-    int fdflags = fcntl(mfp->mf_fd, F_GETFD);
-    if (fdflags >= 0 && (fdflags & FD_CLOEXEC) == 0)
-      fcntl(mfp->mf_fd, F_SETFD, fdflags | FD_CLOEXEC);
+  int fdflags = fcntl(mfp->mf_fd, F_GETFD);
+  if (fdflags >= 0 && (fdflags & FD_CLOEXEC) == 0) {
+    fcntl(mfp->mf_fd, F_SETFD, fdflags | FD_CLOEXEC);
+  }
 #endif
 #ifdef HAVE_SELINUX
-    mch_copy_sec(fname, mfp->mf_fname);
+  mch_copy_sec(fname, mfp->mf_fname);
 #endif
-  }
+
+  return true;
 }
 
 //
@@ -948,20 +945,21 @@ static void mf_hash_init(mf_hashtab_T *mht)
 /// The hash table must not be used again without another mf_hash_init() call.
 static void mf_hash_free(mf_hashtab_T *mht)
 {
-  if (mht->mht_buckets != mht->mht_small_buckets)
+  if (mht->mht_buckets != mht->mht_small_buckets) {
     xfree(mht->mht_buckets);
+  }
 }
 
 /// Free the array of a hash table and all the items it contains.
 static void mf_hash_free_all(mf_hashtab_T *mht)
 {
-  mf_hashitem_T *next;
-
-  for (size_t idx = 0; idx <= mht->mht_mask; idx++)
+  for (size_t idx = 0; idx <= mht->mht_mask; idx++) {
+    mf_hashitem_T *next;
     for (mf_hashitem_T *mhi = mht->mht_buckets[idx]; mhi != NULL; mhi = next) {
       next = mhi->mhi_next;
       xfree(mhi);
     }
+  }
 
   mf_hash_free(mht);
 }
diff --git a/src/nvim/memline.c b/src/nvim/memline.c
index f58b2ac38f..b568279d7d 100644
--- a/src/nvim/memline.c
+++ b/src/nvim/memline.c
@@ -426,10 +426,8 @@ void ml_setname(buf_T *buf)
     /* try to rename the swap file */
     if (vim_rename(mfp->mf_fname, fname) == 0) {
       success = TRUE;
-      xfree(mfp->mf_fname);
-      mfp->mf_fname = fname;
-      xfree(mfp->mf_ffname);
-      mf_set_ffname(mfp);
+      mf_free_fnames(mfp);
+      mf_set_fnames(mfp, fname);
       ml_upd_block0(buf, UB_SAME_DIR);
       break;
     }