[BZ,24544] Use support_install_prefix in elf/tst-pldd.c
diff mbox series

Message ID alpine.LSU.2.20.1905092149260.41215@ncerndobedev5004.etv.nce.amadeus.net
State New
Headers show
Series
  • [BZ,24544] Use support_install_prefix in elf/tst-pldd.c
Related show

Commit Message

Romain Geissler May 9, 2019, 9:52 p.m. UTC
Hi,

This should fix BZ 24544 by using support_install_prefix as suggested by Carlos. Tested on my use case with --prefix, I have not tried without --prefix, is support_install_prefix equal to "/usr" by default, or is it "/", or is it empty ?

Cheers,
Romain


2019-05-09  Romain Geissler  <romain.geissler@amadeus.com>

	[BZ #24544]
	* elf/tst-pldd.c: Include <support/support.h>.
	(PATH_MAX) [!PATH_MAX]: Define PATH_MAX macro.
	(do_test): Use support_install_prefix to compute pldd path.



From 9a311911c3a74ef11626aaf1b1950d89d0ea20be Mon Sep 17 00:00:00 2001
From: Romain Geissler <romain.geissler@amadeus.com>
Date: Thu, 9 May 2019 21:38:42 +0000
Subject: [PATCH] [BZ #24544] Use support_install_prefix in elf/tst-pldd.c

---
 elf/tst-pldd.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

Comments

Carlos O'Donell May 9, 2019, 10:06 p.m. UTC | #1
On 5/9/19 5:52 PM, Romain Geissler wrote:
> Hi,
> 
> This should fix BZ 24544 by using support_install_prefix as suggested
> by Carlos. Tested on my use case with --prefix, I have not tried
> without --prefix, is support_install_prefix equal to "/usr" by
> default, or is it "/", or is it empty ?

support_install_prefix is equal to INSTDIR_PATH which is equal to $(prefix),
that is anything that you pass via --prefix, and by default '/usr'.
 
> Cheers,
> Romain
> 
> 

This looks good to me.

Could you please test without --prefix and if that works I'll commit
this for you?

You don't have an FSF copyright assignment, but the following below
is only 8 lines of changes, and so not yet legally significant.
However, if we want to accept more patches from you, you'll need to
get assignment, which should be straight forward:
https://sourceware.org/glibc/wiki/Contribution%20checklist#FSF_copyright_Assignment

Reviewed-by: Carlos O'Donell <carlos@redhat.com>

> 2019-05-09  Romain Geissler  <romain.geissler@amadeus.com>
> 
> 	[BZ #24544]
> 	* elf/tst-pldd.c: Include <support/support.h>.
> 	(PATH_MAX) [!PATH_MAX]: Define PATH_MAX macro.
> 	(do_test): Use support_install_prefix to compute pldd path.

OK.
 
> 
> From 9a311911c3a74ef11626aaf1b1950d89d0ea20be Mon Sep 17 00:00:00 2001
> From: Romain Geissler <romain.geissler@amadeus.com>
> Date: Thu, 9 May 2019 21:38:42 +0000
> Subject: [PATCH] [BZ #24544] Use support_install_prefix in elf/tst-pldd.c
> 
> ---
>  elf/tst-pldd.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/elf/tst-pldd.c b/elf/tst-pldd.c
> index 2a9f58936f0..534e28ed502 100644
> --- a/elf/tst-pldd.c
> +++ b/elf/tst-pldd.c
> @@ -25,10 +25,15 @@
>  #include <array_length.h>
>  #include <gnu/lib-names.h>
> 
> +#include <support/support.h>

OK.

>  #include <support/subprocess.h>
>  #include <support/capture_subprocess.h>
>  #include <support/check.h>
> 
> +#ifndef PATH_MAX
> +# define PATH_MAX 4096
> +#endif

OK.

> +
>  static void
>  target_process (void *arg)
>  {
> @@ -60,7 +65,9 @@ do_test (void)
>      char pid[3 * sizeof (uint32_t) + 1];
>      snprintf (pid, array_length (pid), "%d", target.pid);
> 
> -    const char prog[] = "/usr/bin/pldd";
> +    char prog[PATH_MAX] = "";
> +    strcpy(prog, support_install_prefix);
> +    strcat(prog, "/bin/pldd");

OK.

> 
>      pldd = support_capture_subprogram (prog,
>        (char *const []) { (char *) prog, pid, NULL });
>
Romain Geissler May 9, 2019, 10:53 p.m. UTC | #2
(re-sending mail in proper text format for the mailing list)

On Thu, 9 May 2019, Carlos O'Donell wrote:

>
> On 5/9/19 5:52 PM, Romain Geissler wrote:
> > Hi,
> >
> > This should fix BZ 24544 by using support_install_prefix as suggested
> > by Carlos. Tested on my use case with --prefix, I have not tried
> > without --prefix, is support_install_prefix equal to "/usr" by
> > default, or is it "/", or is it empty ?
>
> support_install_prefix is equal to INSTDIR_PATH which is equal to $(prefix),
> that is anything that you pass via --prefix, and by default '/usr'.

Actually I tried without --prefix, and I get the same error as in here
https://lists.gnu.org/archive/html/bug-glibc/2004-08/msg00139.html So
can I assume everyone on Linux distribution side uses --prefix=/usr ?

> This looks good to me.
>
> Could you please test without --prefix and if that works I'll commit
> this for you?

As written above, without --prefix, the configure script ends up with an
error saying that the default (/usr/local) is not Ok. So I tried with
--prefix=/usr and totally destroyed my system where all commands was
segfaulting (hopefully it was in a throwable container). So I cannot
tell you if it works with --prefix=/usr, but it should.

>
> You don't have an FSF copyright assignment, but the following below
> is only 8 lines of changes, and so not yet legally significant.
> However, if we want to accept more patches from you, you'll need to
> get assignment, which should be straight forward:

Yes I know quite well this process, I have pushed the legal team from my
employer to contact the FSF quite some time ago. Discussions are still
in progress for now.

Cheers,
Romain
Adhemerval Zanella May 10, 2019, 12:29 p.m. UTC | #3
On 09/05/2019 18:52, Romain Geissler wrote:
> Hi,
> 
> This should fix BZ 24544 by using support_install_prefix as suggested by Carlos. Tested on my use case with --prefix, I have not tried without --prefix, is support_install_prefix equal to "/usr" by default, or is it "/", or is it empty ?
> 
> Cheers,
> Romain
> 
> 
> 2019-05-09  Romain Geissler  <romain.geissler@amadeus.com>
> 
> 	[BZ #24544]
> 	* elf/tst-pldd.c: Include <support/support.h>.
> 	(PATH_MAX) [!PATH_MAX]: Define PATH_MAX macro.
> 	(do_test): Use support_install_prefix to compute pldd path.
> 
> 
> 
> From 9a311911c3a74ef11626aaf1b1950d89d0ea20be Mon Sep 17 00:00:00 2001
> From: Romain Geissler <romain.geissler@amadeus.com>
> Date: Thu, 9 May 2019 21:38:42 +0000
> Subject: [PATCH] [BZ #24544] Use support_install_prefix in elf/tst-pldd.c
> 
> ---
>  elf/tst-pldd.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/elf/tst-pldd.c b/elf/tst-pldd.c
> index 2a9f58936f0..534e28ed502 100644
> --- a/elf/tst-pldd.c
> +++ b/elf/tst-pldd.c
> @@ -25,10 +25,15 @@
>  #include <array_length.h>
>  #include <gnu/lib-names.h>
> 
> +#include <support/support.h>
>  #include <support/subprocess.h>
>  #include <support/capture_subprocess.h>
>  #include <support/check.h>
> 
> +#ifndef PATH_MAX
> +# define PATH_MAX 4096
> +#endif
> +
>  static void
>  target_process (void *arg)
>  {
> @@ -60,7 +65,9 @@ do_test (void)
>      char pid[3 * sizeof (uint32_t) + 1];
>      snprintf (pid, array_length (pid), "%d", target.pid);
> 
> -    const char prog[] = "/usr/bin/pldd";
> +    char prog[PATH_MAX] = "";
> +    strcpy(prog, support_install_prefix);
> +    strcat(prog, "/bin/pldd");

Use snprintf instead (there is no need to actually initialize 
prog as well):

  snprintf (prog, sizeof prog, "%s/bin/pldd", support_install_prefix) 

LGTM with the change.

(as a side note, I think we might include a xsnprintf).

> 
>      pldd = support_capture_subprogram (prog,
>        (char *const []) { (char *) prog, pid, NULL });
>
Carlos O'Donell May 13, 2019, 3:43 p.m. UTC | #4
On 5/10/19 8:29 AM, Adhemerval Zanella wrote:
>> -    const char prog[] = "/usr/bin/pldd";
>> +    char prog[PATH_MAX] = "";
>> +    strcpy(prog, support_install_prefix);
>> +    strcat(prog, "/bin/pldd");
> 
> Use snprintf instead (there is no need to actually initialize 
> prog as well):
> 
>   snprintf (prog, sizeof prog, "%s/bin/pldd", support_install_prefix) 
> 
> LGTM with the change.

This won't work.

Users can configure --prefix, and --bindir, so you have to abstract
this up a level:

* support/Makefile (CFLAGS-support_paths.c): Define -DBINDIR_PATH=\"$(bindir)\"
* support/support_paths.h (support_install_bindir): Define as BINDIR_PATH
* Use support_install_bindir to set pldd's path.
  snprintf (prog, sizeof prog, "%s/pldd", support_install_bindir)

We'll eventually need one of each kind of variable for all the places
binaries are installed because we want to test each of them in a
container under test conditions.
Adhemerval Zanella May 13, 2019, 4:52 p.m. UTC | #5
On 13/05/2019 12:43, Carlos O'Donell wrote:
> On 5/10/19 8:29 AM, Adhemerval Zanella wrote:
>>> -    const char prog[] = "/usr/bin/pldd";
>>> +    char prog[PATH_MAX] = "";
>>> +    strcpy(prog, support_install_prefix);
>>> +    strcat(prog, "/bin/pldd");
>>
>> Use snprintf instead (there is no need to actually initialize 
>> prog as well):
>>
>>   snprintf (prog, sizeof prog, "%s/bin/pldd", support_install_prefix) 
>>
>> LGTM with the change.
> 
> This won't work.
> 
> Users can configure --prefix, and --bindir, so you have to abstract
> this up a level:

In fact this does not work because we don't have a bindir definition
on config.make.in, so --bindir does not actually change anything. The
default value set by Makeconfig:206 is used regardless.

> 
> * support/Makefile (CFLAGS-support_paths.c): Define -DBINDIR_PATH=\"$(bindir)\"
> * support/support_paths.h (support_install_bindir): Define as BINDIR_PATH
> * Use support_install_bindir to set pldd's path.
>   snprintf (prog, sizeof prog, "%s/pldd", support_install_bindir)

In fact I think we should first fix the bindir set by configure, add
the bindir on libsupport and then fix tst-pldd with asprintf.  I am
working on this.

> 
> We'll eventually need one of each kind of variable for all the places
> binaries are installed because we want to test each of them in a
> container under test conditions.
>
Carlos O'Donell May 13, 2019, 4:56 p.m. UTC | #6
On 5/13/19 12:52 PM, Adhemerval Zanella wrote:
> 
> 
> On 13/05/2019 12:43, Carlos O'Donell wrote:
>> On 5/10/19 8:29 AM, Adhemerval Zanella wrote:
>>>> -    const char prog[] = "/usr/bin/pldd";
>>>> +    char prog[PATH_MAX] = "";
>>>> +    strcpy(prog, support_install_prefix);
>>>> +    strcat(prog, "/bin/pldd");
>>>
>>> Use snprintf instead (there is no need to actually initialize 
>>> prog as well):
>>>
>>>   snprintf (prog, sizeof prog, "%s/bin/pldd", support_install_prefix) 
>>>
>>> LGTM with the change.
>>
>> This won't work.
>>
>> Users can configure --prefix, and --bindir, so you have to abstract
>> this up a level:
> 
> In fact this does not work because we don't have a bindir definition
> on config.make.in, so --bindir does not actually change anything. The
> default value set by Makeconfig:206 is used regardless.

A similar bug exists for aux-cache :-( we don't use localstatedir there.


>>
>> * support/Makefile (CFLAGS-support_paths.c): Define -DBINDIR_PATH=\"$(bindir)\"
>> * support/support_paths.h (support_install_bindir): Define as BINDIR_PATH
>> * Use support_install_bindir to set pldd's path.
>>   snprintf (prog, sizeof prog, "%s/pldd", support_install_bindir)
> 
> In fact I think we should first fix the bindir set by configure, add
> the bindir on libsupport and then fix tst-pldd with asprintf.  I am
> working on this.
> 
>>
>> We'll eventually need one of each kind of variable for all the places
>> binaries are installed because we want to test each of them in a
>> container under test conditions.
>>
>

Patch
diff mbox series

diff --git a/elf/tst-pldd.c b/elf/tst-pldd.c
index 2a9f58936f0..534e28ed502 100644
--- a/elf/tst-pldd.c
+++ b/elf/tst-pldd.c
@@ -25,10 +25,15 @@ 
 #include <array_length.h>
 #include <gnu/lib-names.h>

+#include <support/support.h>
 #include <support/subprocess.h>
 #include <support/capture_subprocess.h>
 #include <support/check.h>

+#ifndef PATH_MAX
+# define PATH_MAX 4096
+#endif
+
 static void
 target_process (void *arg)
 {
@@ -60,7 +65,9 @@  do_test (void)
     char pid[3 * sizeof (uint32_t) + 1];
     snprintf (pid, array_length (pid), "%d", target.pid);

-    const char prog[] = "/usr/bin/pldd";
+    char prog[PATH_MAX] = "";
+    strcpy(prog, support_install_prefix);
+    strcat(prog, "/bin/pldd");

     pldd = support_capture_subprogram (prog,
       (char *const []) { (char *) prog, pid, NULL });