| Message ID | CAJ+F1C+fmnbhUaS=dtR7hRTogwSSd2CbGSmLJ=cKMrM5PyF2pg@mail.gmail.com |
|---|---|
| State | New |
| Headers | show |
On 26 October 2015 at 09:28, Marc-André Lureau <marcandre.lureau@gmail.com> wrote: > Hi Peter, > > On Mon, Oct 19, 2015 at 5:57 PM, Peter Maydell <peter.maydell@linaro.org> wrote: >> >> What is happening here is that we are looping infinitely in >> mktempshmem() because shm_open() returns -1 with errno ENOSYS, >> and there's no code in the loop that stops the loop on anything >> except shm_open succeeding, or even prints anything out about >> shm_open failing. >> >> I think this is failing for me because my system's chroot doesn't have >> /dev/shm mounted. It would be nice if we could at a minimum handle >> this reasonably gracefully... > > The following diff works for me: > > diff --git a/tests/ivshmem-test.c b/tests/ivshmem-test.c > index efaa6e3..c8f0cf0 100644 > --- a/tests/ivshmem-test.c > +++ b/tests/ivshmem-test.c > @@ -441,13 +441,18 @@ static gchar *mktempshm(int size, int *fd) > } > > g_free(name); > + > + if (errno != EEXIST) { > + perror("shm_open"); > + return NULL; > + } > } > } > > int main(int argc, char **argv) > { > int ret, fd; > gchar dir[] = "/tmp/ivshmem-test.XXXXXX"; > > #if !GLIB_CHECK_VERSION(2, 31, 0) > if (!g_thread_supported()) { > @@ -460,6 +465,9 @@ int main(int argc, char **argv) > qtest_add_abrt_handler(abrt_handler, NULL); > /* shm */ > tmpshm = mktempshm(TMPSHMSIZE, &fd); > + if (!tmpshm) { > + return 0; > + } > tmpshmem = mmap(0, TMPSHMSIZE, PROT_READ|PROT_WRITE, MAP_SHARED, fd, 0); > g_assert(tmpshmem != MAP_FAILED); > /* server */ This will print a cryptic error message and then not fail the test, which is not great. Maybe that's ok for the moment in the interests of not keeping this huge patchset out of tree for too long[*], but we should look at what glib's test framework provides in the way of being able to report "skipped this test" outcomes. [*] Incidentally this whole saga demonstrates why my general recommendation is to keep pull requests at much less than 50 patches... > I rebased and updated the tag. If you mean by this "please retry the pull" you should send a fresh coverletter email so my scripts will pick it up... thanks -- PMM
Hi On Mon, Oct 26, 2015 at 10:58 AM, Peter Maydell <peter.maydell@linaro.org> wrote: > This will print a cryptic error message and then not fail the test, > which is not great. Maybe that's ok for the moment in the interests > of not keeping this huge patchset out of tree for too long[*], but > we should look at what glib's test framework provides in the way > of being able to report "skipped this test" outcomes. g_test_skip() is since 2.38 (and can't be added in compat, because it uses internal variable etc) Furthermore, the shm error is a precondition for all the tests, it doesn't fit well with g_test_skip() which is inside the individual unit tests. > [*] Incidentally this whole saga demonstrates why my general > recommendation is to keep pull requests at much less than > 50 patches... > >> I rebased and updated the tag. > > If you mean by this "please retry the pull" you should send a fresh > coverletter email so my scripts will pick it up... ok
diff --git a/tests/ivshmem-test.c b/tests/ivshmem-test.c index efaa6e3..c8f0cf0 100644 --- a/tests/ivshmem-test.c +++ b/tests/ivshmem-test.c @@ -441,13 +441,18 @@ static gchar *mktempshm(int size, int *fd) } g_free(name); + + if (errno != EEXIST) { + perror("shm_open"); + return NULL; + } } } int main(int argc, char **argv) { int ret, fd; gchar dir[] = "/tmp/ivshmem-test.XXXXXX"; #if !GLIB_CHECK_VERSION(2, 31, 0) if (!g_thread_supported()) { @@ -460,6 +465,9 @@ int main(int argc, char **argv) qtest_add_abrt_handler(abrt_handler, NULL); /* shm */ tmpshm = mktempshm(TMPSHMSIZE, &fd); + if (!tmpshm) { + return 0; + } tmpshmem = mmap(0, TMPSHMSIZE, PROT_READ|PROT_WRITE, MAP_SHARED, fd, 0); g_assert(tmpshmem != MAP_FAILED); /* server */