[gnuastro-commits] master 23a541a 056/125: Single correction of string keyword values in FITS tables

[Mohammad Akhlaghi]

branch: master
commit 23a541ac58b7c68f677cb9fc6132ae8ad4349626
Author: Mohammad Akhlaghi <address@hidden>
Commit: Mohammad Akhlaghi <address@hidden>

    Single correction of string keyword values in FITS tables
    
    Unfortunately the CFITSIO functions that return the value of a string
    keyword do so with the single quotation marks before and after the
    string. This can cause problems when we need to read the string as in
    `gal_fits_table_info'. Until now, for each keyword, this correction was
    made separately, but now, when the first character of the keyword value is
    a single quote('), then the string will be corrected, so no matter what the
    option is, a unified (trust-worthy) pointer (`val') can be used.
    
    Another important issue that was found by running `make check' in the
    previous commit was a `double free' crash. The cause was that when
    `gal_data_string_to_type' couldn't read a value, it would free the
    allocated array, but it wouldn't reset it to NULL. So later steps would
    also try to free it. This is directly related to the paragraph above:
    because of the single quotation marks, `TNULLn' keywords in FITS ASCII
    tables couldn't be read and because of this issue, the check would fail.
    
    During this process the following corrections were also made:
    
     - The `ui_read_check_options' functions in each program's `ui.c' will be
       in charge of putting the options into the main program's data structure
       and also doing the sanity checks. So the old `sanitycheck' function in
       `ui.c' is no longer needed.
    
     - The old `ui_preparations' option will be called `ui_preparations' from
       now on.
---
 bin/table/args.h |  4 ----
 bin/table/ui.c   | 59 +++++++++++++++++++++++++++++++++++++-------------------
 lib/data.c       |  9 +++++----
 lib/fits.c       | 58 ++++++++++++++++++++++++++++---------------------------
 lib/table.c      |  6 +++---
 5 files changed, 77 insertions(+), 59 deletions(-)

diff --git a/bin/table/args.h b/bin/table/args.h
index 17f45a2..7a69fbb 100644
--- a/bin/table/args.h
+++ b/bin/table/args.h
@@ -81,10 +81,6 @@ enum option_keys_enum
   /* Only with long version. */
 };
 
-
-
-
-
 /* Array of acceptable options. */
 struct argp_option options[] =
   {
diff --git a/bin/table/ui.c b/bin/table/ui.c
index f00f70f..3e6bb13 100644
--- a/bin/table/ui.c
+++ b/bin/table/ui.c
@@ -77,8 +77,23 @@ ui_option_is_mandatory(char *name)
 
 
 
+/* This function is in charge of reading the option values from the arrays
+   of the `argp_option' structure and put them into each program's own data
+   structure.
+
+   IMPORTANT: DO NOT USE THE SAME POINTERS. When the types are pointers
+   (like stirings or linked lists), do not use the same pointers, allocate
+   and keep a new copy. This is because the printing of options will take
+   place after this sanity check, so users can be sure that the values that
+   are printed and used as configuration files later have no non-sane
+   values. Thus the poitners in `argp_option' will be freed after printing,
+   but the program's pointers must stay to the end.
+
+   After setting all the values, do other forms of sanity checks that
+   involve more than one option.
+*/
 void
-fill_params_from_options(struct tableparams *p)
+ui_read_check_options(struct tableparams *p)
 {
   size_t i;
 
@@ -136,17 +151,20 @@ fill_params_from_options(struct tableparams *p)
           error(EXIT_FAILURE, 0, "option key %d not recognized in "
                 "`fill_params_from_options'", options[i].key);
         }
-}
-
-
-
-
-
-void
-sanitycheck(struct tableparams *p)
-{
 
+  /* Make sure an input file name was given and if it was a FITS file, that
+     a HDU is also given. */
+  if(p->up.filename)
+    {
+      if( gal_fits_name_is_fits(p->up.filename) && p->cp.hdu==NULL )
+        error(EXIT_FAILURE, 0, "no HDU specified. When the input is a FITS "
+              "file, a HDU must also be specified, you can use the `--hdu' "
+              "(`-h') option and give it the HDU number (starting from "
+              "zero), extension name, or anything acceptable by CFITSIO");
 
+    }
+  else
+    error(EXIT_FAILURE, 0, "no input file is specified");
 }
 
 
