[squid-dev] To make squid works in snap world.

[Alex Rousskov]

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.



> 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.


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('-');
>     >      }