srcfiles.cxx: Addressed all patch review notes. typos, formatting, documentation, show -z and -b option even if libarchive is not available (and adjust test script accordingly), show source files missing from -z option

Message ID 20240126210028.939879-1-halamour@redhat.com
State Superseded
Delegated to: Aaron Merey
Headers
Series srcfiles.cxx: Addressed all patch review notes. typos, formatting, documentation, show -z and -b option even if libarchive is not available (and adjust test script accordingly), show source files missing from -z option |

Commit Message

Housam Alamour Jan. 26, 2024, 9 p.m. UTC
  ---
 doc/srcfiles.1             |  11 +-
 src/srcfiles.cxx           | 304 ++++++++++++++++++-------------------
 tests/run-srcfiles-self.sh |   5 +-
 3 files changed, 161 insertions(+), 159 deletions(-)
  

Comments

Aaron Merey Jan. 29, 2024, 11:45 p.m. UTC | #1
Hi Housam,

On Fri, Jan 26, 2024 at 4:01 PM Housam Alamour <halamour@redhat.com> wrote:
>
> +/* Definitions of arguments for argp functions.  */
>  static const struct argp_option options[] =
>  {
>    { NULL, 0, NULL, OPTION_DOC, N_("Output options:"), 1 },
> @@ -89,58 +89,50 @@ static const struct argp_option options[] =
>    { "verbose", 'v', NULL, 0,
>      N_ ("Increase verbosity of logging messages."), 0 },
>    { "cu-only", 'c', NULL, 0, N_ ("Only list the CU names."), 0 },
> -  #ifdef HAVE_LIBARCHIVE
> -  { "zip", 'z', NULL, 0, N_ ("Zip all the source files and send to stdout."
> -    "Cannot be used with the null option"), 0 },
> -    #ifdef ENABLE_LIBDEBUGINFOD
> -    { "no-backup", 'b', NULL, 0, N_ ("Disables local source file search when "
> -      "debuginfod fails to fetch files. This option is only applicable"
> -      "when fetching and zipping files."), 0 },
> -    #endif
> -  #endif
> +  { "zip", 'z', NULL, 0, N_ ("Zip all the source files and send to stdout. "
> +    "By default, priority is given to fetching source files from "
> +    "debuginfod over local source files"), 0 },
> +  { "no-backup", 'b', NULL, 0, N_ ("Disables local source file search when "
> +    "debuginfod fails to fetch files.  This option is only applicable "
> +    "when fetching and zipping files."), 0 },
>    { NULL, 0, NULL, 0, NULL, 0 }
>  };

I originally suggested keeping -z/-b as valid options even when
srcfiles isn't built with libarchive/libdebuginfod support, but later on
IRC we decided to keep -z/-b conditional on building srcfiles with
the necessary support. I think these #ifdef might have been removed
by mistake.

Also when you post a revised patch, it's a bit easier for me if you just
post the current definitive patch with all commits squashed.  I'll just diff
your patches if I want to compare changes since the last revision!

Aaron
  

Patch

diff --git a/doc/srcfiles.1 b/doc/srcfiles.1
index a7cde664..c6338315 100644
--- a/doc/srcfiles.1
+++ b/doc/srcfiles.1
@@ -83,9 +83,16 @@  Print program version.
 \fB\-0, \-\-null\fR
 Separate items by a null instead of a newline.
 
+.TP
+\fB\-b, \-\-no-backup\fR
+Disables local source file search when 
+debuginfod fails to fetch files.  This 
+option is only applicable when fetching and 
+zipping files.
+
 .TP
 \fB\-c, \-\-cu\-only\fR
-Only list the (compilation unit) CU names.
+Only list the CU (compilation unit) names.
 
 .TP
 \fB\-v, \-\-verbose\fR
@@ -94,7 +101,7 @@  Increase verbosity of logging messages.
 .TP
 \fB\-z, \-\-zip\fR
 Zip all the source files and send to stdout. 
-Files may be automatically fetched by 
+By default, files will be automatically fetched by 
 debuginfod (if applicable) or locally as a 
 backup. Any source files that were not found 
 will not be archived.
diff --git a/src/srcfiles.cxx b/src/srcfiles.cxx
index 0e4f756f..24dc2a30 100644
--- a/src/srcfiles.cxx
+++ b/src/srcfiles.cxx
@@ -20,11 +20,11 @@ 
 /* In case we have a bad fts we include this before config.h because it
    can't handle _FILE_OFFSET_BITS.
    Everything we need here is fine if its declarations just come first.
-   Also, include sys/types.h before fts. On some systems fts.h is not self
-   contained. */
+   Also, include sys/types.h before fts.  On some systems fts.h is not self
+   contained.  */
 #ifdef BAD_FTS
