Re: [PATCH] Name shared memory segments in a more portable way

On 10/25/2011 09:12 AM, Alex Rousskov wrote:
> On 10/25/2011 01:50 AM, Kinkie wrote:
>> On Tue, Oct 25, 2011 at 8:05 AM, Kinkie <gkinkie_at_gmail.com> wrote:
>>> On Tue, Oct 25, 2011 at 7:33 AM, Alex Rousskov
>>> <rousskov_at_measurement-factory.com> wrote:
>>>> Hello,
>>>>
>>>> This patch should make shm_open() work on FreeBSD and some other
>>>> OSes. It is untested beyond test-builds.sh on Linux, but is based on
>>>> working boost::interprocess code. The preamble has the details.
>>>>
>>>
>>> I'm giving it a spin in the build farm on freebsd and openbsd.
>
> Thank you.
>
>> It's building but failing unit tests:
>> FreeBSD:
>> http://build.squid-cache.org/job/3.ALPHA-PATCH-amd64-FreeBSD-7.2/20/consoleText
>
> There are two problems here:
>
> 1) I guessed the return value of shm_portable_segment_name_is_path wrong
> for FreeBSD v7+ jails (at least).
>
> 2) Fixing #1 exposes a more serious problem:
>
>> Fatal: Ipc::Mem::Segment::create failed to shm_open(/usr/local/squid/var/run/squidtestRock__testRockSearch): (2) No such file or directory
>
> Here, our "use PREFIX for FreeBSD segment names" logic kicks in as it
> should, but the test fails because there is no /usr/local/squid/var/run/
> when/where the test runs. PREFIX-based names do not work for tests. I
> see two possible solutions:
>
> A) Create a special "we are in a test mode", "use current dir", or a
> similar flag so that shm_open can use current dir for shared segments
> during testing. Special flags are ugly, but this will allow general code
> to continue to use path-agnostic names for shared segments.
>
> B) Switch to path-based names for shared segments in the general code.
> When general code needs to name a segment, it will call a
> Segment::GenerateName()-like functions explicitly. The shm_open() code
> itself will no longer try to add a path when needed. This will make the
> general code a little more complex, but the test cases can then use
> names that do not start with a slash. This is what Store test cases
> already do for cache_dir names.
>
> I favor (B) because it is cleaner and more flexible, even though it is
> more work.

I tried to implement (B) but it turned out to be a bad idea: Store has
several internal objects that are named automatically, without the
calling code explicit participation. For example, the shared page pool
name is internal to the pool code. Test cases do not have access to all
those internal names and, hence, cannot force them to use local paths.
Moreover, test cases should not really know about all internal objects
that need a name.

I am almost done implementing an (A)-like approach, where Segment uses
PREFIX-based paths by default but test cases can overwrite a public
Segment::BasePath member to use the current directory instead. It is not
very elegant, but it is simple:

> +// test cases change this
> +const char *Ipc::Mem::Segment::BasePath = DEFAULT_STATEDIR;
...
> String
> Ipc::Mem::Segment::GenerateName(const char *id)
> {
> + assert(BasePath);
> + static const bool nameIsPath = shm_portable_segment_name_is_path();
> + String name(nameIsPath ? BasePath : "/squid-");
> +
> + // append id, replacing slashes with dots
...

I should be able to test this within 24 hours.

Thank you,

Alex.
Received on Wed Oct 26 2011 - 05:34:14 MDT