[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [patch] install with a missing strip tool

From: Ondrej Oprala
Subject: Re: [patch] install with a missing strip tool
Date: Thu, 21 Feb 2013 20:40:40 +0100
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:17.0) Gecko/20130110 Thunderbird/17.0.2

On 02/21/2013 05:46 PM, Bernhard Voelker wrote:
On 02/21/2013 04:44 PM, Ondrej Oprala wrote:
   this patch address the problem mentioned here by John Reiser:  .
Install now checks the $PATH or the path given for the strip
program and turns off the -s option if none is found.

Hi Ondrej,

thanks for working on this old bug report.

+  if (strip_files)
+    {
+      /* if specified with a path.  */
+      if (strip_program_specified && strchr (strip_program, '/'))
Doesn't this hard-coded directory separator make porting
difficult (e.g. to Cygwin)?

+        {
+          struct stat dst_st;
+          if (lstat (strip_program, &dst_st) == -1)
What if strip_program is dangling symlink? Then lstat will
succeed but the installed program will also be left behind
with wrong permissions (yet ginstall would indicate this failure
with a proper diagnostic, as today).

+            strip_files = false;
+        }
+      else
+        {
+          char *path = (char *)find_in_path (strip_program);
A little and very unlikely race - if the strip_program is removed
between the call of find_in_path () and strip (), but well ...

+          if (STREQ (path, strip_program))
+            strip_files = false;
+          else
+            free (path);
+        }
+      if (!strip_files)
+        error (0, 0, _("WARNING: %s not found - ignoring the -s option"),
+              quote (strip_program));
+    }
In general, I'm a bit biased about the solution: wouldn't it
be better to simply "unlink (name)" if the strip_program failed?
This would be also safer because the strip_program might fail due
to other reasons (system limits, bad ELF format, ...). The bare
check if the file can be found (even in PATH) and is executable is
not sufficient IMO.

Have a nice day,

Hi Bernhard,
thanks for looking at the patch; it is, of course, possible to unlink the file if the strip itself fails, which seems like an easier solution as well. To be honest I like the idea of checking for strip more than just reverting the changes if something goes wrong, but you raise some valid points in my code and I don't see a reasonable way to fix all of them (and I doubt it would be worth the effort) , so let's do your approach ;) .


Attachment: DIFF
Description: Text document

reply via email to

[Prev in Thread] Current Thread [Next in Thread]