From: Jan S. <jst...@re...> - 2012-10-26 10:54:52
|
This test picked fixed address in setup() which was used throughout the test. There has been reports that it can fail with EINVAL. I could reproduce it by adding single printf in between detach/attach: http://article.gmane.org/gmane.linux.ltp/16480 This patch picks suitable base_addr before each test. So even if glibc would mmap extra page with addr == base_addr picked by setup(), new suitable address will be chosen. Signed-off-by: Jan Stancek <jst...@re...> --- testcases/kernel/syscalls/ipc/shmat/Makefile | 3 ++ testcases/kernel/syscalls/ipc/shmat/shmat01.c | 16 ++--------- testcases/kernel/syscalls/ipc/shmat/shmat02.c | 16 ++--------- testcases/kernel/syscalls/ipc/shmat/shmat_common.c | 29 ++++++++++++++++++++ 4 files changed, 38 insertions(+), 26 deletions(-) create mode 100644 testcases/kernel/syscalls/ipc/shmat/shmat_common.c diff --git a/testcases/kernel/syscalls/ipc/shmat/Makefile b/testcases/kernel/syscalls/ipc/shmat/Makefile index f467389..5cbf37c 100644 --- a/testcases/kernel/syscalls/ipc/shmat/Makefile +++ b/testcases/kernel/syscalls/ipc/shmat/Makefile @@ -20,4 +20,7 @@ top_srcdir ?= ../../../../.. include $(top_srcdir)/include/mk/testcases.mk include $(abs_srcdir)/../Makefile.inc + +MAKE_TARGETS := $(patsubst $(abs_srcdir)/%.c,%,$(wildcard $(abs_srcdir)/*[0-9].c)) + include $(top_srcdir)/include/mk/generic_leaf_target.mk diff --git a/testcases/kernel/syscalls/ipc/shmat/shmat01.c b/testcases/kernel/syscalls/ipc/shmat/shmat01.c index 6e4dee1..160abca 100644 --- a/testcases/kernel/syscalls/ipc/shmat/shmat01.c +++ b/testcases/kernel/syscalls/ipc/shmat/shmat01.c @@ -56,6 +56,7 @@ */ #include "ipcshm.h" +#include "shmat_common.c" char *TCID = "shmat01"; @@ -108,6 +109,7 @@ int main(int ac, char **av) /* * Use TEST macro to make the call */ + base_addr = probe_free_addr(); errno = 0; addr = shmat(*(TC[i].shmid), base_addr+TC[i].offset, TC[i].flags); @@ -262,19 +264,7 @@ void setup(void) "resource 1 in setup()"); } - /* Probe an available linear address for attachment */ - if ((base_addr = shmat(shm_id_1, NULL, 0)) == (void *)-1) { - tst_brkm(TBROK, cleanup, "Couldn't attach shared memory"); - } - if (shmdt((const void *)base_addr) == -1) { - tst_brkm(TBROK, cleanup, "Couldn't detach shared memory"); - } - - /* some architectures (e.g. parisc) are strange, so better always align to - * next SHMLBA address. */ - base_addr = - (void *)(((unsigned long)(base_addr) + (SHMLBA - 1)) & - ~(SHMLBA - 1)); + base_addr = probe_free_addr(); } /* diff --git a/testcases/kernel/syscalls/ipc/shmat/shmat02.c b/testcases/kernel/syscalls/ipc/shmat/shmat02.c index d4cca0e..b260e18 100644 --- a/testcases/kernel/syscalls/ipc/shmat/shmat02.c +++ b/testcases/kernel/syscalls/ipc/shmat/shmat02.c @@ -55,6 +55,7 @@ #include "ipcshm.h" #include <pwd.h> +#include "shmat_common.c" char *TCID = "shmat02"; char nobody_uid[] = "nobody"; @@ -125,6 +126,7 @@ int main(int ac, char **av) setup_tc(i, tc); + base_addr = probe_free_addr(); errno = 0; addr = shmat(*(tc->shmid), base_addr + tc->offset, 0); @@ -181,19 +183,7 @@ void setup(void) -1) tst_brkm(TBROK|TERRNO, cleanup, "shmget #2 failed"); - /* Probe an available linear address for attachment */ - if ((base_addr = shmat(shm_id_2, NULL, 0)) == (void *)-1) - tst_brkm(TBROK|TERRNO, cleanup, "shmat #1 failed"); - - if (shmdt((const void *)base_addr) == -1) - tst_brkm(TBROK|TERRNO, cleanup, "shmat #2 failed"); - - /* - * some architectures (e.g. parisc) are strange, so better always align - * to next SHMLBA address - */ - base_addr = - (void *)(((unsigned long)(base_addr) & ~(SHMLBA - 1)) + SHMLBA); + base_addr = probe_free_addr(); } /* diff --git a/testcases/kernel/syscalls/ipc/shmat/shmat_common.c b/testcases/kernel/syscalls/ipc/shmat/shmat_common.c new file mode 100644 index 0000000..826cf2d --- /dev/null +++ b/testcases/kernel/syscalls/ipc/shmat/shmat_common.c @@ -0,0 +1,29 @@ +void *probe_free_addr() +{ + void *p; + int ret; + int shm_id = -1; + + /* create a shared memory resource with read and write permissions + * We align this to SHMLBA so we should allocate at least + * SHMLBA*2 in case SHMLBA > page_size. */ + shm_id = shmget(getipckey(), SHMLBA*2, SHM_RW | IPC_CREAT | IPC_EXCL); + if (shm_id == -1) + tst_brkm(TBROK, cleanup, "probe: shmget failed"); + + /* Probe an available linear address for attachment */ + p = shmat(shm_id, NULL, 0); + if (p == (void *)-1) + tst_brkm(TBROK, cleanup, "probe: shmat failed"); + ret = shmdt((const void *)p); + if (ret == -1) + tst_brkm(TBROK, cleanup, "probe: shmdt failed"); + + rm_shm(shm_id); + + /* some architectures (e.g. parisc) are strange, so better always + * align to next SHMLBA address. */ + p = (void *)(((unsigned long)(p) + (SHMLBA - 1)) & ~(SHMLBA - 1)); + return p; +} + -- 1.7.1 |
From: Jan S. <jst...@re...> - 2012-10-26 11:15:42
|
----- Original Message ----- > From: "Jan Stancek" <jst...@re...> > To: ltp...@li... > Sent: Friday, 26 October, 2012 12:54:37 PM > Subject: [LTP] [PATCH] ipc/shmat: pick suitable base_addr before each testcase > > This test picked fixed address in setup() which was used throughout > the test. There has been reports that it can fail with EINVAL. > > I could reproduce it by adding single printf in between > detach/attach: > http://article.gmane.org/gmane.linux.ltp/16480 > > This patch picks suitable base_addr before each test. So even if > glibc > would mmap extra page with addr == base_addr picked by setup(), > new suitable address will be chosen. > > Signed-off-by: Jan Stancek <jst...@re...> I'll send v2, getipckey() appears to return only few unique keys and after couple loops this fails because it returns key which already exists. Regards, Jan |
From: Jan S. <jst...@re...> - 2012-10-26 11:20:51
|
This test picked fixed address in setup() which was used throughout the test. There has been reports that it can fail with EINVAL. I could reproduce it by adding single printf in between detach/attach: http://article.gmane.org/gmane.linux.ltp/16480 This patch picks suitable base_addr before each test. So even if glibc would mmap extra page with addr == base_addr picked by setup(), new suitable address will be chosen. Signed-off-by: Jan Stancek <jst...@re...> --- testcases/kernel/syscalls/ipc/shmat/Makefile | 3 ++ testcases/kernel/syscalls/ipc/shmat/shmat01.c | 16 ++------- testcases/kernel/syscalls/ipc/shmat/shmat02.c | 16 ++------- testcases/kernel/syscalls/ipc/shmat/shmat_common.c | 34 ++++++++++++++++++++ 4 files changed, 43 insertions(+), 26 deletions(-) create mode 100644 testcases/kernel/syscalls/ipc/shmat/shmat_common.c diff --git a/testcases/kernel/syscalls/ipc/shmat/Makefile b/testcases/kernel/syscalls/ipc/shmat/Makefile index f467389..5cbf37c 100644 --- a/testcases/kernel/syscalls/ipc/shmat/Makefile +++ b/testcases/kernel/syscalls/ipc/shmat/Makefile @@ -20,4 +20,7 @@ top_srcdir ?= ../../../../.. include $(top_srcdir)/include/mk/testcases.mk include $(abs_srcdir)/../Makefile.inc + +MAKE_TARGETS := $(patsubst $(abs_srcdir)/%.c,%,$(wildcard $(abs_srcdir)/*[0-9].c)) + include $(top_srcdir)/include/mk/generic_leaf_target.mk diff --git a/testcases/kernel/syscalls/ipc/shmat/shmat01.c b/testcases/kernel/syscalls/ipc/shmat/shmat01.c index 6e4dee1..160abca 100644 --- a/testcases/kernel/syscalls/ipc/shmat/shmat01.c +++ b/testcases/kernel/syscalls/ipc/shmat/shmat01.c @@ -56,6 +56,7 @@ */ #include "ipcshm.h" +#include "shmat_common.c" char *TCID = "shmat01"; @@ -108,6 +109,7 @@ int main(int ac, char **av) /* * Use TEST macro to make the call */ + base_addr = probe_free_addr(); errno = 0; addr = shmat(*(TC[i].shmid), base_addr+TC[i].offset, TC[i].flags); @@ -262,19 +264,7 @@ void setup(void) "resource 1 in setup()"); } - /* Probe an available linear address for attachment */ - if ((base_addr = shmat(shm_id_1, NULL, 0)) == (void *)-1) { - tst_brkm(TBROK, cleanup, "Couldn't attach shared memory"); - } - if (shmdt((const void *)base_addr) == -1) { - tst_brkm(TBROK, cleanup, "Couldn't detach shared memory"); - } - - /* some architectures (e.g. parisc) are strange, so better always align to - * next SHMLBA address. */ - base_addr = - (void *)(((unsigned long)(base_addr) + (SHMLBA - 1)) & - ~(SHMLBA - 1)); + base_addr = probe_free_addr(); } /* diff --git a/testcases/kernel/syscalls/ipc/shmat/shmat02.c b/testcases/kernel/syscalls/ipc/shmat/shmat02.c index d4cca0e..b260e18 100644 --- a/testcases/kernel/syscalls/ipc/shmat/shmat02.c +++ b/testcases/kernel/syscalls/ipc/shmat/shmat02.c @@ -55,6 +55,7 @@ #include "ipcshm.h" #include <pwd.h> +#include "shmat_common.c" char *TCID = "shmat02"; char nobody_uid[] = "nobody"; @@ -125,6 +126,7 @@ int main(int ac, char **av) setup_tc(i, tc); + base_addr = probe_free_addr(); errno = 0; addr = shmat(*(tc->shmid), base_addr + tc->offset, 0); @@ -181,19 +183,7 @@ void setup(void) -1) tst_brkm(TBROK|TERRNO, cleanup, "shmget #2 failed"); - /* Probe an available linear address for attachment */ - if ((base_addr = shmat(shm_id_2, NULL, 0)) == (void *)-1) - tst_brkm(TBROK|TERRNO, cleanup, "shmat #1 failed"); - - if (shmdt((const void *)base_addr) == -1) - tst_brkm(TBROK|TERRNO, cleanup, "shmat #2 failed"); - - /* - * some architectures (e.g. parisc) are strange, so better always align - * to next SHMLBA address - */ - base_addr = - (void *)(((unsigned long)(base_addr) & ~(SHMLBA - 1)) + SHMLBA); + base_addr = probe_free_addr(); } /* diff --git a/testcases/kernel/syscalls/ipc/shmat/shmat_common.c b/testcases/kernel/syscalls/ipc/shmat/shmat_common.c new file mode 100644 index 0000000..508bf4b --- /dev/null +++ b/testcases/kernel/syscalls/ipc/shmat/shmat_common.c @@ -0,0 +1,34 @@ +static key_t probe_key; + +void *probe_free_addr() +{ + void *p; + int ret; + int shm_id = -1; + + if (probe_key == 0) + probe_key = getipckey(); + + /* create a shared memory resource with read and write permissions + * We align this to SHMLBA so we should allocate at least + * SHMLBA*2 in case SHMLBA > page_size. */ + shm_id = shmget(probe_key, SHMLBA*2, SHM_RW | IPC_CREAT | IPC_EXCL); + if (shm_id == -1) + tst_brkm(TBROK, cleanup, "probe: shmget failed"); + + /* Probe an available linear address for attachment */ + p = shmat(shm_id, NULL, 0); + if (p == (void *)-1) + tst_brkm(TBROK, cleanup, "probe: shmat failed"); + ret = shmdt((const void *)p); + if (ret == -1) + tst_brkm(TBROK, cleanup, "probe: shmdt failed"); + + rm_shm(shm_id); + + /* some architectures (e.g. parisc) are strange, so better always + * align to next SHMLBA address. */ + p = (void *)(((unsigned long)(p) + (SHMLBA - 1)) & ~(SHMLBA - 1)); + return p; +} + -- 1.7.1 |
From: Wanlong G. <gao...@cn...> - 2012-10-29 02:18:11
|
On 10/26/2012 07:20 PM, Jan Stancek wrote: > This test picked fixed address in setup() which was used throughout > the test. There has been reports that it can fail with EINVAL. > > I could reproduce it by adding single printf in between > detach/attach: > http://article.gmane.org/gmane.linux.ltp/16480 > > This patch picks suitable base_addr before each test. So even if glibc > would mmap extra page with addr == base_addr picked by setup(), > new suitable address will be chosen. > > Signed-off-by: Jan Stancek <jst...@re...> > --- > testcases/kernel/syscalls/ipc/shmat/Makefile | 3 ++ > testcases/kernel/syscalls/ipc/shmat/shmat01.c | 16 ++------- > testcases/kernel/syscalls/ipc/shmat/shmat02.c | 16 ++------- > testcases/kernel/syscalls/ipc/shmat/shmat_common.c | 34 ++++++++++++++++++++ > 4 files changed, 43 insertions(+), 26 deletions(-) > create mode 100644 testcases/kernel/syscalls/ipc/shmat/shmat_common.c > > diff --git a/testcases/kernel/syscalls/ipc/shmat/Makefile b/testcases/kernel/syscalls/ipc/shmat/Makefile > index f467389..5cbf37c 100644 > --- a/testcases/kernel/syscalls/ipc/shmat/Makefile > +++ b/testcases/kernel/syscalls/ipc/shmat/Makefile > @@ -20,4 +20,7 @@ top_srcdir ?= ../../../../.. > > include $(top_srcdir)/include/mk/testcases.mk > include $(abs_srcdir)/../Makefile.inc > + > +MAKE_TARGETS := $(patsubst $(abs_srcdir)/%.c,%,$(wildcard $(abs_srcdir)/*[0-9].c)) > + > include $(top_srcdir)/include/mk/generic_leaf_target.mk > diff --git a/testcases/kernel/syscalls/ipc/shmat/shmat01.c b/testcases/kernel/syscalls/ipc/shmat/shmat01.c > index 6e4dee1..160abca 100644 > --- a/testcases/kernel/syscalls/ipc/shmat/shmat01.c > +++ b/testcases/kernel/syscalls/ipc/shmat/shmat01.c > @@ -56,6 +56,7 @@ > */ > > #include "ipcshm.h" > +#include "shmat_common.c" > > char *TCID = "shmat01"; > > @@ -108,6 +109,7 @@ int main(int ac, char **av) > /* > * Use TEST macro to make the call > */ > + base_addr = probe_free_addr(); > errno = 0; > addr = shmat(*(TC[i].shmid), base_addr+TC[i].offset, > TC[i].flags); > @@ -262,19 +264,7 @@ void setup(void) > "resource 1 in setup()"); > } > > - /* Probe an available linear address for attachment */ > - if ((base_addr = shmat(shm_id_1, NULL, 0)) == (void *)-1) { > - tst_brkm(TBROK, cleanup, "Couldn't attach shared memory"); > - } > - if (shmdt((const void *)base_addr) == -1) { > - tst_brkm(TBROK, cleanup, "Couldn't detach shared memory"); > - } > - > - /* some architectures (e.g. parisc) are strange, so better always align to > - * next SHMLBA address. */ > - base_addr = > - (void *)(((unsigned long)(base_addr) + (SHMLBA - 1)) & > - ~(SHMLBA - 1)); > + base_addr = probe_free_addr(); We don't need to get base_addr in setup(), right? seems get twice. > } > > /* > diff --git a/testcases/kernel/syscalls/ipc/shmat/shmat02.c b/testcases/kernel/syscalls/ipc/shmat/shmat02.c > index d4cca0e..b260e18 100644 > --- a/testcases/kernel/syscalls/ipc/shmat/shmat02.c > +++ b/testcases/kernel/syscalls/ipc/shmat/shmat02.c > @@ -55,6 +55,7 @@ > > #include "ipcshm.h" > #include <pwd.h> > +#include "shmat_common.c" > > char *TCID = "shmat02"; > char nobody_uid[] = "nobody"; > @@ -125,6 +126,7 @@ int main(int ac, char **av) > > setup_tc(i, tc); > > + base_addr = probe_free_addr(); > errno = 0; > addr = shmat(*(tc->shmid), base_addr + tc->offset, 0); > > @@ -181,19 +183,7 @@ void setup(void) > -1) > tst_brkm(TBROK|TERRNO, cleanup, "shmget #2 failed"); > > - /* Probe an available linear address for attachment */ > - if ((base_addr = shmat(shm_id_2, NULL, 0)) == (void *)-1) > - tst_brkm(TBROK|TERRNO, cleanup, "shmat #1 failed"); > - > - if (shmdt((const void *)base_addr) == -1) > - tst_brkm(TBROK|TERRNO, cleanup, "shmat #2 failed"); > - > - /* > - * some architectures (e.g. parisc) are strange, so better always align > - * to next SHMLBA address > - */ > - base_addr = > - (void *)(((unsigned long)(base_addr) & ~(SHMLBA - 1)) + SHMLBA); > + base_addr = probe_free_addr(); ditto > } > > /* > diff --git a/testcases/kernel/syscalls/ipc/shmat/shmat_common.c b/testcases/kernel/syscalls/ipc/shmat/shmat_common.c > new file mode 100644 > index 0000000..508bf4b > --- /dev/null > +++ b/testcases/kernel/syscalls/ipc/shmat/shmat_common.c > @@ -0,0 +1,34 @@ > +static key_t probe_key; > + > +void *probe_free_addr() > +{ > + void *p; > + int ret; > + int shm_id = -1; > + > + if (probe_key == 0) > + probe_key = getipckey(); > + > + /* create a shared memory resource with read and write permissions > + * We align this to SHMLBA so we should allocate at least > + * SHMLBA*2 in case SHMLBA > page_size. */ > + shm_id = shmget(probe_key, SHMLBA*2, SHM_RW | IPC_CREAT | IPC_EXCL); > + if (shm_id == -1) > + tst_brkm(TBROK, cleanup, "probe: shmget failed"); > + > + /* Probe an available linear address for attachment */ > + p = shmat(shm_id, NULL, 0); > + if (p == (void *)-1) > + tst_brkm(TBROK, cleanup, "probe: shmat failed"); > + ret = shmdt((const void *)p); > + if (ret == -1) > + tst_brkm(TBROK, cleanup, "probe: shmdt failed"); > + > + rm_shm(shm_id); > + > + /* some architectures (e.g. parisc) are strange, so better always > + * align to next SHMLBA address. */ > + p = (void *)(((unsigned long)(p) + (SHMLBA - 1)) & ~(SHMLBA - 1)); > + return p; > +} > + Here, add a new blank line. Thanks, Wanlong Gao > |
From: Jan S. <jst...@re...> - 2012-10-29 07:47:03
|
----- Original Message ----- > From: "Wanlong Gao" <gao...@cn...> > To: "Jan Stancek" <jst...@re...> > Cc: ltp...@li..., "Caspar Zhang" <ca...@ca...>, "Cyril Hrubis" <ch...@su...> > Sent: Monday, 29 October, 2012 3:18:29 AM > Subject: Re: [LTP] [PATCH v2] ipc/shmat: pick suitable base_addr before each testcase > > On 10/26/2012 07:20 PM, Jan Stancek wrote: > > This test picked fixed address in setup() which was used throughout > > the test. There has been reports that it can fail with EINVAL. > > > > I could reproduce it by adding single printf in between > > detach/attach: > > http://article.gmane.org/gmane.linux.ltp/16480 > > > > This patch picks suitable base_addr before each test. So even if > > glibc > > would mmap extra page with addr == base_addr picked by setup(), > > new suitable address will be chosen. > > > > Signed-off-by: Jan Stancek <jst...@re...> > > --- > > testcases/kernel/syscalls/ipc/shmat/Makefile | 3 ++ > > testcases/kernel/syscalls/ipc/shmat/shmat01.c | 16 > > ++------- > > testcases/kernel/syscalls/ipc/shmat/shmat02.c | 16 > > ++------- > > testcases/kernel/syscalls/ipc/shmat/shmat_common.c | 34 > > ++++++++++++++++++++ > > 4 files changed, 43 insertions(+), 26 deletions(-) > > create mode 100644 > > testcases/kernel/syscalls/ipc/shmat/shmat_common.c > > > > diff --git a/testcases/kernel/syscalls/ipc/shmat/Makefile > > b/testcases/kernel/syscalls/ipc/shmat/Makefile > > index f467389..5cbf37c 100644 > > --- a/testcases/kernel/syscalls/ipc/shmat/Makefile > > +++ b/testcases/kernel/syscalls/ipc/shmat/Makefile > > @@ -20,4 +20,7 @@ top_srcdir ?= ../../../../.. > > > > include $(top_srcdir)/include/mk/testcases.mk > > include $(abs_srcdir)/../Makefile.inc > > + > > +MAKE_TARGETS := $(patsubst > > $(abs_srcdir)/%.c,%,$(wildcard $(abs_srcdir)/*[0-9].c)) > > + > > include $(top_srcdir)/include/mk/generic_leaf_target.mk > > diff --git a/testcases/kernel/syscalls/ipc/shmat/shmat01.c > > b/testcases/kernel/syscalls/ipc/shmat/shmat01.c > > index 6e4dee1..160abca 100644 > > --- a/testcases/kernel/syscalls/ipc/shmat/shmat01.c > > +++ b/testcases/kernel/syscalls/ipc/shmat/shmat01.c > > @@ -56,6 +56,7 @@ > > */ > > > > #include "ipcshm.h" > > +#include "shmat_common.c" > > > > char *TCID = "shmat01"; > > > > @@ -108,6 +109,7 @@ int main(int ac, char **av) > > /* > > * Use TEST macro to make the call > > */ > > + base_addr = probe_free_addr(); > > errno = 0; > > addr = shmat(*(TC[i].shmid), base_addr+TC[i].offset, > > TC[i].flags); > > @@ -262,19 +264,7 @@ void setup(void) > > "resource 1 in setup()"); > > } > > > > - /* Probe an available linear address for attachment */ > > - if ((base_addr = shmat(shm_id_1, NULL, 0)) == (void *)-1) { > > - tst_brkm(TBROK, cleanup, "Couldn't attach shared memory"); > > - } > > - if (shmdt((const void *)base_addr) == -1) { > > - tst_brkm(TBROK, cleanup, "Couldn't detach shared memory"); > > - } > > - > > - /* some architectures (e.g. parisc) are strange, so better always > > align to > > - * next SHMLBA address. */ > > - base_addr = > > - (void *)(((unsigned long)(base_addr) + (SHMLBA - 1)) & > > - ~(SHMLBA - 1)); > > + base_addr = probe_free_addr(); > > We don't need to get base_addr in setup(), right? seems get twice. Probably not, I left it there just to exercise that codepath too before actual test. > > > } > > > > /* > > diff --git a/testcases/kernel/syscalls/ipc/shmat/shmat02.c > > b/testcases/kernel/syscalls/ipc/shmat/shmat02.c > > index d4cca0e..b260e18 100644 > > --- a/testcases/kernel/syscalls/ipc/shmat/shmat02.c > > +++ b/testcases/kernel/syscalls/ipc/shmat/shmat02.c > > @@ -55,6 +55,7 @@ > > > > #include "ipcshm.h" > > #include <pwd.h> > > +#include "shmat_common.c" > > > > char *TCID = "shmat02"; > > char nobody_uid[] = "nobody"; > > @@ -125,6 +126,7 @@ int main(int ac, char **av) > > > > setup_tc(i, tc); > > > > + base_addr = probe_free_addr(); > > errno = 0; > > addr = shmat(*(tc->shmid), base_addr + tc->offset, 0); > > > > @@ -181,19 +183,7 @@ void setup(void) > > -1) > > tst_brkm(TBROK|TERRNO, cleanup, "shmget #2 failed"); > > > > - /* Probe an available linear address for attachment */ > > - if ((base_addr = shmat(shm_id_2, NULL, 0)) == (void *)-1) > > - tst_brkm(TBROK|TERRNO, cleanup, "shmat #1 failed"); > > - > > - if (shmdt((const void *)base_addr) == -1) > > - tst_brkm(TBROK|TERRNO, cleanup, "shmat #2 failed"); > > - > > - /* > > - * some architectures (e.g. parisc) are strange, so better always > > align > > - * to next SHMLBA address > > - */ > > - base_addr = > > - (void *)(((unsigned long)(base_addr) & ~(SHMLBA - 1)) + > > SHMLBA); > > + base_addr = probe_free_addr(); > > ditto > > > } > > > > /* > > diff --git a/testcases/kernel/syscalls/ipc/shmat/shmat_common.c > > b/testcases/kernel/syscalls/ipc/shmat/shmat_common.c > > new file mode 100644 > > index 0000000..508bf4b > > --- /dev/null > > +++ b/testcases/kernel/syscalls/ipc/shmat/shmat_common.c > > @@ -0,0 +1,34 @@ > > +static key_t probe_key; > > + > > +void *probe_free_addr() > > +{ > > + void *p; > > + int ret; > > + int shm_id = -1; > > + > > + if (probe_key == 0) > > + probe_key = getipckey(); > > + > > + /* create a shared memory resource with read and write > > permissions > > + * We align this to SHMLBA so we should allocate at least > > + * SHMLBA*2 in case SHMLBA > page_size. */ > > + shm_id = shmget(probe_key, SHMLBA*2, SHM_RW | IPC_CREAT | > > IPC_EXCL); > > + if (shm_id == -1) > > + tst_brkm(TBROK, cleanup, "probe: shmget failed"); > > + > > + /* Probe an available linear address for attachment */ > > + p = shmat(shm_id, NULL, 0); > > + if (p == (void *)-1) > > + tst_brkm(TBROK, cleanup, "probe: shmat failed"); > > + ret = shmdt((const void *)p); > > + if (ret == -1) > > + tst_brkm(TBROK, cleanup, "probe: shmdt failed"); > > + > > + rm_shm(shm_id); > > + > > + /* some architectures (e.g. parisc) are strange, so better always > > + * align to next SHMLBA address. */ > > + p = (void *)(((unsigned long)(p) + (SHMLBA - 1)) & ~(SHMLBA - > > 1)); > > + return p; > > +} > > + > > Here, add a new blank line. I'm not following you here. Do you want 2 blank lines at the end of file? Regards, Jan |
From: Wanlong G. <gao...@cn...> - 2012-10-29 07:56:53
|
On 10/29/2012 03:46 PM, Jan Stancek wrote: >>> + rm_shm(shm_id); >>> > > + >>> > > + /* some architectures (e.g. parisc) are strange, so better always >>> > > + * align to next SHMLBA address. */ >>> > > + p = (void *)(((unsigned long)(p) + (SHMLBA - 1)) & ~(SHMLBA - >>> > > 1)); >>> > > + return p; >>> > > +} >>> > > + >> > >> > Here, add a new blank line. > I'm not following you here. Do you want 2 blank lines at the end of file? Oops, sorry for confusing you, I meant no blank line at the end of file, it's warned when running "git am". ;) Thanks, Wanlong Gao > > Regards, > Jan > |
From: Jan S. <jst...@re...> - 2012-10-29 09:50:44
|
----- Original Message ----- > From: "Wanlong Gao" <gao...@cn...> > To: "Jan Stancek" <jst...@re...> > Cc: ltp...@li..., "Caspar Zhang" <ca...@ca...>, "Cyril Hrubis" <ch...@su...> > Sent: Monday, 29 October, 2012 3:18:29 AM > Subject: Re: [LTP] [PATCH v2] ipc/shmat: pick suitable base_addr before each testcase ~(SHMLBA - 1)); > > + base_addr = probe_free_addr(); > > We don't need to get base_addr in setup(), right? seems get twice. > I had a look at glibc/uClibc and I think you are correct. I removed it in v3, along with that new line at the end. Regards, Jan |
From: Jan S. <jst...@re...> - 2012-10-29 09:49:14
|
This test picked fixed address in setup() which was used throughout the test. There has been reports that it can fail with EINVAL. I could reproduce it by adding single printf in between detach/attach: http://article.gmane.org/gmane.linux.ltp/16480 This patch picks suitable base_addr before each test. So even if glibc would mmap extra page with addr == base_addr picked by setup(), new suitable address will be chosen. Signed-off-by: Jan Stancek <jst...@re...> --- testcases/kernel/syscalls/ipc/shmat/Makefile | 3 ++ testcases/kernel/syscalls/ipc/shmat/shmat01.c | 16 +-------- testcases/kernel/syscalls/ipc/shmat/shmat02.c | 16 +-------- testcases/kernel/syscalls/ipc/shmat/shmat_common.c | 33 ++++++++++++++++++++ 4 files changed, 40 insertions(+), 28 deletions(-) create mode 100644 testcases/kernel/syscalls/ipc/shmat/shmat_common.c diff --git a/testcases/kernel/syscalls/ipc/shmat/Makefile b/testcases/kernel/syscalls/ipc/shmat/Makefile index f467389..5cbf37c 100644 --- a/testcases/kernel/syscalls/ipc/shmat/Makefile +++ b/testcases/kernel/syscalls/ipc/shmat/Makefile @@ -20,4 +20,7 @@ top_srcdir ?= ../../../../.. include $(top_srcdir)/include/mk/testcases.mk include $(abs_srcdir)/../Makefile.inc + +MAKE_TARGETS := $(patsubst $(abs_srcdir)/%.c,%,$(wildcard $(abs_srcdir)/*[0-9].c)) + include $(top_srcdir)/include/mk/generic_leaf_target.mk diff --git a/testcases/kernel/syscalls/ipc/shmat/shmat01.c b/testcases/kernel/syscalls/ipc/shmat/shmat01.c index 6e4dee1..1f46b23 100644 --- a/testcases/kernel/syscalls/ipc/shmat/shmat01.c +++ b/testcases/kernel/syscalls/ipc/shmat/shmat01.c @@ -56,6 +56,7 @@ */ #include "ipcshm.h" +#include "shmat_common.c" char *TCID = "shmat01"; @@ -108,6 +109,7 @@ int main(int ac, char **av) /* * Use TEST macro to make the call */ + base_addr = probe_free_addr(); errno = 0; addr = shmat(*(TC[i].shmid), base_addr+TC[i].offset, TC[i].flags); @@ -261,20 +263,6 @@ void setup(void) tst_brkm(TBROK, cleanup, "Failed to create shared memory " "resource 1 in setup()"); } - - /* Probe an available linear address for attachment */ - if ((base_addr = shmat(shm_id_1, NULL, 0)) == (void *)-1) { - tst_brkm(TBROK, cleanup, "Couldn't attach shared memory"); - } - if (shmdt((const void *)base_addr) == -1) { - tst_brkm(TBROK, cleanup, "Couldn't detach shared memory"); - } - - /* some architectures (e.g. parisc) are strange, so better always align to - * next SHMLBA address. */ - base_addr = - (void *)(((unsigned long)(base_addr) + (SHMLBA - 1)) & - ~(SHMLBA - 1)); } /* diff --git a/testcases/kernel/syscalls/ipc/shmat/shmat02.c b/testcases/kernel/syscalls/ipc/shmat/shmat02.c index d4cca0e..9ac6794 100644 --- a/testcases/kernel/syscalls/ipc/shmat/shmat02.c +++ b/testcases/kernel/syscalls/ipc/shmat/shmat02.c @@ -55,6 +55,7 @@ #include "ipcshm.h" #include <pwd.h> +#include "shmat_common.c" char *TCID = "shmat02"; char nobody_uid[] = "nobody"; @@ -125,6 +126,7 @@ int main(int ac, char **av) setup_tc(i, tc); + base_addr = probe_free_addr(); errno = 0; addr = shmat(*(tc->shmid), base_addr + tc->offset, 0); @@ -180,20 +182,6 @@ void setup(void) if ((shm_id_3 = shmget(shmkey2, INT_SIZE, IPC_CREAT|IPC_EXCL)) == -1) tst_brkm(TBROK|TERRNO, cleanup, "shmget #2 failed"); - - /* Probe an available linear address for attachment */ - if ((base_addr = shmat(shm_id_2, NULL, 0)) == (void *)-1) - tst_brkm(TBROK|TERRNO, cleanup, "shmat #1 failed"); - - if (shmdt((const void *)base_addr) == -1) - tst_brkm(TBROK|TERRNO, cleanup, "shmat #2 failed"); - - /* - * some architectures (e.g. parisc) are strange, so better always align - * to next SHMLBA address - */ - base_addr = - (void *)(((unsigned long)(base_addr) & ~(SHMLBA - 1)) + SHMLBA); } /* diff --git a/testcases/kernel/syscalls/ipc/shmat/shmat_common.c b/testcases/kernel/syscalls/ipc/shmat/shmat_common.c new file mode 100644 index 0000000..5a920c8 --- /dev/null +++ b/testcases/kernel/syscalls/ipc/shmat/shmat_common.c @@ -0,0 +1,33 @@ +static key_t probe_key; + +void *probe_free_addr() +{ + void *p; + int ret; + int shm_id = -1; + + if (probe_key == 0) + probe_key = getipckey(); + + /* create a shared memory resource with read and write permissions + * We align this to SHMLBA so we should allocate at least + * SHMLBA*2 in case SHMLBA > page_size. */ + shm_id = shmget(probe_key, SHMLBA*2, SHM_RW | IPC_CREAT | IPC_EXCL); + if (shm_id == -1) + tst_brkm(TBROK, cleanup, "probe: shmget failed"); + + /* Probe an available linear address for attachment */ + p = shmat(shm_id, NULL, 0); + if (p == (void *)-1) + tst_brkm(TBROK, cleanup, "probe: shmat failed"); + ret = shmdt((const void *)p); + if (ret == -1) + tst_brkm(TBROK, cleanup, "probe: shmdt failed"); + + rm_shm(shm_id); + + /* some architectures (e.g. parisc) are strange, so better always + * align to next SHMLBA address. */ + p = (void *)(((unsigned long)(p) + (SHMLBA - 1)) & ~(SHMLBA - 1)); + return p; +} -- 1.7.1 |
From: <ch...@su...> - 2012-10-29 14:06:50
|
Hi! > diff --git a/testcases/kernel/syscalls/ipc/shmat/Makefile b/testcases/kernel/syscalls/ipc/shmat/Makefile > index f467389..5cbf37c 100644 > --- a/testcases/kernel/syscalls/ipc/shmat/Makefile > +++ b/testcases/kernel/syscalls/ipc/shmat/Makefile > @@ -20,4 +20,7 @@ top_srcdir ?= ../../../../.. > > include $(top_srcdir)/include/mk/testcases.mk > include $(abs_srcdir)/../Makefile.inc > + > +MAKE_TARGETS := $(patsubst $(abs_srcdir)/%.c,%,$(wildcard $(abs_srcdir)/*[0-9].c)) Hm, perhaps it would be easier to rename the shmat_common.c to shmat_common.h. > include $(top_srcdir)/include/mk/generic_leaf_target.mk > diff --git a/testcases/kernel/syscalls/ipc/shmat/shmat01.c b/testcases/kernel/syscalls/ipc/shmat/shmat01.c > index 6e4dee1..1f46b23 100644 > --- a/testcases/kernel/syscalls/ipc/shmat/shmat01.c > +++ b/testcases/kernel/syscalls/ipc/shmat/shmat01.c > @@ -56,6 +56,7 @@ > */ > > #include "ipcshm.h" > +#include "shmat_common.c" > > char *TCID = "shmat01"; > > @@ -108,6 +109,7 @@ int main(int ac, char **av) > /* > * Use TEST macro to make the call > */ Once you are touching the source, you are welcomed to remove dumbious comments like this (ideally in separate patch). ;) > diff --git a/testcases/kernel/syscalls/ipc/shmat/shmat_common.c b/testcases/kernel/syscalls/ipc/shmat/shmat_common.c > new file mode 100644 > index 0000000..5a920c8 > --- /dev/null > +++ b/testcases/kernel/syscalls/ipc/shmat/shmat_common.c > @@ -0,0 +1,33 @@ > +static key_t probe_key; > + > +void *probe_free_addr() Add void as the function parameters here. > +{ > + void *p; > + int ret; > + int shm_id = -1; > + > + if (probe_key == 0) > + probe_key = getipckey(); > + > + /* create a shared memory resource with read and write permissions > + * We align this to SHMLBA so we should allocate at least > + * SHMLBA*2 in case SHMLBA > page_size. */ > + shm_id = shmget(probe_key, SHMLBA*2, SHM_RW | IPC_CREAT | IPC_EXCL); > + if (shm_id == -1) > + tst_brkm(TBROK, cleanup, "probe: shmget failed"); > + > + /* Probe an available linear address for attachment */ > + p = shmat(shm_id, NULL, 0); > + if (p == (void *)-1) > + tst_brkm(TBROK, cleanup, "probe: shmat failed"); > + ret = shmdt((const void *)p); Hmm, is this cast really needed? > + if (ret == -1) > + tst_brkm(TBROK, cleanup, "probe: shmdt failed"); > + > + rm_shm(shm_id); > + > + /* some architectures (e.g. parisc) are strange, so better always > + * align to next SHMLBA address. */ > + p = (void *)(((unsigned long)(p) + (SHMLBA - 1)) & ~(SHMLBA - 1)); > + return p; > +} -- Cyril Hrubis ch...@su... |
From: Jan S. <jst...@re...> - 2012-10-30 10:32:01
|
----- Original Message ----- > From: ch...@su... > To: "Jan Stancek" <jst...@re...> > Cc: ltp...@li... > Sent: Monday, 29 October, 2012 3:04:25 PM > Subject: Re: [LTP] [PATCH v3] ipc/shmat: pick suitable base_addr before each testcase > > Hi! > > diff --git a/testcases/kernel/syscalls/ipc/shmat/Makefile > > b/testcases/kernel/syscalls/ipc/shmat/Makefile > > index f467389..5cbf37c 100644 > > --- a/testcases/kernel/syscalls/ipc/shmat/Makefile > > +++ b/testcases/kernel/syscalls/ipc/shmat/Makefile > > @@ -20,4 +20,7 @@ top_srcdir ?= ../../../../.. > > > > include $(top_srcdir)/include/mk/testcases.mk > > include $(abs_srcdir)/../Makefile.inc > > + > > +MAKE_TARGETS := $(patsubst > > $(abs_srcdir)/%.c,%,$(wildcard $(abs_srcdir)/*[0-9].c)) > > Hm, perhaps it would be easier to rename the shmat_common.c to > shmat_common.h. Yes, that would work too. > > --- /dev/null > > +++ b/testcases/kernel/syscalls/ipc/shmat/shmat_common.c > > @@ -0,0 +1,33 @@ > > +static key_t probe_key; > > + > > +void *probe_free_addr() > > Add void as the function parameters here. Of course, I always manage to miss this.. > > > +{ > > + void *p; > > + int ret; > > + int shm_id = -1; > > + > > + if (probe_key == 0) > > + probe_key = getipckey(); > > + > > + /* create a shared memory resource with read and write > > permissions > > + * We align this to SHMLBA so we should allocate at least > > + * SHMLBA*2 in case SHMLBA > page_size. */ > > + shm_id = shmget(probe_key, SHMLBA*2, SHM_RW | IPC_CREAT | > > IPC_EXCL); > > + if (shm_id == -1) > > + tst_brkm(TBROK, cleanup, "probe: shmget failed"); > > + > > + /* Probe an available linear address for attachment */ > > + p = shmat(shm_id, NULL, 0); > > + if (p == (void *)-1) > > + tst_brkm(TBROK, cleanup, "probe: shmat failed"); > > + ret = shmdt((const void *)p); > > Hmm, is this cast really needed? No, that was carried over from original code. Implicit conversion should work just fine. > > > + if (ret == -1) > > + tst_brkm(TBROK, cleanup, "probe: shmdt failed"); > > + > > + rm_shm(shm_id); > > + > > + /* some architectures (e.g. parisc) are strange, so better always > > + * align to next SHMLBA address. */ > > + p = (void *)(((unsigned long)(p) + (SHMLBA - 1)) & ~(SHMLBA - > > 1)); > > + return p; > > +} > > -- > Cyril Hrubis > ch...@su... > |
From: Jan S. <jst...@re...> - 2012-10-30 10:52:58
|
This test picked fixed address in setup() which was used throughout the test. There has been reports that it can fail with EINVAL. I could reproduce it by adding single printf in between detach/attach: http://article.gmane.org/gmane.linux.ltp/16480 This patch picks suitable base_addr before each test. So even if glibc would mmap extra page with addr == base_addr picked by setup(), new suitable address will be chosen. Signed-off-by: Jan Stancek <jst...@re...> --- testcases/kernel/syscalls/ipc/shmat/shmat01.c | 18 ++--------- testcases/kernel/syscalls/ipc/shmat/shmat02.c | 16 +-------- testcases/kernel/syscalls/ipc/shmat/shmat_common.h | 33 ++++++++++++++++++++ 3 files changed, 38 insertions(+), 29 deletions(-) create mode 100644 testcases/kernel/syscalls/ipc/shmat/shmat_common.h diff --git a/testcases/kernel/syscalls/ipc/shmat/shmat01.c b/testcases/kernel/syscalls/ipc/shmat/shmat01.c index 6e4dee1..b64cf61 100644 --- a/testcases/kernel/syscalls/ipc/shmat/shmat01.c +++ b/testcases/kernel/syscalls/ipc/shmat/shmat01.c @@ -56,6 +56,7 @@ */ #include "ipcshm.h" +#include "shmat_common.h" char *TCID = "shmat01"; @@ -108,6 +109,7 @@ int main(int ac, char **av) /* * Use TEST macro to make the call */ + base_addr = probe_free_addr(); errno = 0; addr = shmat(*(TC[i].shmid), base_addr+TC[i].offset, TC[i].flags); @@ -128,7 +130,7 @@ int main(int ac, char **av) * clean up things in case we are looping - in * this case, detach the shared memory */ - if (shmdt((const void *)addr) == -1) { + if (shmdt(addr) == -1) { tst_brkm(TBROK, cleanup, "Couldn't detach shared memory"); } @@ -261,20 +263,6 @@ void setup(void) tst_brkm(TBROK, cleanup, "Failed to create shared memory " "resource 1 in setup()"); } - - /* Probe an available linear address for attachment */ - if ((base_addr = shmat(shm_id_1, NULL, 0)) == (void *)-1) { - tst_brkm(TBROK, cleanup, "Couldn't attach shared memory"); - } - if (shmdt((const void *)base_addr) == -1) { - tst_brkm(TBROK, cleanup, "Couldn't detach shared memory"); - } - - /* some architectures (e.g. parisc) are strange, so better always align to - * next SHMLBA address. */ - base_addr = - (void *)(((unsigned long)(base_addr) + (SHMLBA - 1)) & - ~(SHMLBA - 1)); } /* diff --git a/testcases/kernel/syscalls/ipc/shmat/shmat02.c b/testcases/kernel/syscalls/ipc/shmat/shmat02.c index d4cca0e..af28d6c 100644 --- a/testcases/kernel/syscalls/ipc/shmat/shmat02.c +++ b/testcases/kernel/syscalls/ipc/shmat/shmat02.c @@ -55,6 +55,7 @@ #include "ipcshm.h" #include <pwd.h> +#include "shmat_common.h" char *TCID = "shmat02"; char nobody_uid[] = "nobody"; @@ -125,6 +126,7 @@ int main(int ac, char **av) setup_tc(i, tc); + base_addr = probe_free_addr(); errno = 0; addr = shmat(*(tc->shmid), base_addr + tc->offset, 0); @@ -180,20 +182,6 @@ void setup(void) if ((shm_id_3 = shmget(shmkey2, INT_SIZE, IPC_CREAT|IPC_EXCL)) == -1) tst_brkm(TBROK|TERRNO, cleanup, "shmget #2 failed"); - - /* Probe an available linear address for attachment */ - if ((base_addr = shmat(shm_id_2, NULL, 0)) == (void *)-1) - tst_brkm(TBROK|TERRNO, cleanup, "shmat #1 failed"); - - if (shmdt((const void *)base_addr) == -1) - tst_brkm(TBROK|TERRNO, cleanup, "shmat #2 failed"); - - /* - * some architectures (e.g. parisc) are strange, so better always align - * to next SHMLBA address - */ - base_addr = - (void *)(((unsigned long)(base_addr) & ~(SHMLBA - 1)) + SHMLBA); } /* diff --git a/testcases/kernel/syscalls/ipc/shmat/shmat_common.h b/testcases/kernel/syscalls/ipc/shmat/shmat_common.h new file mode 100644 index 0000000..06cf5ab --- /dev/null +++ b/testcases/kernel/syscalls/ipc/shmat/shmat_common.h @@ -0,0 +1,33 @@ +static key_t probe_key; + +void *probe_free_addr(void) +{ + void *p; + int ret; + int shm_id = -1; + + if (probe_key == 0) + probe_key = getipckey(); + + /* create a shared memory resource with read and write permissions + * We align this to SHMLBA so we should allocate at least + * SHMLBA*2 in case SHMLBA > page_size. */ + shm_id = shmget(probe_key, SHMLBA*2, SHM_RW | IPC_CREAT | IPC_EXCL); + if (shm_id == -1) + tst_brkm(TBROK, cleanup, "probe: shmget failed"); + + /* Probe an available linear address for attachment */ + p = shmat(shm_id, NULL, 0); + if (p == (void *)-1) + tst_brkm(TBROK, cleanup, "probe: shmat failed"); + ret = shmdt(p); + if (ret == -1) + tst_brkm(TBROK, cleanup, "probe: shmdt failed"); + + rm_shm(shm_id); + + /* some architectures (e.g. parisc) are strange, so better always + * align to next SHMLBA address. */ + p = (void *)(((unsigned long)(p) + (SHMLBA - 1)) & ~(SHMLBA - 1)); + return p; +} -- 1.7.1 |
From: Wanlong G. <gao...@cn...> - 2012-10-30 11:03:28
|
On 10/30/2012 06:52 PM, Jan Stancek wrote: > This test picked fixed address in setup() which was used throughout > the test. There has been reports that it can fail with EINVAL. > > I could reproduce it by adding single printf in between > detach/attach: > http://article.gmane.org/gmane.linux.ltp/16480 > > This patch picks suitable base_addr before each test. So even if glibc > would mmap extra page with addr == base_addr picked by setup(), > new suitable address will be chosen. > > Signed-off-by: Jan Stancek <jst...@re...> Pushed, thank you. Wanlong Gao |