@@ -172,7 +190,7 @@ sanitycheck(struct tableparams *p)
 /***************       Preparations         *******************/
 /**************************************************************/
 void
-preparearrays(struct tableparams *p)
+ui_preparations(struct tableparams *p)
 {
   char *numstr;
   int tabletype;
@@ -191,6 +209,7 @@ preparearrays(struct tableparams *p)
       if(allcols==NULL)
         error(EXIT_FAILURE, 0, "%s: no usable data rows", p->up.filename);
 
+
       /* If the user just wanted information, then print it. */
       if(p->information)
         {
@@ -206,13 +225,17 @@ preparearrays(struct tableparams *p)
           gal_table_print_info(allcols, numcols, numrows);
         }
 
+
       /* Free the information from all the columns. */
       for(i=0;i<numcols;++i)
         gal_data_free(&allcols[i], 1);
       free(allcols);
 
-      /* Add the number of columns to the list if the user wanted to print
-         the columns (didn't just want their information. */
+
+      /* If the user just wanted information, then free the allocated
+         spaces and exit. Otherwise, add the number of columns to the list
+         if the user wanted to print the columns (didn't just want their
+         information. */
       if(p->information)
         {
           freeandreport(p);
@@ -289,12 +312,8 @@ setparams(int argc, char *argv[], struct tableparams *p)
   gal_options_config_files(PROG_EXEC, PROG_NAME, options,
                            gal_commonopts_options, &p->cp);
 
-  /* Fill the parameters from the options. */
-  fill_params_from_options(p);
-
-
-  /* Do a sanity check. */
-  sanitycheck(p);
+  /* Fill the parameters from the options and check their values. */
+  ui_read_check_options(p);
 
   /* Print the necessary information if asked. Note that this needs to be
      done after the sanity check so un-sane values are not printed in the
@@ -303,7 +322,7 @@ setparams(int argc, char *argv[], struct tableparams *p)
                           gal_commonopts_options);
 
   /* Read/allocate all the necessary starting arrays */
-  preparearrays(p);
+  ui_preparations(p);
 
   /* Free all the allocated spaces in the option structures. */
   gal_options_free(options);
diff --git a/lib/data.c b/lib/data.c
index 4e39dc2..48aa615 100644
--- a/lib/data.c
+++ b/lib/data.c
@@ -835,7 +835,6 @@ gal_data_free(gal_data_t *data, int only_contents)
   if(data->wcs)     wcsfree(data->wcs);
   if(data->comment) free(data->comment);
 
-
   /* If the data type is string, then each element in the array is actually
      a pointer to the array of characters, so free them before freeing the
      actual array. */
@@ -858,7 +857,6 @@ gal_data_free(gal_data_t *data, int only_contents)
   else
     if(data->array) free(data->array);
 
-
   /* Finally, free the actual data structure. */
   if(only_contents==0)
     free(data);
@@ -2155,7 +2153,7 @@ gal_data_string_to_type(void **out, char *string, int 
type)
   /* If the output is NULL, then allocate the necessary space if we are not
      dealing with a linked list. In a linked list, a NULL value is
      meaningful (it is the end of the list). */
-  if(*out==NULL && gal_data_is_linked_list(type)==0)
+  if( *out==NULL && !gal_data_is_linked_list(type) )
     {
       allocated=1;
       *out=gal_data_malloc_array(type, 1);
@@ -2231,6 +2229,9 @@ gal_data_string_to_type(void **out, char *string, int 
type)
   /* If reading was unsuccessful, then free the space if it was allocated,
      then return the status. */
   if(status && allocated)
-    free(value);
+    {
+      free(*out);
+      *out=NULL;
+    }
   return status;
 }
diff --git a/lib/fits.c b/lib/fits.c
index 693b410..676a7f3 100644
--- a/lib/fits.c
+++ b/lib/fits.c
@@ -1633,7 +1633,9 @@ remove_trailing_space(char *str)
   size_t i;
 
   /* Start from the second last character (the last is a single quote) and
-     go down until you hit a non-space character. */
+     go down until you hit a non-space character. This will also work when
+     there is no space characters between the last character of the value
+     and ending single-quote: it will be set to '\0' after this loop. */
   for(i=strlen(str)-2;i>0;--i)
     if(str[i]!=' ')
       break;
@@ -1795,7 +1797,7 @@ gal_fits_table_info(char *filename, char *hdu, size_t 
*numcols,
   long long *tzero;
   gal_data_t *allcols;
   int status=0, datatype, *tscal;
-  char keyname[FLEN_KEYWORD]="XXXXXXXXXXXXX", value[FLEN_VALUE];
+  char keyname[FLEN_KEYWORD]="XXXXXXXXXXXXX", value[FLEN_VALUE], *val;
 
   /* Open the FITS file and get the basic information. */
   fptr=gal_fits_read_hdu(filename, hdu, 1);
@@ -1831,6 +1833,17 @@ gal_fits_table_info(char *filename, char *hdu, size_t 
*numcols,
       /* Read the next keyword. */
       fits_read_keyn(fptr, i, keyname, value, NULL, &status);
 
+      /* For string valued keywords, CFITSIO's function above, keeps the
+         single quotes around the value string, one before and one
+         after. The latter single-quote will be automatically be removed
+         with the `remove_trailign_space' function.*/
+      if(value[0]=='\'')
+        {
+          val=value+1;
+          remove_trailing_space(val);
+        }
+      else val=value;
+
       /* COLUMN DATA TYPE. According the the FITS standard, the value of
          TFORM is most generally in this format: `rTa'. `T' is actually a
          code of the datatype. `r' is the `repeat' counter and `a' is
@@ -1849,29 +1862,28 @@ gal_fits_table_info(char *filename, char *hdu, size_t 
*numcols,
           index = strtoul(&keyname[5], &tailptr, 10) - 1;
           if(index<tfields)     /* Counting from zero was corrected above. */
             {
-              /* Remove the ending trailing space and quotation sign. */
-              remove_trailing_space(value);
+              /* The FITS standard's value to this option for FITS ASCII
+                 and binary files differ. */
               if(*tabletype==GAL_TABLE_TYPE_AFITS)
-                fits_ascii_tform(&value[1], &datatype, NULL, NULL, &status);
+                fits_ascii_tform(val, &datatype, NULL, NULL, &status);
               else
-                fits_binary_tform(&value[1], &datatype, &repeat, NULL,
-                                  &status);
+                fits_binary_tform(val, &datatype, &repeat, NULL, &status);
 
               /* Write the type into the data structure. */
               allcols[index].type=gal_fits_datatype_to_type(datatype);
 
               /* If we are dealing with a string type, we need to know the
-                 number of bytes in both cases. */
+                 number of bytes in both cases for printing later. */
               if( allcols[index].type==GAL_DATA_TYPE_STRING )
                 {
                   if(*tabletype==GAL_TABLE_TYPE_AFITS)
                     {
-                      repeat=strtol(&value[2], &tailptr, 0);
+                      repeat=strtol(val+1, &tailptr, 0);
                       if(*tailptr!='\0')
                         error(EXIT_FAILURE, 0, "%s (hdu: %s): the value to "
                               "keyword `%s' (`%s') is not in `Aw' format "
                               "(for strings) as required by the FITS "
-                              "standard", filename, hdu, keyname, &value[1]);
+                              "standard", filename, hdu, keyname, val);
                     }
                   allcols[index].disp_width=repeat;
                 }
@@ -1884,11 +1896,11 @@ gal_fits_table_info(char *filename, char *hdu, size_t 
*numcols,
           index = strtoul(&keyname[5], &tailptr, 10) - 1;
           if(index<tfields)
             {
-              tscal[index]=strtol(value, &tailptr, 0);
+              tscal[index]=strtol(val, &tailptr, 0);
               if(*tailptr!='\0')
                 error(EXIT_FAILURE, 0, "%s (hdu: %s): value to %s keyword "
                       "(`%s') couldn't be read as a number", filename, hdu,
-                      keyname, value);
+                      keyname, val);
             }
         }
 
@@ -1898,11 +1910,11 @@ gal_fits_table_info(char *filename, char *hdu, size_t 
*numcols,
           index = strtoul(&keyname[5], &tailptr, 10) - 1;
           if(index<tfields)
             {
-              tzero[index]=strtoll(value, &tailptr, 0);
+              tzero[index]=strtoll(val, &tailptr, 0);
               if(*tailptr!='\0')
                 error(EXIT_FAILURE, 0, "%s (hdu: %s): value to %s keyword "
                       "(`%s') couldn't be read as a number", filename, hdu,
-                      keyname, value);
+                      keyname, val);
             }
         }
 
@@ -1915,19 +1927,15 @@ gal_fits_table_info(char *filename, char *hdu, size_t 
*numcols,
         {
           index = strtoul(&keyname[5], &tailptr, 10) - 1;
           if(index<tfields)
-            {
-              remove_trailing_space(value);
-              gal_checkset_allocate_copy(&value[1], &allcols[index].name);
-            }
+            gal_checkset_allocate_copy(val, &allcols[index].name);
         }
 
       /* COLUMN UNITS. */
       else if(strncmp(keyname, "TUNIT", 5)==0)
         {
-          remove_trailing_space(value);
           index = strtoul(&keyname[5], &tailptr, 10) - 1;
           if(index<tfields)
-            gal_checkset_allocate_copy(&value[1], &allcols[index].unit);
+            gal_checkset_allocate_copy(val, &allcols[index].unit);
         }
 
       /* COLUMN COMMENTS */
@@ -1935,10 +1943,7 @@ gal_fits_table_info(char *filename, char *hdu, size_t 
*numcols,
         {
           index = strtoul(&keyname[5], &tailptr, 10) - 1;
           if(index<tfields)
-            {
-              remove_trailing_space(value);
-              gal_checkset_allocate_copy(&value[1], &allcols[index].comment);
-            }
+            gal_checkset_allocate_copy(val, &allcols[index].comment);
         }
 
       /* COLUMN BLANK VALUE. Note that to interpret the blank value the
@@ -1956,10 +1961,7 @@ gal_fits_table_info(char *filename, char *hdu, size_t 
*numcols,
                         "blank value cannot be deduced", filename, hdu,
                         keyname, index+1);
               else
-                {
-                  remove_trailing_space(value);
-                  gal_table_read_blank(&allcols[index], value);
-                }
+                gal_table_read_blank(&allcols[index], val);
             }
         }
 
diff --git a/lib/table.c b/lib/table.c
index bda2f62..586c324 100644
--- a/lib/table.c
+++ b/lib/table.c
@@ -360,11 +360,11 @@ gal_table_read_blank(gal_data_t *col, char *blank)
           "to the array in the data structure passed "
           "`to gal_table_read_blank' must be zero");
 
-  /* Read the blank value as the given type, If successful, then
-     `gal_data_string_to_type) will return 0. In that case, we need to
+  /* Read the blank value as the given type. If successful, then
+     `gal_data_string_to_type' will return 0. In that case, we need to
      initialize the necessary paramters to read this data structure
      correctly. */
-  if(gal_data_string_to_type(&(col->array), blank, col->type)==0)
+  if( !gal_data_string_to_type(&(col->array), blank, col->type) )
     {
       errno=0;
       col->dsize=malloc(sizeof *col->dsize);