[gnuastro-commits] master 22054c4 1/2: Fits program opening file fixed for keyword editing

[Mohammad Akhlaghi]

branch: master
commit 22054c4a1d52e0f38b4baa3e41242d57ba3d7e52
Author: Mohammad Akhlaghi <address@hidden>
Commit: Mohammad Akhlaghi <address@hidden>

    Fits program opening file fixed for keyword editing
    
    When any of the commands to edit FITS keywords (except `--delete') were
    given to the Fits program, it would crash with a segmentation fault!
    
    The problem was that the call to CFITSIO to read in the FITS file
    information was only being read for the `--delete' option and not the rest
    of the operations. This issue is fixed now.
    
    I also noticed that when a CFITSIO error came up (for example a keyword
    that has been asked to be deleted doesn't exist), we wouldn't reset the
    `status' value. So none of the other operations would be done and in the
    end CFITSIO would complain (which is not the expected operation in Fits
    when `--quitonerror' is not called).
    
    The problem was found by Johannes Zabl.
---
 NEWS                         |  4 ++++
 THANKS                       |  1 +
 bin/fits/fits.c              | 13 ++++++++----
 bin/fits/keywords.c          | 47 +++++++++++++++++++++++++++++++++++---------
 doc/announce-acknowledge.txt |  1 +
 5 files changed, 53 insertions(+), 13 deletions(-)

diff --git a/NEWS b/NEWS
index 0c88d37..6f585be 100644
--- a/NEWS
+++ b/NEWS
@@ -50,6 +50,10 @@ GNU Astronomy Utilities NEWS                          -*- 
outline -*-
 
   NoiseChisel crash when there is no detection (bug #53304).
 
+  Fits crash on keyword editing (except --delete) (bug #53312).
+
+
+
 
 
 * Noteworthy changes in release 0.5 (library 3.0.0) (2017-12-22) [stable]
diff --git a/THANKS b/THANKS
index 7ae2873..219ff35 100644
--- a/THANKS
+++ b/THANKS
@@ -55,6 +55,7 @@ support in Gnuastro. The list is ordered alphabetically (by 
family name).
     Aaron Watkins                        address@hidden
     Christopher Willmer                  address@hidden
     Sara Yousefi Taemeh                  address@hidden
+    Johannes Zabl                        address@hidden
 
 
 Teams
diff --git a/bin/fits/fits.c b/bin/fits/fits.c
index b836dd4..895e7c0 100644
--- a/bin/fits/fits.c
+++ b/bin/fits/fits.c
@@ -64,11 +64,11 @@ fits_has_error(struct fitsparams *p, int actioncode, char 
*string, int status)
   if(p->quitonerror)
     {
       fits_report_error(stderr, status);
-      error(EXIT_FAILURE, 0, "%s: not %s: %s\n", __func__, action, string);
+      error(EXIT_FAILURE, 0, "%s: %s: not %s\n", __func__, string, action);
     }
   else
     {
-      fprintf(stderr, "Not %s: %s\n", action, string);
+      fprintf(stderr, "%s: Not %s.\n", string, action);
       r=EXIT_FAILURE;
     }
   return r;
@@ -259,6 +259,7 @@ fits_hdu_remove(struct fitsparams *p, int *r)
       /* Delete the extension. */
       if( fits_delete_hdu(fptr, &hdutype, &status) )
         *r=fits_has_error(p, FITS_ACTION_REMOVE, hdu, status);
+      status=0;
 
       /* Close the file. */
       fits_close_file(fptr, &status);
@@ -293,11 +294,15 @@ fits_hdu_copy(struct fitsparams *p, int cut1_copy0, int 
*r)
       /* Copy to the extension. */
       if( fits_copy_hdu(in, out, 0, &status) )
         *r=fits_has_error(p, FITS_ACTION_COPY, hdu, status);
+      status=0;
 
       /* If this is a `cut' operation, then remove the extension. */
       if(cut1_copy0)
-        if( fits_delete_hdu(in, &hdutype, &status) )
-          *r=fits_has_error(p, FITS_ACTION_REMOVE, hdu, status);
+        {
+          if( fits_delete_hdu(in, &hdutype, &status) )
+            *r=fits_has_error(p, FITS_ACTION_REMOVE, hdu, status);
+          status=0;
+        }
 
       /* Close the input file. */
       fits_close_file(in, &status);
diff --git a/bin/fits/keywords.c b/bin/fits/keywords.c
index cc0a80a..f36e49f 100644
--- a/bin/fits/keywords.c
+++ b/bin/fits/keywords.c
@@ -109,6 +109,7 @@ keywords_rename_keys(struct fitsparams *p, fitsfile **fptr, 
int *r)
       /* Rename the keyword */
       fits_modify_name(*fptr, from, to, &status);
       if(status) *r=fits_has_error(p, FITS_ACTION_RENAME, from, status);
+      status=0;
 
       /* Clean up the user's input string. Note that `strtok' just changes
          characters within the allocated string, no extra allocation is
@@ -208,9 +209,6 @@ keywords_print_all_keys(struct fitsparams *p, fitsfile 
**fptr)
   int nkeys, status=0;
   char *fullheader, *c, *cf;
 
-  /* Open the FITS file. */
-  keywords_open(p, fptr, READONLY);
-
   /* Conver the header into a contiguous string. */
   if( fits_hdr2str(*fptr, 0, NULL, 0, &fullheader, &nkeys, &status) )
     gal_fits_io_error(status, NULL);
@@ -257,6 +255,15 @@ keywords_print_all_keys(struct fitsparams *p, fitsfile 
**fptr)
 /***********************************************************************/
 /******************           Main function         ********************/
 /***********************************************************************/
+/* NOTE ON CALLING keywords_open FOR EACH OPERATION:
+
+   `keywords_open' is being called individually for each separate operation
+   because the necessary permissions differ: when the user only wants to
+   read keywords, they don't necessarily need write permissions. So if they
+   haven't asked for any writing/editing operation, we shouldn't open in
+   write-mode. Because the user might not have the permissions to write and
+   they might not want to write. `keywords_open' will only open the file
+   once (if the pointer is already allocated, it won't do anything). */
 int
 keywords(struct fitsparams *p)
 {
@@ -269,29 +276,42 @@ keywords(struct fitsparams *p)
   /* Delete the requested keywords. */
   if(p->delete)
     {
+      /* Open the FITS file. */
       keywords_open(p, &fptr, READWRITE);
+
+      /* Go over all the keywords to delete. */
       for(tstll=p->delete; tstll!=NULL; tstll=tstll->next)
         {
           fits_delete_key(fptr, tstll->v, &status);
           if(status)
             r=fits_has_error(p, FITS_ACTION_DELETE, tstll->v, status);
+          status=0;
         }
     }
 
 
   /* Rename the requested keywords. */
   if(p->rename)
-    keywords_rename_keys(p, &fptr, &r);
+    {
+      keywords_open(p, &fptr, READWRITE);
+      keywords_rename_keys(p, &fptr, &r);
+    }
 
 
   /* Update the requested keywords. */
   if(p->update)
-    keywords_write_update(p, &fptr, p->update_keys, 1);
+    {
+      keywords_open(p, &fptr, READWRITE);
+      keywords_write_update(p, &fptr, p->update_keys, 1);
+    }
 
 
   /* Write the requested keywords. */
   if(p->write)
-    keywords_write_update(p, &fptr, p->write_keys, 2);
+    {
+      keywords_open(p, &fptr, READWRITE);
+      keywords_write_update(p, &fptr, p->write_keys, 2);
+    }
 
 
   /* Put in any full line of keywords as-is. */
@@ -300,37 +320,46 @@ keywords(struct fitsparams *p)
       {
         fits_write_record(fptr, tstll->v, &status);
         if(status) r=fits_has_error(p, FITS_ACTION_WRITE, tstll->v, status);
+        status=0;
       }
 
 
   /* Add the history keyword(s). */
   if(p->history)
     {
+      keywords_open(p, &fptr, READWRITE);
       fits_write_history(fptr, p->history, &status);
       if(status) r=fits_has_error(p, FITS_ACTION_WRITE, "HISTORY", status);
+      status=0;
     }
 
 
   /* Add comment(s). */
   if(p->comment)
     {
+      keywords_open(p, &fptr, READWRITE);
       fits_write_comment(fptr, p->comment, &status);
       if(status) r=fits_has_error(p, FITS_ACTION_WRITE, "COMMENT", status);
+      status=0;
     }
 
 
   /* Update/add the date. */
   if(p->date)
     {
+      keywords_open(p, &fptr, READWRITE);
       fits_write_date(fptr, &status);
       if(status) r=fits_has_error(p, FITS_ACTION_WRITE, "DATE", status);
+      status=0;
     }
 
 
-  /* If nothing was requested, then print all the keywords in this
-     extension. */
+  /* Print all the keywords in the extension. */
   if(p->printallkeys)
-    keywords_print_all_keys(p, &fptr);
+    {
+      keywords_open(p, &fptr, READONLY);
+      keywords_print_all_keys(p, &fptr);
+    }
 
 
   /* Close the FITS file */
diff --git a/doc/announce-acknowledge.txt b/doc/announce-acknowledge.txt
index 7e8767c..b76f702 100644
--- a/doc/announce-acknowledge.txt
+++ b/doc/announce-acknowledge.txt
@@ -9,3 +9,4 @@ Juan C. Tello
 Éric Thiébaut
 Aaron Watkins
 Sara Yousefi Taemeh
+Johannes Zabl