[squid-dev] To make squid works in snap world.
Gary Wang
gary.wang at canonical.com
Wed Mar 15 09:24:13 UTC 2017
Regarding the confinement of usage of shared memory in snap world,
Please take a look at the bug
https://bugs.launchpad.net/snappy/+bug/1653955
And Jamie's reply can be fuond in snapcraft maillist
https://www.mail-archive.com/snapcraft@lists.snapcraft.io/
msg02465.html
"sem_open() is https://bugs.launchpad.net/snappy/+bug/1653955 and
<https://bugs.launchpad.net/snappy/+bug/1653955%C2%A0and> snapd 2.21
added support to allow /{dev,run}/shm/sem.snap.@{SNAP_NAME}.*. This is
sufficient to make use of sem_open() possible."
On Wed, Mar 15, 2017 at 12:48 PM, Alex Rousskov <
rousskov at measurement-factory.com> wrote:
> On 03/14/2017 09:52 PM, Gary Wang wrote:
> > On Tue, Mar 14, 2017 at 11:49 PM, Alex Rousskov wrote:
> >> On 03/14/2017 08:44 AM, Gary Wang wrote:
> >> > About the DEFAULT_STATEDIR,
> >> > ...
> >> > DEFS += -DDEFAULT_STATEDIR=\"$(localstatedir)/run/squid\"
> >> > ...
> >> > According to the above share memory namespace in snap world,
> it
> >> > couldn't help on this either.
>
> >> Why not make DEFAULT_STATEDIR configurable instead?
>
> > A: Actually, DEFAULT_IPC_PREFIX only changes the shared memory file
> name
> > not the file path. So the usage of these two options are
> different.
>
> You are describing your implementation. I am asking why not support the
> same functionality using a _different_ general implementation that
> avoids adding a yet another option that controls IPC-related file names.
> You added an option that changes the "middle" part of the full file
> name. Why not make the existing constant that controls the "prefix" part
> of the full file name configurable instead?
>
> I understand that the two approaches result in [slightly] different
> code, but if a single option can accomplish the desired outcome and
> supports more use cases, then using that single option may be a better
> approach.
>
>
> > DEFAULT_STATEDIR only helps the case that the segment name is path.
>
> Can the updated/renamed DEFAULT_STATEDIR be interpreted as
>
> <path> / <filename prefix>
>
> where the first component is used if and only when nameIsPath is true
> and the second component is optional? AFAICT, such an implementation
> would cover the already supported use cases and your new use case.
>
> For snap environments, the option could be set to something like:
>
> $(localstatedir)/run/squid/sem.snap.squid.
>
> A: I understood your point that a single option can keep clean code base.
But interpreting DEFAULT_STATEDIR as <path> / <filename prefix>
is more like a trick. Wihout snap-related knowledge, normal developer
will
treat the the option as an absolute path instead of combination
of <path>/<filename prefix>
I can make a change to fix it based on your comment.
But please double-check if it's the right implementation.
Meanwhile, according to shared memory file namespace
/dev/shm/sem.snap.@{SNAP_NAME}.*
If we use the option in the following way
$(localstatedir)/run/squid/sem.snap.squid,
which means SNAP_NAME would be "squid",
I have a quick look at ubuntu store and found that "squid" snap name
is registered by
other developers. To avoid name disputes, there're two options
1. change
$(localstatedir)/run/squid/sem.snap.squid.
to
$(localstatedir)/run/squid/sem.snap.squid-snap.
2. make STATEDIR configurable so developer can customize it.
I perfer the latter one.
>
> > Another concern is that even though we make DEFAULT_STATEDIR
> > configurable instead of introducing new option, STATEDIR, IPCDIR is not
> > suitable as end-user might specify the dir path for these options
> however the
> > purpose of DEFAULT_IPC_PREFIX is only add prefix to shared memory file
> name
>
> I am not sure I understand the above concern, but perhaps my suggestion
> to ignore the path component (as needed) would address it?
>
>
> >> AFAICT, DEFAULT_STATEDIR is also used for IPC communications in
> >> src/ipc/Port.cc. Your patch does not change that code. This means
> that
> >>
> >> * either you forgot to change src/ipc/Port.cc to honor the new
> >> DEFAULT_IPC_PREFIX
>
> > A: I will change src/ipc/Port.cc in the next commit once we reach an
> > agreement on the usage of compiler option.
>
> For Squid to work in a snap world, will you need the same
> sem.snap.@{SNAP_NAME}. prefix for Unix Domain Sockets as for shared
> memory segments?
>
> * If yes, then using a single option for both ipc/mem/Segment.cc and
> ipc/Port.cc may work.
>
> * Otherwise, we would have to add two different options, one for shared
> memory segment names and one for UDS names.
>
> A: No, I don't need the same prefix for UDS names. As you said, we might
need two different options.
>
> Cheers,
>
> Alex.
>
>
> > * or src/ipc/Port.cc cannot use DEFAULT_IPC_PREFIX for some reason
> (and,
> > hence, DEFAULT_IPC_PREFIX is the wrong name because the parameter
> does
> > not apply to some of the IPC code).
> >
> > Alex.
> >
> >
> >
> > > === modified file 'configure.ac <http://configure.ac>'
> > > --- configure.ac <http://configure.ac> 2017-01-28 03:54:15
> +0000
> > > +++ configure.ac <http://configure.ac> 2017-03-02 03:51:46
> +0000
> > > @@ -311,6 +311,15 @@
> > > ])
> > > AC_SUBST(DEFAULT_SWAP_DIR)
> > >
> > > +DEFAULT_IPC_PREFIX=""
> > > +AC_ARG_WITH(ipcprefix,
> > > + AS_HELP_STRING([--with-ipcprefix=NAME],
> > > + [Default namespace prefix for shared memory object for ipc
> > communication.
> > > + Default is empty string and the share memory object name
> > will be specified by squid. ]), [
> > > + DEFAULT_IPC_PREFIX="$withval"
> > > +])
> > > +AC_SUBST(DEFAULT_IPC_PREFIX)
> > > +
> > > if test "$squid_cv_compiler" = "gcc"; then
> > > GCCVER=`$CC -v 2>&1 | awk '$2 == "version" {print $3}'`
> > > GCCVER2=`echo $GCCVER | awk '{print $1 * 100}'`
> > >
> > > === modified file 'src/ipc/Makefile.am'
> > > --- src/ipc/Makefile.am 2017-01-01 00:16:45 +0000
> > > +++ src/ipc/Makefile.am 2017-03-02 03:51:46 +0000
> > > @@ -68,7 +68,7 @@
> > > mem/Segment.cc \
> > > mem/Segment.h
> > >
> > > -DEFS += -DDEFAULT_STATEDIR=\"$(localstatedir)/run/squid\"
> > > +DEFS += -DDEFAULT_STATEDIR=\"$(localstatedir)/run/squid\"
> > -DDEFAULT_IPC_PREFIX=\"$(DEFAULT_IPC_PREFIX)\"
> > >
> > > install-data-local:
> > > $(mkinstalldirs) $(DESTDIR)$(localstatedir)/run/squid;
> > >
> > > === modified file 'src/ipc/mem/Segment.cc'
> > > --- src/ipc/mem/Segment.cc 2017-01-01 00:16:45 +0000
> > > +++ src/ipc/mem/Segment.cc 2017-03-02 03:51:46 +0000
> > > @@ -30,6 +30,8 @@
> > > #include <unistd.h>
> > > #endif
> > >
> > > +#define DEFAULT_SQUID_IPC_PREFIX "/" DEFAULT_IPC_PREFIX
> > > +
> > > // test cases change this
> > > const char *Ipc::Mem::Segment::BasePath = DEFAULT_STATEDIR;
> > >
> > > @@ -229,9 +231,9 @@
> > > if (nameIsPath) {
> > > name.append(BasePath);
> > > if (name[name.size()-1] != '/')
> > > - name.append('/');
> > > + name.append(DEFAULT_SQUID_IPC_PREFIX);
> > > } else {
> > > - name.append('/');
> > > + name.append(DEFAULT_SQUID_IPC_PREFIX);
> > > name.append(service_name.c_str());
> > > name.append('-');
> > > }
>
>
>
--
Br
Gary.Wzl
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.squid-cache.org/pipermail/squid-dev/attachments/20170315/7fef1db5/attachment-0001.html>
More information about the squid-dev
mailing list