Re: Fresh patch for Solaris
In message [EMAIL PROTECTED], Theo Schlossnagle writes: http://lethargy.org/~jesus/misc/varnish-solaris-trunk-3071.diff OK, I've picked the obvious stuff out. Various questions about the rest: Doesn't Solaris have fcntl(F_SETLK) as mandated by POSIX ? flock() is not a standardized API, and I havn't seen a system yet which supports it, which doesn't also have fcntl(F_SETLK), so I would rather not mess up the source with a pointless check. Do you know for sure that sendfile on Solaris has no reference to the relevant parts of the file when it returns ? Otherwise it is not safe to use. Why is the extra include of sys/statvfs.h necessary in storage_file.c ? Is there a reason to name the shared object .so instead of .o or is it just cosmetics ? In cache_acceptor.c, please implement the -pass function which does the port_send() call. Why the initialization in mgt_child.c ? -- Poul-Henning Kamp | UNIX since Zilog Zeus 3.20 [EMAIL PROTECTED] | TCP/IP since RFC 956 FreeBSD committer | BSD since 4.3-tahoe Never attribute to malice what can adequately be explained by incompetence. ___ varnish-dev mailing list varnish-dev@projects.linpro.no http://projects.linpro.no/mailman/listinfo/varnish-dev
Re: Fresh patch for Solaris
On Aug 11, 2008, at 5:30 AM, Poul-Henning Kamp wrote: In message [EMAIL PROTECTED], Theo Schlossnagle writes: http://lethargy.org/~jesus/misc/varnish-solaris-trunk-3071.diff OK, I've picked the obvious stuff out. Various questions about the rest: Doesn't Solaris have fcntl(F_SETLK) as mandated by POSIX ? flock() is not a standardized API, and I havn't seen a system yet which supports it, which doesn't also have fcntl(F_SETLK), so I would rather not mess up the source with a pointless check. Ah, good catch. That wasn't a patch for Solaris. It was for Linux. We've run into some issues with flock being more reliable than fcntl on Linux under high concurrency environments. We ran into this problem on another project and thought to make a preemptive strike in Varnish. However, my patch prefers flock over fcntl, which is clearly wrong as it might do that on BSD*. flock() on Solaris is only available with - lucb (the meager BSD-ish compatibility layer). So, with that patch, Solaris still uses fcntl. (Go POSIX!) The ugly way we did it on the other project (FastXSL) that had this issue is here: http://labs.omniti.com/trac/fastxsl/changeset/43 Do you know for sure that sendfile on Solaris has no reference to the relevant parts of the file when it returns ? Otherwise it is not safe to use. I asked the internal engineering team at Sun, they said that it has no references. I asked move forcefully and they did a code review (albeit very short) and said again that it had no references. So, at this point, I believe that it is safe to use in Varnish. Why is the extra include of sys/statvfs.h necessary in storage_file.c ? My mistake. I wasn't thorough when I merged up to trunk. No conflict and my lack of attention. Ignore that. Is there a reason to name the shared object .so instead of .o or is it just cosmetics ? The Sun Studio toolchain barfs on the .o. It knows better and seems to ignore the request to make it shared. I could get around this with a shell scripts (as you had once suggested), but this change makes it work in all the toolchains I have tried out of the box In cache_acceptor.c, please implement the -pass function which does the port_send() call. Will do. Not sure how that got reverted actually. Why the initialization in mgt_child.c ? I believe that is left over from something else. A valgrind warning -- but that could only have been due to other code I added and then removed. It looks clean and safe without initialization. -- Theo Schlossnagle Esoteric Curio -- http://lethargy.org/ OmniTI Computer Consulting, Inc. -- http://omniti.com/ ___ varnish-dev mailing list varnish-dev@projects.linpro.no http://projects.linpro.no/mailman/listinfo/varnish-dev
Re: Fresh patch for Solaris
On Aug 11, 2008, at 5:30 AM, Poul-Henning Kamp wrote: In message [EMAIL PROTECTED], Theo Schlossnagle writes: http://lethargy.org/~jesus/misc/varnish-solaris-trunk-3071.diff OK, I've picked the obvious stuff out. Next pass: http://lethargy.org/~jesus/misc/varnish-solaris-trunk-3080.diff cleaned up some redundant includes removed the flock stuff (not Solaris related) made the cache_acceptor_ports use -pass removed unnecessary things (includes, defines and initializations... removed from my patch only) made the umem allocator a new file instead of a modified svn copy of the malloc allocator. Patch is about half the size of the previous one. AWESOME progress. This is great. Thanks for your attention phk! -- Theo Schlossnagle Esoteric Curio -- http://lethargy.org/ OmniTI Computer Consulting, Inc. -- http://omniti.com/ ___ varnish-dev mailing list varnish-dev@projects.linpro.no http://projects.linpro.no/mailman/listinfo/varnish-dev
Re: Fresh patch for Solaris
In message [EMAIL PROTECTED], Theo Schlossnagle writes: Next pass: http://lethargy.org/~jesus/misc/varnish-solaris-trunk-3080.diff You still have the err.h compat stuff, that's not necessary any more, I removed the two uses of err(). Why is the tcp.c patch necessay ? You have not answered my questions about .so and sendfile ? -- Poul-Henning Kamp | UNIX since Zilog Zeus 3.20 [EMAIL PROTECTED] | TCP/IP since RFC 956 FreeBSD committer | BSD since 4.3-tahoe Never attribute to malice what can adequately be explained by incompetence. ___ varnish-dev mailing list varnish-dev@projects.linpro.no http://projects.linpro.no/mailman/listinfo/varnish-dev
Re: Fresh patch for Solaris
Sorry for the comment at the end of my prevous email, got your emails in reverse order here :-) Doesn't Solaris have fcntl(F_SETLK) as mandated by POSIX ? Ah, good catch. That wasn't a patch for Solaris. It was for Linux. We've run into some issues with flock being more reliable than fcntl on Linux under high concurrency environments. We're not even remotely close to concurrency, so I'd prefer to stick with fcntl until we see any actual problems. Is there a reason to name the shared object .so instead of .o or is it just cosmetics ? The Sun Studio toolchain barfs on the .o. It knows better [...] Ok, fair enough. -- Poul-Henning Kamp | UNIX since Zilog Zeus 3.20 [EMAIL PROTECTED] | TCP/IP since RFC 956 FreeBSD committer | BSD since 4.3-tahoe Never attribute to malice what can adequately be explained by incompetence. ___ varnish-dev mailing list varnish-dev@projects.linpro.no http://projects.linpro.no/mailman/listinfo/varnish-dev
Re: Fresh patch for Solaris
Next pass: http://lethargy.org/~jesus/misc/varnish-solaris-trunk-3080-2.diff On Aug 11, 2008, at 10:47 AM, Poul-Henning Kamp wrote: You still have the err.h compat stuff, that's not necessary any more, I removed the two uses of err(). Didn't catch that. Fixed. Why is the tcp.c patch necessay ? FIONBIO isn't available by default as it is a BSD thing. You're supposed to use fcntl on Solaris. fcntl is slower and ioctl _will_ work, but the define of FIONBIO is in sys/filio.h. You get it if you turn on BSD compatibility when including ioctl.h, but we don't want the other stuff that comes with that. So far, that single line of code is the only one requiring BSD_COMP on Solaris side, so I'm hesitant to -DBSD_COMP the whole thing. You have not answered my questions about .so and sendfile ? .o - .so is not purely cosmetic. There is an issue with the Sun Studio toolchain that makes .o not work. sendfile on Solaris should be safe. When the call returns no bits should be referenced at all. -- Theo Schlossnagle Esoteric Curio -- http://lethargy.org/ OmniTI Computer Consulting, Inc. -- http://omniti.com/ ___ varnish-dev mailing list varnish-dev@projects.linpro.no http://projects.linpro.no/mailman/listinfo/varnish-dev
