| Message ID | alpine.LSU.2.20.1905092149260.41215@ncerndobedev5004.etv.nce.amadeus.net |
|---|---|
| State | DCO or assignment missing, archived |
| Headers |
Received: (qmail 54553 invoked by alias); 9 May 2019 21:56:16 -0000 Mailing-List: contact libc-alpha-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: <libc-alpha.sourceware.org> List-Unsubscribe: <mailto:libc-alpha-unsubscribe-##L=##H@sourceware.org> List-Subscribe: <mailto:libc-alpha-subscribe@sourceware.org> List-Archive: <http://sourceware.org/ml/libc-alpha/> List-Post: <mailto:libc-alpha@sourceware.org> List-Help: <mailto:libc-alpha-help@sourceware.org>, <http://sourceware.org/ml/#faqs> Sender: libc-alpha-owner@sourceware.org Delivered-To: mailing list libc-alpha@sourceware.org Received: (qmail 54541 invoked by uid 89); 9 May 2019 21:56:16 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-13.4 required=5.0 tests=AWL, BAYES_00, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, RCVD_IN_DNSWL_NONE, SPF_HELO_PASS autolearn=ham version=3.3.1 spammy=strcat, prog X-HELO: mail1.bemta26.messagelabs.com Return-Path: <romain.geissler@amadeus.com> DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=amadeus.com; s=SYM2018Q2; t=1557438972; i=@amadeus.com; bh=7bPGS7MjdYvV6Xzp2TbqzdRz8sFzi1oZMZdf2aGwY/w=; h=Date:From:To:CC:Subject:Message-ID:MIME-Version:Content-Type; b=dMyiinMTm60r/982P5MiZB39mTGXBPf0/wm3v1134jtwZK3B5mKXyYkdsQzXrPyuX QbnTyUNQzkmP9nqGRqrQFkEppgqUIOa9E2KxxnJr6zPjBInECQJ6qNLNqQVCjDkBQu BToAsKoZaXYPWK8teHznZ5lCsT0cW4TQgD+tGDAtgCQNiAONWvAzcZ4XTrUJClzrCK fnc9+g8q9IA+NwidQAZlRauu2ot5WWJJXVQvpW6M2YG7nR05dJmNBvqZVoUDzoMWod SYT7BQNXlHuOYMxXSxNRjP2+n4Atiuws14IMRNo8JiIiTZwoCHBgaYSO7H3daUbtge 5XChSUXZBwgMQ== X-Env-Sender: romain.geissler@amadeus.com X-Msg-Ref: server-4.tower-230.messagelabs.com!1557438966!4982085!1 X-SYMC-ESS-Client-Auth: outbound-route-from=pass X-StarScan-Received: X-StarScan-Version: 9.31.5; banners=-,-,- X-VirusChecked: Checked Date: Thu, 9 May 2019 21:52:48 +0000 From: Romain Geissler <romain.geissler@amadeus.com> To: <libc-alpha@sourceware.org> CC: <carlos@redhat.com> Subject: [BZ 24544] Use support_install_prefix in elf/tst-pldd.c Message-ID: <alpine.LSU.2.20.1905092149260.41215@ncerndobedev5004.etv.nce.amadeus.net> User-Agent: Alpine 2.20 (LSU 67 2015-01-07) MIME-Version: 1.0 Content-Type: text/plain; charset="US-ASCII" |
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
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 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.
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 });