Re: [PATCH] abipkgdiff should properly report missing files

Re: [PATCH] abipkgdiff should properly report missing files

[Dodji Seketeli]

Ondrej Oprala <ondrej.oprala@gmail.com> a écrit:

> Subject: [PATCH] Properly report missing files for abipkgdiff
>
> Currently, if abipkgdiff is given a path to a nonexistent file,
> it propagates all the way until package classification
> and ends up being reported as "PKG should be a valid package file",
> which doesn't hint it's not there at all.
>
>         * tools/abipkgdiff.cc: (class options): Add the "nonexistent_file" flag
>         (parse_command_line): Check if the files given exist.
>         (main): Check the nonexistent_file flag. If any of the input
>         files don't exist, report it and exit.

This is OK.  But I'd like a regrestion test to be added for this.  That is,
please add a new field to the in_out_specs array in
tests/test-diff-pkg.cc.  The would contain paths to non-existing input
RPMs as well as reference output reports which contain the what
"abipkgdiff non-existing1.rpm non-existing2.rpm" would have emitted.

OK to commit to master with that change.

Thanks!

Cheers,

-- 
		Dodji

[PATCH] abipkgdiff should properly report missing files

[Ondrej Oprala]

[-- Attachment #1: Type: text/plain, Size: 381 bytes --]

Hi,

this patch is based on our discussion on the IRC channel from today.

Basically, when abipkgdiff is given a non-existent path for a package, 
the logic will let it pass and just mark it as an unknown kind of 
package, giving the vague warning "<PKG> should be a valid package 
file". This patch ensures we properly diagnose the issue and quit 
prematurely.

Cheers,

Ondrej



[-- Attachment #2: 0001-Properly-report-missing-files-for-abipkgdiff.patch --]
[-- Type: text/x-patch, Size: 2995 bytes --]

From 20ca3d5e670877bcba57fefb791b05303d4ab1e0 Mon Sep 17 00:00:00 2001
From: Ondrej Oprala <ondrej.oprala@gmail.com>
Date: Mon, 12 Dec 2016 16:27:15 +0100
Subject: [PATCH] Properly report missing files for abipkgdiff

Currently, if abipkgdiff is given a path to a nonexistent file,
it propagates all the way until package classification
and ends up being reported as "PKG should be a valid package file",
which doesn't hint it's not there at all.

        * tools/abipkgdiff.cc: (class options): Add the "nonexistent_file" flag
        (parse_command_line): Check if the files given exist.
        (main): Check the nonexistent_file flag. If any of the input
        files don't exist, report it and exit.

Signed-off-by: Ondrej Oprala <ondrej.oprala@gmail.com>
---
 tools/abipkgdiff.cc | 27 +++++++++++++++++++++++++--
 1 file changed, 25 insertions(+), 2 deletions(-)

diff --git a/tools/abipkgdiff.cc b/tools/abipkgdiff.cc
index 0999141..a00073f 100644
--- a/tools/abipkgdiff.cc
+++ b/tools/abipkgdiff.cc
@@ -162,6 +162,7 @@ public:
   bool		display_usage;
   bool		display_version;
   bool		missing_operand;
+  bool		nonexistent_file;
   bool		abignore;
   bool		parallel;
   string	package1;
@@ -188,6 +189,7 @@ public:
       display_usage(),
       display_version(),
       missing_operand(),
+      nonexistent_file(),
       abignore(true),
       parallel(true),
       show_relative_offset_changes(true),
@@ -1844,14 +1846,26 @@ parse_command_line(int argc, char* argv[], options& opts)
       if (argv[i][0] != '-')
         {
           if (opts.package1.empty())
-            opts.package1 = abigail::tools_utils::make_path_absolute(argv[i]).get();
+            {
+              opts.package1 = abigail::tools_utils::make_path_absolute(argv[i]).get();
+              opts.nonexistent_file = !file_exists(opts.package1);
+            }
           else if (opts.package2.empty())
-            opts.package2 = abigail::tools_utils::make_path_absolute(argv[i]).get();
+            {
+              opts.package2 = abigail::tools_utils::make_path_absolute(argv[i]).get();
+              opts.nonexistent_file = !file_exists(opts.package2);
+            }
           else
 	    {
 	      opts.wrong_arg = argv[i];
 	      return false;
 	    }
+
+          if (opts.nonexistent_file)
+            {
+              opts.wrong_option = argv[i];
+              return true;
+            }
         }
       else if (!strcmp(argv[i], "--debug-info-pkg1")
 	       || !strcmp(argv[i], "--d1"))
@@ -1998,6 +2012,15 @@ main(int argc, char* argv[])
 	      | abigail::tools_utils::ABIDIFF_ERROR);
     }
 
+  if (opts.nonexistent_file)
+    {
+      emit_prefix("abipkgdiff", cerr)
+	<< "The input file " << opts.wrong_option << " doesn't exist\n"
+	"try the --help option for more information\n";
+      return (abigail::tools_utils::ABIDIFF_USAGE_ERROR
+	      | abigail::tools_utils::ABIDIFF_ERROR);
+    }
+
   if (opts.display_usage)
     {
       display_usage(argv[0], cout);
-- 
2.7.4