-  #include <sys/types.h>
-  #include <fts.h>
+#include <sys/types.h>
+#include <fts.h>
 #endif
 
 #ifdef HAVE_CONFIG_H
@@ -42,7 +42,7 @@ 
 #include <memory>
 
 #ifdef ENABLE_LIBDEBUGINFOD
-  #include "debuginfod.h"
+#include "debuginfod.h"
 #endif
 
 #include <libdwfl.h>
@@ -54,17 +54,17 @@ 
 
 /* Libraries for use by the --zip option */
 #ifdef HAVE_LIBARCHIVE
-  #include <archive.h>
-  #include <archive_entry.h>
+#include <archive.h>
+#include <archive_entry.h>
 #endif
 
 /* If fts.h is included before config.h, its indirect inclusions may not
-   give us the right LFS aliases of these functions, so map them manually. */
+   give us the right LFS aliases of these functions, so map them manually.  */
 #ifdef BAD_FTS
-  #ifdef _FILE_OFFSET_BITS
-    #define open open64
-    #define fopen fopen64
-  #endif
+#ifdef _FILE_OFFSET_BITS
+#define open open64
+#define fopen fopen64
+#endif
 #else
   #include <sys/types.h>
   #include <fts.h>
@@ -72,15 +72,15 @@ 
 
 using namespace std;
 
-/* Name and version of program. */
+/* Name and version of program.  */
 ARGP_PROGRAM_VERSION_HOOK_DEF = print_version;
 
-/* Bug report address. */
+/* Bug report address.  */
 ARGP_PROGRAM_BUG_ADDRESS_DEF = PACKAGE_BUGREPORT;
 
 constexpr size_t BUFFER_SIZE = 8192;
 
-/* Definitions of arguments for argp functions. */
+/* Definitions of arguments for argp functions.  */
 static const struct argp_option options[] = 
 {
   { NULL, 0, NULL, OPTION_DOC, N_("Output options:"), 1 },
@@ -89,58 +89,50 @@  static const struct argp_option options[] =
   { "verbose", 'v', NULL, 0,
     N_ ("Increase verbosity of logging messages."), 0 },
   { "cu-only", 'c', NULL, 0, N_ ("Only list the CU names."), 0 },
-  #ifdef HAVE_LIBARCHIVE
-  { "zip", 'z', NULL, 0, N_ ("Zip all the source files and send to stdout."
-    "Cannot be used with the null option"), 0 },
-    #ifdef ENABLE_LIBDEBUGINFOD
-    { "no-backup", 'b', NULL, 0, N_ ("Disables local source file search when " 
-      "debuginfod fails to fetch files. This option is only applicable"
-      "when fetching and zipping files."), 0 },
-    #endif
-  #endif
+  { "zip", 'z', NULL, 0, N_ ("Zip all the source files and send to stdout. "
+    "By default, priority is given to fetching source files from "
+    "debuginfod over local source files"), 0 },
+  { "no-backup", 'b', NULL, 0, N_ ("Disables local source file search when " 
+    "debuginfod fails to fetch files.  This option is only applicable "
+    "when fetching and zipping files."), 0 },
   { NULL, 0, NULL, 0, NULL, 0 } 
 };
 
-/* Short description of program. */
-static const char doc[] = N_("Lists the source files of a DWARF/ELF file. The default input is the file 'a.out'.");
+/* Short description of program.  */
+static const char doc[] = N_("Lists the source files of a DWARF/ELF file.  The default input is the file 'a.out'.");
 
-/* Strings for arguments in help texts. */
+/* Strings for arguments in help texts.  */
 static const char args_doc[] = N_("INPUT");
 
-/* Prototype for option handler. */
+/* Prototype for option handler.  */
 static error_t parse_opt (int key, char *arg, struct argp_state *state);
 
-static struct argp_child argp_children[2]; /* [0] is set in main. */
+static struct argp_child argp_children[2]; /* [0] is set in main.  */
 
-/* Data structure to communicate with argp functions. */
+/* Data structure to communicate with argp functions.  */
 static const struct argp argp =
 {
   options, parse_opt, args_doc, doc, argp_children, NULL, NULL
 };
 
-/* Verbose message printing. */
+/* Verbose message printing.  */
 static bool verbose;
-/* Delimit the output with nulls. */
+/* Delimit the output with nulls.  */
 static bool null_arg;
-/* Only print compilation unit names. */
+/* Only print compilation unit names.  */
 static bool CU_only;
