We're an editor now, as we let you add, delete, and edit frame comments,
authorGuy Harris <guy@alum.mit.edu>
Tue, 22 May 2012 10:36:40 +0000 (10:36 -0000)
committerGuy Harris <guy@alum.mit.edu>
Tue, 22 May 2012 10:36:40 +0000 (10:36 -0000)
so "Save" should, for non-temporary files, mean "save the current state
of the capture file on top of the existing file" without prompting for a
file name.

That means we have to do a "safe save" - i.e, write the capture out to a
new file and, if that succeeds, rename the new file on top of the old
file - as the actual packet data to write out is in the file we're
overwriting, not in memory.  (We'd want to do that anyway, of
course....)

Update some comments.

Clean up indentation slightly, and get rid of an unnecessary variable
(in all the cases where we use it, we assign it the same value, and that
value isn't modified out from under us before we use it).

Note that after a "Save", or a "Save As" that writes out all captured
packets, we shouldn't have to close the current file and open the new
file and reread it - we should be able to open the new file and update
the frame offsets in the frame_data structures.

Note that we need to do some a better job of reporting rename failures.

svn path=/trunk/; revision=42777

file.c
file.h
ui/gtk/capture_file_dlg.c
ui/gtk/capture_file_dlg.h

diff --git a/file.c b/file.c
index 360879d3766de1d92df34c1ea948e333487951fb..a1272a834bec194db708f614b6b65be16961b0c5 100644 (file)
--- a/file.c
+++ b/file.c
@@ -3805,10 +3805,11 @@ cf_can_save_as(capture_file *cf)
   return FALSE;
 }
 
