Patchwork [BZ,24544] Use support_install_prefix in elf/tst-pldd.c

login
register
mail settings
Submitter Romain Geissler
Date May 9, 2019, 9:52 p.m.
Message ID <alpine.LSU.2.20.1905092149260.41215@ncerndobedev5004.etv.nce.amadeus.net>
Download mbox | patch
Permalink /patch/32622/
State New
Headers show

Comments

Romain Geissler - May 9, 2019, 9:52 p.m.
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(-)
Carlos O'Donell - May 9, 2019, 10:06 p.m.
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.
(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 Netto - May 10, 2019, 12:29 p.m.
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.
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.
Andreas Schwab - May 13, 2019, 3:54 p.m.
On Mai 10 2019, Adhemerval Zanella <adhemerval.zanella@linaro.org> wrote:

> Use snprintf instead (there is no need to actually initialize 
> prog as well):
>
>   snprintf (prog, sizeof prog, "%s/bin/pldd", support_install_prefix) 

That should be asprintf instead.

Andreas.
Adhemerval Zanella Netto - May 13, 2019, 4:52 p.m.
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.
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 --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 });