-#ifdef HAVE_LIBARCHIVE
-  /* Zip all the source files and send to stdout. */
-  static bool zip;
-
-  #ifdef ENABLE_LIBDEBUGINFOD
-    /* Disables local source file search when debuginfod fails to fetch them. 
-       This option is only applicable when fetching and zipping files.*/
-    static bool no_backup;
-  #endif
-#endif
-
-/* Handle program arguments. Note null arg and zip 
-    cannot be combined due to warnings raised when unzipping. */
+/* Zip all the source files and send to stdout.  */
+static bool zip;
+/* Disables local source file search when debuginfod fails to fetch them.  
+    This option is only applicable when fetching and zipping files.*/
+static bool no_backup;
+
+/* Handle program arguments.  Note null arg and zip 
+    cannot be combined due to warnings raised when unzipping.  */
 static error_t
 parse_opt (int key, char *arg, struct argp_state *state)
 {
-  /* Suppress "unused parameter" warning. */
+  /* Suppress "unused parameter" warning.  */
   (void)arg;
   switch (key)
     {
@@ -179,18 +171,18 @@  parse_opt (int key, char *arg, struct argp_state *state)
 }
 
 /* Remove the "/./" , "../" and the preceding directory 
-    that some paths include which raise errors during unzip. */
+    that some paths include which raise errors during unzip.  */
 string canonicalize_path(string path) 
 {
     stringstream ss(path);
     string token;
     vector<string> tokens;
-    /* Extract each directory of the path and place into a vector. */
+    /* Extract each directory of the path and place into a vector.  */
     while (getline(ss, token, '/')) {
-      /* Ignore any empty //, or /./ dirs. */
+      /* Ignore any empty //, or /./ dirs.  */
         if (token == "" || token == ".")
             continue;
-      /* When /.. is encountered, remove the most recent directory from the vector. */
+      /* When /..  is encountered, remove the most recent directory from the vector.  */
         else if (token == "..") {
             if (!tokens.empty())
                 tokens.pop_back();
@@ -200,17 +192,17 @@  string canonicalize_path(string path)
     stringstream result;
     if (tokens.empty())
         return "/";
-    /* Reconstruct the path from the extracted directories. */
+    /* Reconstruct the path from the extracted directories.  */
     for (const string &t : tokens) {
         result << '/' << t;
     }
     return result.str();
 }
 
-/* Global list of collected source files and their respective module.  
+/* Global list of collected source files and their respective module.
    Normally, it'll contain the sources of just one named binary, but 
    the '-K' option can cause multiple dwfl modules to be loaded, thus 
-   listed. */
+   listed.  */
 set<pair<string, Dwfl_Module*>> debug_sourcefiles;
 
 static int
@@ -221,14 +213,14 @@  collect_sourcefiles (Dwfl_Module *dwflmod,
                      void *arg __attribute__ ((unused)))
 {
   Dwarf *dbg;
-  Dwarf_Addr bias; /* ignored - for addressing purposes only. */
+  Dwarf_Addr bias; /* ignored - for addressing purposes only.  */
   
   dbg = dwfl_module_getdwarf (dwflmod, &bias);
 
   Dwarf_Off offset = 0;
   Dwarf_Off old_offset;
   size_t hsize;
-  /* Traverse all CUs of this module. */
+  /* Traverse all CUs of this module.  */
   while (dwarf_nextcu (dbg, old_offset = offset, &offset, &hsize, NULL, NULL, NULL) == 0)
     {
       Dwarf_Die cudie_mem;
@@ -243,7 +235,7 @@  collect_sourcefiles (Dwfl_Module *dwflmod,
       if (dwarf_getsrcfiles (cudie, &files, &nfiles) != 0)
         continue;
 
-      /* extract DW_AT_comp_dir to resolve relative file names. */
+      /* extract DW_AT_comp_dir to resolve relative file names.  */
       const char *comp_dir = "";
       const char *const *dirs;
       size_t ndirs;
@@ -261,7 +253,7 @@  collect_sourcefiles (Dwfl_Module *dwflmod,
       if (comp_dir[0] == '\0' && cuname[0] != '/')
         {
           /* This is a common symptom for dwz-compressed debug files,
-             where the altdebug file cannot be resolved. */
+             where the altdebug file cannot be resolved.  */
           if (verbose)
             clog << "skipping cu=" << cuname << " due to empty comp_dir" << endl;
           continue;
@@ -304,112 +296,118 @@  collect_sourcefiles (Dwfl_Module *dwflmod,
 }
 
 #ifdef HAVE_LIBARCHIVE
-  void zip_files()
+void zip_files()
+{
+  struct archive *a = archive_write_new();
+  struct stat st;
+  char buff[BUFFER_SIZE];
+  int len;
+  int fd;
+  #ifdef ENABLE_LIBDEBUGINFOD
+  /* Initialize a debuginfod client.  */
+  static unique_ptr <debuginfod_client, void (*)(debuginfod_client*)>
+    client (debuginfod_begin(), &debuginfod_end);
+  #endif
+
+  archive_write_set_format_zip(a);
+  archive_write_open_fd(a, STDOUT_FILENO);
+
+  int missing_files = 0;
+  for (const auto &pair : debug_sourcefiles)
   {
-    struct archive *a = archive_write_new();
-    struct stat st;
-    char buff[BUFFER_SIZE];
-    int len;
-    int fd;
+    fd = -1;
+    const std::string &file_path = pair.first;
+
+    /* Attempt to query debuginfod client to fetch source files.  */
     #ifdef ENABLE_LIBDEBUGINFOD
-      /* Initialize a debuginfod client. */
-      static unique_ptr <debuginfod_client, void (*)(debuginfod_client*)>
-        client (debuginfod_begin(), &debuginfod_end);
+    Dwfl_Module* dwflmod = pair.second;
+    /* Obtain source file's build ID.  */
+    const unsigned char *bits;
+    GElf_Addr vaddr;
+    int bits_length = dwfl_module_build_id(dwflmod, &bits, &vaddr);
+    /* Ensure successful client and build ID acquisition.  */
+    if (client.get() != NULL && bits_length > 0)
+    {
+      fd = debuginfod_find_source(client.get(),
+                                    bits, bits_length, 
+                                    file_path.c_str(), NULL);
+    }
+    else
+    {
+        if (client.get() == NULL)
+            cerr << "Error: Failed to initialize debuginfod client." << endl;
+        else
+            cerr << "Error: Invalid build ID length (" << bits_length << ")." << endl;
+    }
     #endif
 
-    archive_write_set_format_zip(a);
-    archive_write_open_fd(a, STDOUT_FILENO);
-
-    int missing_files = 0;
-    for (const auto &pair : debug_sourcefiles)
+    if (!no_backup)
+      /* Files could not be located using debuginfod, search locally */
+      if (fd < 0)
+        fd = open(file_path.c_str(), O_RDONLY);
+    if (fd < 0)
     {
-      fd = -1;
-      const std::string &file_path = pair.first;
+      if (verbose)
+        cerr << file_path << endl;
+      missing_files++;
+      continue;
+    }
 
-      /* Attempt to query debuginfod client to fetch source files. */
-      #ifdef ENABLE_LIBDEBUGINFOD
-        Dwfl_Module* dwflmod = pair.second;
-        /* Obtain source file's build ID. */
-        const unsigned char *bits;
-        GElf_Addr vaddr;
-        int bits_length = dwfl_module_build_id(dwflmod, &bits, &vaddr);
-        /* Ensure successful client and build ID acquisition. */
-        if (client.get() != NULL && bits_length > 0)
-        {
-          fd = debuginfod_find_source(client.get(),
-                                        bits, bits_length, 
-                                        file_path.c_str(), NULL);
-        }
-        else
-        {
-            if (client.get() == NULL)
-                cerr << "Error: Failed to initialize debuginfod client." << endl;
-            else
-                cerr << "Error: Invalid build ID length (" << bits_length << ")." << endl;
-        }
-      
-      #endif
-      if (!no_backup)
-        /* Files could not be located using debuginfod, search locally */
-        if (fd < 0)
-          fd = open(file_path.c_str(), O_RDONLY);
-      if (fd < 0)
-      {
-        missing_files++;
-        continue;
-      }
+    /* Create an entry for each file including file information to be placed in the zip.  */
+    if (fstat(fd, &st) == -1)
+    {
+      if (verbose)
+        cerr << file_path << endl;
+      missing_files++;
+      if (verbose)
+        cerr << "Error: Failed to get file status for " << file_path << ": " << strerror(errno) << endl;
+      continue;
+    }
+    struct archive_entry *entry = archive_entry_new();
+    /* Removing first "/"" to make the path "relative" before zipping, otherwise warnings are raised when unzipping.  */
+    string entry_name = file_path.substr(file_path.find_first_of('/') + 1);
+    archive_entry_set_pathname(entry, entry_name.c_str());
+    archive_entry_copy_stat(entry, &st);
+    if (archive_write_header(a, entry) != ARCHIVE_OK)
+    {
+      if (verbose)
+        cerr << file_path << endl;
+      missing_files++;
+      if (verbose)
+        cerr << "Error: failed to write header for " << file_path << ": " << archive_error_string(a) << endl;
+      continue;
+    }
 
-      /* Create an entry for each file including file information to be placed in the zip. */
-      if (fstat(fd, &st) == -1)
-      {
-        missing_files++;
-        if (verbose)
-          cerr << "Error: Failed to get file status for " << file_path << ": " << strerror(errno) << endl;
-        continue;
-      }
-      struct archive_entry *entry = archive_entry_new();
-      /* Removing first "/"" to make the path "relative" before zipping, otherwise warnings are raised when unzipping. */
-      string entry_name = file_path.substr(file_path.find_first_of('/') + 1);
-      archive_entry_set_pathname(entry, entry_name.c_str());
-      archive_entry_copy_stat(entry, &st);
-      if (archive_write_header(a, entry) != ARCHIVE_OK)
+    /* Write the file to the zip.  */
+    len = read(fd, buff, sizeof(buff));
+    if (len == -1)
+    {
+      if (verbose)
+        cerr << file_path << endl;
+      missing_files++;
+      if (verbose)
+        cerr << "Error: Failed to open file: " << file_path << ": " << strerror(errno) <<endl;
+      continue;
+    }
+    while (len > 0)
+    {
+      if (archive_write_data(a, buff, len) < ARCHIVE_OK)
       {
-        missing_files++;
         if (verbose)
-          cerr << "Error: failed to write header for " << file_path << ": " << archive_error_string(a) << endl;
-        continue;
+          cerr << "Error: Failed to read from the file: " << file_path << ": " << strerror(errno) << endl;
+        break; 
       }
-
-      /* Write the file to the zip. */
       len = read(fd, buff, sizeof(buff));
-      if (len == -1)
-      {
-        missing_files++;
-        if (verbose)
-          cerr << "Error: Failed to open file: " << file_path << ": " << strerror(errno) <<endl;
-        continue;
-      }
-      if (verbose && len > 0)
-        clog << "Writing to zip: " << file_path << endl;
-      while (len > 0)
-      {
-        if (archive_write_data(a, buff, len) < ARCHIVE_OK)
-        {
-          if (verbose)
-            cerr << "Error: Failed to read from the file: " << file_path << ": " << strerror(errno) << endl;
-          break; 
-        }
-        len = read(fd, buff, sizeof(buff));
-      }
-      close(fd);
-      archive_entry_free(entry);
     }
-    if (verbose && missing_files > 0 )
-      cerr << "WARNING: " << missing_files << " files could not be found." << endl;
-
-    archive_write_close(a);
-    archive_write_free(a);
+    close(fd);
+    archive_entry_free(entry);
   }
+  if (verbose && missing_files > 0 )
+    cerr << missing_files << " file(s) listed above could not be found.  " << endl;
+
+  archive_write_close(a);
+  archive_write_free(a);
+}
 #endif
 
 int
@@ -417,14 +415,14 @@  main (int argc, char *argv[])
 {
   int remaining;
   
-  /* Parse and process arguments.  This includes opening the modules. */
+  /* Parse and process arguments.  This includes opening the modules.  */
   argp_children[0].argp = dwfl_standard_argp ();
   argp_children[0].group = 1;
   
   Dwfl *dwfl = NULL;
   (void) argp_parse (&argp, argc, argv, 0, &remaining, &dwfl);
   assert (dwfl != NULL);
-  /* Process all loaded modules - probably just one, except if -K or -p is used. */
+  /* Process all loaded modules - probably just one, except if -K or -p is used.  */
   (void) dwfl_getmodules (dwfl, &collect_sourcefiles, NULL, 0);
 
   if (!debug_sourcefiles.empty ())
diff --git a/tests/run-srcfiles-self.sh b/tests/run-srcfiles-self.sh
index 1ee460f6..afbb1e77 100755
--- a/tests/run-srcfiles-self.sh
+++ b/tests/run-srcfiles-self.sh
@@ -17,9 +17,6 @@ 
 
 . $srcdir/debuginfod-subr.sh
 
-# prereqs
-type unzip 2>/dev/null || exit 77
-
 # for test case debugging, uncomment:
 # set -x
 set -e
@@ -37,7 +34,7 @@  testrun $ET_EXEC -e $ET_EXEC | grep $SRC_NAME > /dev/null
 
 # Check if zip option is available (only available if libarchive is available. 
 #   Debuginfod optional to fetch source files from debuginfod federation.)
-$ET_EXEC --help | grep -q zip && zip=true || zip=false
+$ET_EXEC --help | grep -q zip && command -v unzip >/dev/null 2>&1 && zip=true || zip=false
 
 for null_arg in --null ""; do
   for verbose_arg in --verbose ""; do