[gnuastro-commits] master 5f1d2c73: Library (fits.h): table metadata parsed by returned status of CFITSIO

[Mohammad Akhlaghi]

branch: master
commit 5f1d2c735e75dd000ed90d62aaaaf728d7d9c807
Author: Mohammad Akhlaghi <mohammad@akhlaghi.org>
Commit: Mohammad Akhlaghi <mohammad@akhlaghi.org>

    Library (fits.h): table metadata parsed by returned status of CFITSIO
    
    Until now, we would check for the 'END' keyword to stop parsing all the
    FITS keywords (when it was necessary to extract table metadata). However,
    due to some intricacies in CFITSIO [1], this could cause problems in
    situations when the keyword before 'END' is empty (see the attached file in
    [2]): the 'END' keyword was never returned by 'fits_read_keyn', so the loop
    in 'gal_fits_tab_info' would never terminate.
    
    Upon finding the problem, a dirty work-around was implemented in Commit
    7d117daa2d0 ("Library (fits.h): Table info is read when END key not read by
    CFITSIO"). But that was not an elegant solution because it would replace a
    fake 'END'.
    
    With this commit, following the advice from Craig in [2], we are no longer
    checking for the 'END' keyword at all. Instead, we check the return status
    of 'fits_read_keyn' and abort the loop when that status is non-zero. If the
    non-zero status is 'KEY_OUT_BOUNDS' then everything is fine (we have
    finished parsing all the keywords). Otherwise, 'gal_fits_tab_info' aborts
    the Gnuastro program calling it, and reports the unexpected CFITSIO return
    status error message.
    
    [1] Reply of Craig Gordon (from CFITSIO) on this problem:
    
        The reason the fits_read_keyn() call for key number 284 returns an
        empty string is because there really is just a blank line at that
        position.  The END keyword is actually located 1 line after that, at
        what would be keyword position 285.
    
        It is not the usual case to have blank lines in between the last real
        keyword in the header and the END keyword, which is why you haven't run
        into this with other files.  But blank lines are allowed here by the
        FITS standard, so this is still a valid file.  (If new keywords are
        added to the header, CFITSIO may insert them into this blank space.)
    
        The problem you're running into is that while END exists at key position
        285, CFITSIO functions such as fits_read_keyn() and fits_find_nextkey()
        only let you access one position beyond the last real keyword in the 
header
        (which you can find by functions such as fits_get_hdrspace() and
        fits_get_hdrpos()).  So if you tried replacing 284 with 285 in your
        example, fits_read_keyn would return an error.
    
        This brings up the question of whether it is necessary for Gnuastro to
        actually access the END keyword. In any case it's probably not a good
        idea to use the location of the END keyword as the criteria for
        stopping a keyword-reading loop. You could just cut it off at the
        position of the last non-END keyword, as given by fits_get_hdrpos and
        fits_get_hdrspace.  Or you could also loop through all the keys by
        continually calling the fits_find_nextkey function until it returns a
        bad status.
    
    [2] https://lists.gnu.org/archive/html/bug-gnuastro/2022-07/msg00005.html
---
 THANKS                       |  1 +
 doc/announce-acknowledge.txt |  1 +
 lib/fits.c                   | 55 ++++++++++++++++++--------------------------
 3 files changed, 24 insertions(+), 33 deletions(-)

diff --git a/THANKS b/THANKS
index 3e6ac378..51be824a 100644
--- a/THANKS
+++ b/THANKS
@@ -55,6 +55,7 @@ support in Gnuastro. The list is ordered alphabetically (by 
family name).
     Zohreh Ghaffari                      zoh.ghaffari@gmail.com
     Thérèse Godefroy                     godef.th@free.fr
     Giulia Golini                        giulia.golini@gmail.com
+    Craig Gordon                         craig.a.gordon@nasa.gov
     Martin Guerrero Roncel               mar@iaa.es
     Madusha Gunawardhana                 gunawardhana@strw.leidenuniv.nl
     Bruno Haible                         bruno@clisp.org
diff --git a/doc/announce-acknowledge.txt b/doc/announce-acknowledge.txt
index 1097ce77..30b159e9 100644
--- a/doc/announce-acknowledge.txt
+++ b/doc/announce-acknowledge.txt
@@ -5,6 +5,7 @@ Faezeh Bijarchian
 Hilderic Browne
 Sepideh Eskandarlou
 Sílvia Farras
+Craig Gordon
 Teet Kuutma
 Jeremy Lim
 Juan Miro
diff --git a/lib/fits.c b/lib/fits.c
index d26f6e1d..5833912a 100644
--- a/lib/fits.c
+++ b/lib/fits.c
@@ -3028,7 +3028,7 @@ gal_fits_tab_info(char *filename, char *hdu, size_t 
*numcols,
   size_t i, index;
   long long *tzero;
   gal_data_t *allcols;
-  int status=0, datatype, *tscal;
+  int status=0, rkstatus=0, datatype, *tscal;
   char keyname[FLEN_KEYWORD]="XXXXXXXXXXXXX", value[FLEN_VALUE];
 
   /* Necessary when a keyword can't be written immediately as it is read in
@@ -3064,37 +3064,18 @@ gal_fits_tab_info(char *filename, char *hdu, size_t 
*numcols,
           tfields*sizeof *tzero);
 
 
-  /* Read all the keywords one by one and if they match, then put them in
-     the correct value. Note that we are starting from keyword 9 because
-     according to the FITS standard, the first 8 keys in a FITS table are
-     reserved. */
-  for(i=9; strcmp(keyname, "END"); ++i)
+  /* Read all the keywords one by one. If they match any of the standard
+     Table metadata keywords, then put them in the correct value. Some
+     notes about this loop:
+       - We are starting from keyword 9 because according to the FITS
+         standard, the first 8 keys in a FITS table are reserved.
+       - When 'fits_read_keyn' has been able to read the keyword and its
+         value, it will return '0'. The returned value is also stored in
+         'rkstatus' (Read Keyword STATUS), which we need to check after the
+         loop. */
+  i=8;
+  while( fits_read_keyn(fptr, ++i, keyname, value, NULL, &rkstatus) == 0 )
     {
-
-      /* Read the next keyword. */
-      fits_read_keyn(fptr, i, keyname, value, NULL, &status);
-      switch(status)
-        {
-        /* Everything is good, ignore (this is just a place-holder!) */
-        case 0: break;
-
-        /* It can happen that the 'END' keyword is not read by
-           'fits_read_keyn' (see the file in [1] below, with CFITSIO
-           4.1.0). In that case, the loop will pass this keyword and the
-           next call to 'fits_read_key' will return with a 'status' of
-           'KEY_OUT_BOUNDS'. To stop the loop therefore, we'll simply put
-           an 'END' manually and decrement the counter (for any future
-           checks).
-
-           [1] 
https://lists.gnu.org/archive/html/bug-gnuastro/2022-07/msg00005.html*/
-        case KEY_OUT_BOUNDS: --i; sprintf(keyname, "END"); status=0; break;
-
-        /* There is an un-expected problem, abort and let the user know
-           about it. */
-        default: gal_fits_io_error (status, NULL); /* An error. */
-        }
-
-
       /* For string valued keywords, CFITSIO's function above, keeps the
          single quotes around the value string, one before and one
          after. 'gal_fits_key_clean_str_value' will remove these single
@@ -3282,10 +3263,18 @@ gal_fits_tab_info(char *filename, char *hdu, size_t 
*numcols,
             set_display_format(value, &allcols[index], filename, hdu,
                                keyname);
         }
-
-      /* Column zero. */
     }
 
+
+  /* The loop above finishes as soon as the 'status' of 'fits_read_keyn'
+     becomes non-zero. If the status is 'KEY_OUT_BOUNDS', then it shows we
+     have reached the end of the keyword list and there is no
+     problem. Otherwise, there is an unexpected problem and we should abort
+     and inform the user about the problem. */
+  if( rkstatus != KEY_OUT_BOUNDS )
+    gal_fits_io_error(rkstatus, NULL);
+
+
   /* If any columns should be added later because of missing information,
      add them here. */
   if(later_name)