-cf_status_t
-cf_save(capture_file *cf, const char *fname, packet_range_t *range, guint save_format, gboolean compressed)
+static cf_status_t
+cf_save_packets(capture_file *cf, const char *fname, packet_range_t *range,
+                guint save_format, gboolean compressed, gboolean do_overwrite)
 {
-  gchar        *from_filename;
+  gchar        *fname_new = NULL;
   int           err;
   gboolean      do_copy;
   wtap_dumper  *pdh;
@@ -3816,37 +3817,59 @@ cf_save(capture_file *cf, const char *fname, packet_range_t *range, guint save_f
 
   cf_callback_invoke(cf_cb_file_save_started, (gpointer)fname);
 
-  /* don't write over an existing file. */
-  /* this should've been already checked by our caller, just to be sure... */
   if (file_exists(fname)) {
-    simple_dialog(ESD_TYPE_ERROR, ESD_BTN_OK,
-      "%sCapture file: \"%s\" already exists!%s\n\n"
-      "Please choose a different filename.",
-      simple_dialog_primary_start(), fname, simple_dialog_primary_end());
-    goto fail;
+    if (do_overwrite) {
+      /* We're overwriting an existing file; write out to a new file,
+         and, if that succeeds, rename the new file on top of the
+         old file.  That makes this a "safe save", so that we don't
+         lose the old file if we have a problem writing out the new
+         file.  (If the existing file is the current capture file,
+         we *HAVE* to do that, otherwise we're overwriting the file
+         from which we're reading the packets that we're writing!) */
+
+      fname_new = g_strdup_printf("%s~", fname);
+    } else {
+      /* don't write over an existing file. */
+      /* this should've been already checked by our caller, just to be sure... */
+      if (file_exists(fname)) {
+        simple_dialog(ESD_TYPE_ERROR, ESD_BTN_OK,
+                      "%sCapture file: \"%s\" already exists!%s\n\n"
+                      "Please choose a different filename.",
+                      simple_dialog_primary_start(),
+                      fname,
+                      simple_dialog_primary_end());
+        goto fail;
+      }
+    }
   }
 
   packet_range_process_init(range);
 
-  if (packet_range_process_all(range) && save_format == cf->cd_t) {
+  if (packet_range_process_all(range) && save_format == cf->cd_t
+      && !cf->unsaved_changes) {
     /* We're not filtering packets, and we're saving it in the format
-       it's already in, so we can just move or copy the raw data. */
+       it's already in, and there are no changes we have in memory
+       that aren't saved to the file, so we can just move or copy the
+       raw data. */
 
     if (cf->is_tempfile) {
       /* The file being saved is a temporary file from a live
          capture, so it doesn't need to stay around under that name;
-         first, try renaming the capture buffer file to the new name. */
+         first, try renaming the capture buffer file to the new name.
+
+         XXX - ws_rename() should be ws_stdio_rename() on Windows,
+         and ws_stdio_rename() uses MoveFileEx() with MOVEFILE_REPLACE_EXISTING,
+         so it should remove the target if it exists, so this stuff
+         should be OK even on Windows. */
 #ifndef _WIN32
       if (ws_rename(cf->filename, fname) == 0) {
         /* That succeeded - there's no need to copy the source file. */
-        from_filename = NULL;
-    do_copy = FALSE;
+        do_copy = FALSE;
       } else {
         if (errno == EXDEV) {
           /* They're on different file systems, so we have to copy the
              file. */
           do_copy = TRUE;
-          from_filename = cf->filename;
         } else {
           /* The rename failed, but not because they're on different
              file systems - put up an error message.  (Or should we
@@ -3862,24 +3885,32 @@ cf_save(capture_file *cf, const char *fname, packet_range_t *range, guint save_f
       }
 #else
       do_copy = TRUE;
-      from_filename = cf->filename;
 #endif
     } else {
       /* It's a permanent file, so we should copy it, and not remove the
          original. */
       do_copy = TRUE;
-      from_filename = cf->filename;
     }
 
     if (do_copy) {
-      /* Copy the file, if we haven't moved it. */
-      if (!copy_file_binary_mode(from_filename, fname))
+      /* Copy the file, if we haven't moved it.
+         This does not happen if we have unsaved changes (see above),
+         so it's either happening as the result of an explicit "Save
+         As", in which case we've already made sure the target file
+         doesn't exist, or it's a "Save" on a temporary file for a
+         capture, in which case the user was asked for the file name
+         and, again, we've already made sure the target file doesn't
+         exist.  That means we don't need to worry about safe saves. */
+      if (!copy_file_binary_mode(cf->filename, fname))
         goto fail;
     }
   } else {
     /* Either we're filtering packets, or we're saving in a different
-       format; we can't do that by copying or moving the capture file,
-       we have to do it by writing the packets out in Wiretap. */
+       format, or we're saving changes, such as added, modified, or
+       removed comments, that haven't yet been written to the
+       underlying file; we can't do that by copying or moving the
+       capture file, we have to do it by writing the packets out in
+       Wiretap. */
 
     wtapng_section_t *shb_hdr = NULL;
     wtapng_iface_descriptions_t *idb_inf = NULL;
@@ -3887,8 +3918,20 @@ cf_save(capture_file *cf, const char *fname, packet_range_t *range, guint save_f
     shb_hdr = wtap_file_get_shb_info(cf->wth);
     idb_inf = wtap_file_get_idb_info(cf->wth);
 
-    pdh = wtap_dump_open_ng(fname, save_format, cf->lnk_t, cf->snap,
-        compressed, shb_hdr, idb_inf, &err);
+    if (fname_new != NULL) {
+      /* We're overwriting an existing file; write out to a new file,
+         and, if that succeeds, rename the new file on top of the
+         old file.  That makes this a "safe save", so that we don't
+         lose the old file if we have a problem writing out the new
+         file.  (If the existing file is the current capture file,
+         we *HAVE* to do that, otherwise we're overwriting the file
+         from which we're reading the packets that we're writing!) */
+      pdh = wtap_dump_open_ng(fname_new, save_format, cf->lnk_t, cf->snap,
+                              compressed, shb_hdr, idb_inf, &err);
+    } else {
+      pdh = wtap_dump_open_ng(fname, save_format, cf->lnk_t, cf->snap,
+                              compressed, shb_hdr, idb_inf, &err);
+    }
     g_free(idb_inf);
     idb_inf = NULL;
 
@@ -3940,6 +3983,27 @@ cf_save(capture_file *cf, const char *fname, packet_range_t *range, guint save_f
     }
   }
 
+  if (fname_new != NULL) {
+    /* We wrote out to fname_new, and should rename it on top of
+       fname; fname is now closed, so that should be possible even
+       on Windows.  Do the rename. */
+    if (ws_rename(fname_new, fname) == -1) {
+      /* Well, the rename failed.  Discard the file we wrote out,
+         and return an error. */
+      int rename_err = errno;
+
+      /* If this fails, there's nothing we can do to deal with that,
+         and whatever error it got is less relevant to the user than
+         the error from the rename failing, so we don't bother
+         checking for errors in the unlink. */
+      ws_unlink(fname_new);
+
+      simple_dialog(ESD_TYPE_ERROR, ESD_BTN_OK,
+                    file_rename_error_message(rename_err), fname);
+      goto fail;
+    }
+  }
+
   cf_callback_invoke(cf_cb_file_save_finished, NULL);
 
   if (packet_range_process_all(range)) {
@@ -3952,7 +4016,12 @@ cf_save(capture_file *cf, const char *fname, packet_range_t *range, guint save_f
        file be the one we have opened and from which we're reading
        the data, and it means we have to spend time opening and
        reading the file, which could be a significant amount of
-       time if the file is large. */
+       time if the file is large.
+
+       If the capture-file-writing code were to return the
+       seek offset of each packet it writes, we could save that
+       in the frame_data structure for the frame, and just open
+       the file without reading it again. */
     cf->unsaved_changes = FALSE;
 
     if ((cf_open(cf, fname, FALSE, &err)) == CF_OK) {
@@ -3984,6 +4053,23 @@ fail:
   return CF_ERROR;
 }
 
+cf_status_t
+cf_save(capture_file *cf, const char *fname, guint save_format, gboolean compressed)
+{
+  packet_range_t range;
+
+  /* This only does a "save all", so we have our own packet_range_t
+     structure, which is set to the default "save everything" state. */
+  packet_range_init(&range);
+  return cf_save_packets(cf, fname, &range, save_format, compressed, TRUE);
+}
+
+cf_status_t
+cf_save_as(capture_file *cf, const char *fname, packet_range_t *range, guint save_format, gboolean compressed)
+{
+  return cf_save_packets(cf, fname, range, save_format, compressed, FALSE);
+}
+
 static void
 cf_open_failure_alert_box(const char *filename, int err, gchar *err_info,
                           gboolean for_writing, int file_type)
@@ -4120,6 +4206,11 @@ cf_open_failure_alert_box(const char *filename, int err, gchar *err_info,
   }
 }
 
+/*
+ * XXX - whether we mention the source pathname, the target pathname,
+ * or both depends on the error and on what we find if we look for
+ * one or both of them.
+ */
 static const char *
 file_rename_error_message(int err)
 {
@@ -4129,14 +4220,21 @@ file_rename_error_message(int err)
   switch (err) {
 
   case ENOENT:
+    /* XXX - should check whether the source exists and, if not,
+       report it as the problem and, if so, report the destination
+       as the problem. */
     errmsg = "The path to the file \"%s\" doesn't exist.";
     break;
 
   case EACCES:
+    /* XXX - if we're doing a rename after a safe save, we should
+       probably say something else. */
     errmsg = "You don't have permission to move the capture file to \"%s\".";
     break;
 
   default:
+    /* XXX - this should probably mention both the source and destination
+       pathnames. */
     g_snprintf(errmsg_errno, sizeof(errmsg_errno),
             "The file \"%%s\" could not be moved: %s.",
                 wtap_strerror(err));
diff --git a/file.h b/file.h
index 1b093e3c7e28df8ca7b9bb167ed1a02a302e0bb9..6371b611c409f7196878a1a7c21367ad993dae26 100644 (file)
--- a/file.h
+++ b/file.h
@@ -196,7 +196,20 @@ cf_read_status_t cf_finish_tail(capture_file *cf, int *err);
 gboolean cf_can_save_as(capture_file *cf);
 
 /**
- * Save a capture file (or a range of it).
+ * Save a capture file.  Does a "safe save" if the specified
+ * pathname already exists.
+ *
+ * @param cf the capture file to save to
+ * @param fname the filename to save to
+ * @param save_format the format of the file to save (libpcap, ...)
+ * @param compressed whether to gzip compress the file
+ * @return one of cf_status_t
+ */
+cf_status_t cf_save(capture_file * cf, const char *fname, guint save_format, gboolean compressed);
+
+/**
+ * Save a capture file (or a range of it).  Fails if the specified
+ * pathname already exists.
  *
  * @param cf the capture file to save to
  * @param fname the filename to save to
@@ -205,7 +218,7 @@ gboolean cf_can_save_as(capture_file *cf);
  * @param compressed whether to gzip compress the file
  * @return one of cf_status_t
  */
-cf_status_t cf_save(capture_file * cf, const char *fname, packet_range_t *range, guint save_format, gboolean compressed);
+cf_status_t cf_save_as(capture_file * cf, const char *fname, packet_range_t *range, guint save_format, gboolean compressed);
 
 /**
  * Get a displayable name of the capture file.
index cd22ebdf04962a87e8084ca29287236271838217..8bc09c21dd9dd0897d2d9163da5bf9a9a56dd347 100644 (file)
@@ -1100,16 +1100,20 @@ file_close_cmd_cb(GtkWidget *widget _U_, gpointer data _U_) {
 
 void
 file_save_cmd_cb(GtkWidget *w _U_, gpointer data _U_) {
-  /* If the file has no unsaved changes, and is not a temporary file,
-     do nothing.  */
-  if (!cfile.unsaved_changes && !cfile.is_tempfile)
-    return;
-
-  /* XXX TODO - if it's not a temporary file, "save" should just save
-     on top of the existing file. */
-
-  /* Do a "Save As". */
-  file_save_as_cmd(after_save_no_action, NULL, FALSE);
+  if (cfile.is_tempfile) {
+    /* This is a temporary capture file, so saving it means saving
+       it to a permanent file. */
+    file_save_as_cmd(after_save_no_action, NULL, FALSE);
+  } else {
+    if (cfile.unsaved_changes) {
+      /* This is not a temporary capture file, but it has unsaved
+         changes, so saving it means doing a "safe save" on top
+         of the existing file, in the same format - no UI needed. */
+      file_save_cmd(after_save_no_action, NULL);
+    }
+    /* Otherwise just do nothing (this shouldn't even be enabled if
+       it's not a temporary file and there are no unsaved changes). */
+  }
 }
 
 /* Attach a list of the valid 'save as' file types to a combo_box by
@@ -1175,6 +1179,23 @@ action_after_save_e action_after_save_g;
 gpointer            action_after_save_data_g;
 
 
+void
+file_save_cmd(action_after_save_e action_after_save, gpointer action_after_save_data)
+{
+  char *fname;
+
+  action_after_save_g       = action_after_save;
+  action_after_save_data_g  = action_after_save_data;
+
+  /* XXX - cfile.filename might get freed out from under us, because
+     the code path through which cf_save() goes currently closes the
+     current file and then opens and reloads the saved file, so make
+     a copy and free it later. */
+  fname = g_strdup(cfile.filename);
+  cf_save(&cfile, fname, cfile.cd_t, FALSE);
+  g_free(fname);
+}
+
 void
 file_save_as_cmd(action_after_save_e action_after_save, gpointer action_after_save_data, gboolean save_only_displayed)
 {
@@ -1298,11 +1319,11 @@ file_save_as_cb(GtkWidget *w _U_, gpointer fs) {
 
   /* XXX - if the user requests to save to an already existing filename, */
   /* ask in a dialog if that's intended */
-  /* currently, cf_save() will simply deny it */
+  /* currently, cf_save_as() will simply deny it */
 
   /* Write out the packets (all, or only the ones from the current
      range) to the file with the specified name. */
-  if (cf_save(&cfile, cf_name, &range, file_type,
+  if (cf_save_as(&cfile, cf_name, &range, file_type,
          gtk_toggle_button_get_active(GTK_TOGGLE_BUTTON(compressed_cb))) != CF_OK) {
     /* The write failed; don't dismiss the open dialog box,
        just leave it around so that the user can, after they
@@ -1317,7 +1338,7 @@ file_save_as_cb(GtkWidget *w _U_, gpointer fs) {
   }
 
   /* The write succeeded; get rid of the file selection box. */
-  /* cf_save() might already closed our dialog! */
+  /* cf_save_as() might already closed our dialog! */
   if (file_save_as_w)
     window_destroy(GTK_WIDGET (fs));
 
index 0353e08b7c012fa4aad205d48b25c29a569ebbba..3bea625102e65d89f4eb47f07a278bfff0efa5a0 100644 (file)
@@ -42,6 +42,13 @@ typedef enum {
     after_save_exit                 /**< exit program */
 } action_after_save_e;
 
+/** Do a "Save", opening a dialog box if the current file is a temporary file.
+ *
+ * @param action_after_save the action to take, when save completed
+ * @param action_after_save_data data for action_after_save
+ */
+void file_save_cmd(action_after_save_e action_after_save, gpointer action_after_save_data);
+
 /** Open the "Save As" dialog box.
  *
  * @param action_after_save the action to take, when save completed