Libguestfs - Aug 2014 - [PATCH 1/2] fish: edit: factor out download and reupload phases

Share some code between edit_file_editor and edit_file_perl; mostly code
motion, with no actual behaviour change.
---
 fish/file-edit.c | 143 +++++++++++++++++++++++--------------------------------
 1 file changed, 60 insertions(+), 83 deletions(-)

diff --git a/fish/file-edit.c b/fish/file-edit.c
index ff36ac2..74cb89b 100644
--- a/fish/file-edit.c
+++ b/fish/file-edit.c
@@ -35,6 +35,9 @@
 
 #include "guestfs-internal-frontend.h"
 
+static int do_download (guestfs_h *g, const char *filename, char **tempfile);
+static int do_upload (guestfs_h *g, const char *filename, const char *tempfile,
+                      const char *backup_extension);
 static char *generate_random_name (const char *filename);
 static char *generate_backup_name (const char *filename,
                                    const char *backup_extension);
@@ -43,38 +46,15 @@ int
 edit_file_editor (guestfs_h *g, const char *filename, const char *editor,
                   const char *backup_extension, int verbose)
 {
-  CLEANUP_FREE char *tmpdir = guestfs_get_tmpdir (g);
   CLEANUP_UNLINK_FREE char *tmpfilename = NULL;
-  char buf[256];
-  CLEANUP_FREE char *newname = NULL;
   CLEANUP_FREE char *cmd = NULL;
   struct stat oldstat, newstat;
-  int r, fd;
+  int r;
   struct utimbuf times;
 
   /* Download the file and write it to a temporary. */
-  if (asprintf (&tmpfilename, "%s/libguestfsXXXXXX", tmpdir) ==
-1) {
-    perror ("asprintf");
-    return -1;
-  }
-
-  fd = mkstemp (tmpfilename);
-  if (fd == -1) {
-    perror ("mkstemp");
-    return -1;
-  }
-
-  snprintf (buf, sizeof buf, "/dev/fd/%d", fd);
-
-  if (guestfs_download (g, filename, buf) == -1) {
-    close (fd);
+  if (do_download (g, filename, &tmpfilename) == -1)
     return -1;
-  }
-
-  if (close (fd) == -1) {
-    perror (tmpfilename);
-    return -1;
-  }
 
   /* Set the time back a few seconds on the original file.  This is so
    * that if the user is very fast at editing, or if EDITOR is an
@@ -125,38 +105,7 @@ edit_file_editor (guestfs_h *g, const char *filename, const
char *editor,
       oldstat.st_size == newstat.st_size)
     return 1;
 
-  /* Upload to a new file in the same directory, so if it fails we
-   * don't end up with a partially written file.  Give the new file
-   * a completely random name so we have only a tiny chance of
-   * overwriting some existing file.
-   */
-  newname = generate_random_name (filename);
-  if (!newname)
-    return -1;
-
-  /* Write new content. */
-  if (guestfs_upload (g, tmpfilename, newname) == -1)
-    return -1;
-
-  /* Set the permissions, UID, GID and SELinux context of the new
-   * file to match the old file (RHBZ#788641).
-   */
-  if (guestfs_copy_attributes (g, filename, newname,
-      GUESTFS_COPY_ATTRIBUTES_ALL, 1, -1) == -1)
-    return -1;
-
-  /* Backup or overwrite the file. */
-  if (backup_extension) {
-    CLEANUP_FREE char *backupname = NULL;
-
-    backupname = generate_backup_name (filename, backup_extension);
-    if (backupname == NULL)
-      return -1;
-
-    if (guestfs_mv (g, filename, backupname) == -1)
-      return -1;
-  }
-  if (guestfs_mv (g, newname, filename) == -1)
+  if (do_upload (g, filename, tmpfilename, backup_extension) == -1)
     return -1;
 
   return 0;
@@ -166,37 +115,14 @@ int
 edit_file_perl (guestfs_h *g, const char *filename, const char *perl_expr,
                 const char *backup_extension, int verbose)
 {
-  CLEANUP_FREE char *tmpdir = guestfs_get_tmpdir (g);
   CLEANUP_UNLINK_FREE char *tmpfilename = NULL;
-  char buf[256];
-  CLEANUP_FREE char *newname = NULL;
   CLEANUP_FREE char *cmd = NULL;
   CLEANUP_FREE char *outfile = NULL;
-  int r, fd;
+  int r;
 
   /* Download the file and write it to a temporary. */
-  if (asprintf (&tmpfilename, "%s/libguestfsXXXXXX", tmpdir) ==
-1) {
-    perror ("asprintf");
+  if (do_download (g, filename, &tmpfilename) == -1)
     return -1;
-  }
-
-  fd = mkstemp (tmpfilename);
-  if (fd == -1) {
-    perror ("mkstemp");
-    return -1;
-  }
-
-  snprintf (buf, sizeof buf, "/dev/fd/%d", fd);
-
-  if (guestfs_download (g, filename, buf) == -1) {
-    close (fd);
-    return -1;
-  }
-
-  if (close (fd) == -1) {
-    perror (tmpfilename);
-    return -1;
-  }
 
   if (asprintf (&outfile, "%s.out", tmpfilename) == -1) {
     perror ("asprintf");
@@ -238,6 +164,57 @@ edit_file_perl (guestfs_h *g, const char *filename, const
char *perl_expr,
     return -1;
   }
 
+  if (do_upload (g, filename, tmpfilename, backup_extension) == -1)
+    return -1;
+
+  return 0;
+}
+
+static int
+do_download (guestfs_h *g, const char *filename, char **tempfile)
+{
+  CLEANUP_FREE char *tmpdir = guestfs_get_tmpdir (g);
+  CLEANUP_UNLINK_FREE char *tmpfilename = NULL;
+  char buf[256];
+  int fd;
+
+  /* Download the file and write it to a temporary. */
+  if (asprintf (&tmpfilename, "%s/libguestfsXXXXXX", tmpdir) ==
-1) {
+    perror ("asprintf");
+    return -1;
+  }
+
+  fd = mkstemp (tmpfilename);
+  if (fd == -1) {
+    perror ("mkstemp");
+    return -1;
+  }
+
+  snprintf (buf, sizeof buf, "/dev/fd/%d", fd);
+
+  if (guestfs_download (g, filename, buf) == -1) {
+    close (fd);
+    return -1;
+  }
+
+  if (close (fd) == -1) {
+    perror (tmpfilename);
+    return -1;
+  }
+
+  /* Hand over the temporary file. */
+  *tempfile = tmpfilename;
+  tmpfilename = NULL;
+
+  return 0;
+}
+
+static int
+do_upload (guestfs_h *g, const char *filename, const char *tempfile,
+           const char *backup_extension)
+{
+  CLEANUP_FREE char *newname = NULL;
+
   /* Upload to a new file in the same directory, so if it fails we
    * don't end up with a partially written file.  Give the new file
    * a completely random name so we have only a tiny chance of
@@ -248,7 +225,7 @@ edit_file_perl (guestfs_h *g, const char *filename, const
char *perl_expr,
     return -1;
 
   /* Write new content. */
-  if (guestfs_upload (g, tmpfilename, newname) == -1)
+  if (guestfs_upload (g, tempfile, newname) == -1)
     return -1;
 
   /* Set the permissions, UID, GID and SELinux context of the new
-- 
1.9.3

Allow null as value for the editor parameter of edit_file_editor, which
will then get it from the EDITOR envvar (falling back on vi).

This is basically code motion from the two edit_file_editor users to it.
---
 edit/edit.c      | 12 +++---------
 fish/edit.c      |  7 ++-----
 fish/file-edit.c |  6 ++++++
 fish/file-edit.h |  3 +++
 4 files changed, 14 insertions(+), 14 deletions(-)

diff --git a/edit/edit.c b/edit/edit.c
index f1c3b91..821f280 100644
--- a/edit/edit.c
+++ b/edit/edit.c
@@ -333,15 +333,9 @@ edit (const char *filename, const char *root)
 
   if (perl_expr != NULL) {
     r = edit_file_perl (g, filename, perl_expr, backup_extension, verbose);
-  } else {
-    const char *editor;
-
-    editor = getenv ("EDITOR");
-    if (editor == NULL)
-      editor = "vi";
-
-    r = edit_file_editor (g, filename, editor, backup_extension, verbose);
-  }
+  } else
+    r = edit_file_editor (g, filename, NULL /* use $EDITOR */,
+                          backup_extension, verbose);
 
   switch (r) {
   case -1:
diff --git a/fish/edit.c b/fish/edit.c
index 0f41a88..013306e 100644
--- a/fish/edit.c
+++ b/fish/edit.c
@@ -47,11 +47,8 @@ run_edit (const char *cmd, size_t argc, char *argv[])
     editor = "vi";
   else if (STRCASEEQ (cmd, "emacs"))
     editor = "emacs -nw";
-  else {
-    editor = getenv ("EDITOR");
-    if (editor == NULL)
-      editor = "vi"; /* could be cruel here and choose ed(1) */
-  }
+  else
+    editor = NULL; /* use $EDITOR */
 
   /* Handle 'win:...' prefix. */
   remotefilename = win_prefix (argv[0]);
diff --git a/fish/file-edit.c b/fish/file-edit.c
index 74cb89b..e6d25f5 100644
--- a/fish/file-edit.c
+++ b/fish/file-edit.c
@@ -52,6 +52,12 @@ edit_file_editor (guestfs_h *g, const char *filename, const
char *editor,
   int r;
   struct utimbuf times;
 
+  if (editor == NULL) {
+    editor = getenv ("EDITOR");
+    if (editor == NULL)
+      editor = "vi";
+  }
+
   /* Download the file and write it to a temporary. */
   if (do_download (g, filename, &tmpfilename) == -1)
     return -1;
diff --git a/fish/file-edit.h b/fish/file-edit.h
index 3f8b95e..1635254 100644
--- a/fish/file-edit.h
+++ b/fish/file-edit.h
@@ -25,6 +25,9 @@
  * Edit 'filename' using the specified 'editor' application.
  * If 'backup_extension' is not null, then a copy of 'filename'
is saved
  * with 'backup_extension' appended to its file name.
+ * If 'editor' is null, then the EDITOR environment variable
+ * will be queried for the editor application, leaving "vi" as
fallback
+ * if not set.
  *
  * Returns -1 for failure, 0 on success, 1 if the editor did not change
  * the file (e.g. the user closed the editor without saving).
-- 
1.9.3

ACK series.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-top is 'top' for virtual machines.  Tiny program with many
powerful monitoring features, net stats, disk stats, logging, etc.
http://people.redhat.com/~rjones/virt-top