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 | expand |
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 }); >
(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
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 }); >
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.
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. >
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. >> >
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 });
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(-)