Re: [PATCH] Fix testRock on FreeBSD

On 09/21/2012 02:43 AM, Kinkie wrote:
> On Thu, Sep 20, 2012 at 11:46 PM, Alex Rousskov
> <rousskov_at_measurement-factory.com> wrote:
>> On 09/19/2012 10:38 AM, Kinkie wrote:
>>
>>> I've done some testing on FreeBSD 9 (which fails the unit tests in testRock).
>>> Apparently the issue is caused by shm_open in the ipc channels, which
>>> fails with EINVAL because the shm path is not absolute.
>>> This patch changes the path for the unit test shm to /tmp; it is
>>> confirmed working on FreeBSD-9; couldn't be tested on OpenSuSE yet.
>>
>> Hi Kinkie,
>>
>> Thank you for working on this. The test case is using the current
>> directory for the cache files (TESTDIR). It would be better to keep
>> using the current directory for shared memory files as well, to reduce
>> the number of different directories we need to worry about (each of them
>> could lead to troubles in one environment or another). This can be
>> accomplished while still satisfying the "absolute path" restriction by
>> getting the current directory name using getcwd(3), right?
>>
>> However, I cannot be against the proposed simple change since it fixes
>> one build while there is no substantial evidence that it may break
>> others. Your call wither using getcwd(3) is better than "/tmp".
>
>> BTW, what happens if two concurrent "make check" sessions try to use
>> /tmp for this test case? Will one of them occasionally fail due to
>> filename collisions? I think the probability of that collision happening
>> is very low, even in automated build environments, but you might want to
>> add a comment about it.
>
>
> Hi Alex,
> The best solution at all to avoid filename collisions would be to
> use a guaranteed-unique directory (e.g. using mktemp(3)), or to rely
> on something guaranteed unique (e.g. the pid).
>
> I've changed to use getcwd(3). Tested OK on Ubuntu.

I suggest adding "#if HAVE_UNISTD_H" (because other Squid code uses
that, albeit not consistently) and committing to see what happens on the
build farm.

You may also want to check for NULL return for getcwd() (our main.cc
does that) and use "." if it is NULL, but such cases should be rare.

HTH,

Alex.

>
> === modified file 'src/tests/testRock.cc'
> --- src/tests/testRock.cc 2012-09-19 15:25:23 +0000
> +++ src/tests/testRock.cc 2012-09-21 08:00:11 +0000
> @@ -21,6 +21,8 @@
> #if HAVE_STDEXCEPT
> #include <stdexcept>
> #endif
> +#include <unistd.h>
> +
>
> #define TESTDIR "testRock__testRockSearch"
>
> @@ -28,6 +30,8 @@
>
> extern REMOVALPOLICYCREATE createRemovalPolicy_lru;
>
> +static char cwd[MAXPATHLEN];
> +
> static void
> addSwapDir(testRock::SwapDirPointer aStore)
> {
> @@ -45,7 +49,7 @@
> throw std::runtime_error("Failed to clean test work directory");
>
> // use current directory for shared segments (on path-based OSes)
> - Ipc::Mem::Segment::BasePath = "/tmp";
> + Ipc::Mem::Segment::BasePath = getcwd(cwd,MAXPATHLEN);
>
> Store::Root(new StoreController);
>
>
>
>
Received on Sat Sep 22 2012 - 16:49:41 MDT