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