You are viewing a plain text version of this content. The canonical link for it is here.

Posted to dev@subversion.apache.org by kf...@collab.net on 2004/06/23 19:33:07 UTC

Re: svn commit: r10060 - trunk/subversion/libsvn_fs

ghudson@tigris.org writes:
> +#ifndef WIN32
> +/* Required for the sgid code in svn_fs_create */
> +#include <sys/stat.h>
> +#include <sys/types.h>
> +#include <unistd.h>
> +#endif

Is this our normal way of saying "if Unix-like operating system"?

> +#ifndef WIN32
> +  /* Set the sgid bit on the directory, to ensure that group ownership
> +     of new files and directories will be inherited from the parent
> +     rather than determined by the primary gid.  APR provides no
> +     mechanism for setting this bit, so we do it the Unix way.  If we
> +     fail, don't sweat it; such a failure probably indicates the
> +     filesystem doesn't support that bit. */
> +  {
> +    struct stat st;
> +
> +    if (stat (path, &st) == 0)
> +      chmod (path, (st.st_mode & ~S_IFMT) | S_ISGID);
> +  }
> +#endif

(Same question, of course.)

I'm not necessarily objecting, just surprised that we test for the
*presence* of this functionality by seeing if we're *not* on a
particular OS.

-K


---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org

Re: Unix-only code (was Re: svn commit: r10060 - trunk/subversion/libsvn_fs)

Posted by John Peacock <jp...@rowman.com>.

Greg Hudson wrote:
> On Thu, 2004-06-24 at 13:55, John Peacock wrote:
>>The fact that the existing code makes that overly broad assumption
>>doesn't make it right.
> 
> 
> It makes it right in the context of this particular change.  If we've
> fallen into a particular pattern of bad-but-convenient behavior, it
> makes perfect sense to take advantage of that as long as it won't make
> it any harder to dig ourselves out of the hole at such a time as it
> becomes necessary.

I'm not arguing against the patch in question; I am arguing about the 
"we know it's suboptimal but the codebase already makes these poor 
assumptions so why not continue doing it that way" mindset.  If it is 
possible to add new code the correct way now (meaning it won't have to 
be fixed later), that should always be preferred to adding it knowing 
that it will have to be adjusted "at such time as it becomes necessary."

I just happen to be sensitive to this because I forsee having to clean 
up someone else's mess.

John

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org

Re: Unix-only code (was Re: svn commit: r10060 - trunk/subversion/libsvn_fs)

Posted by Greg Hudson <gh...@MIT.EDU>.

On Thu, 2004-06-24 at 13:55, John Peacock wrote:
> > No, but are there really that many people trying to build Subversion on
> > VMS?  It's about correctness, not numbers.

> But that is exactly my point.

No, it's a convenient rhetorical trick.  I agree that the WIN32 test is
incorrect, but you want to ditch it for another incorrect solution,
using "but how many people really care if it's incorrect?" as an
argument.

> The fact that the existing code makes that overly broad assumption
> doesn't make it right.

It makes it right in the context of this particular change.  If we've
fallen into a particular pattern of bad-but-convenient behavior, it
makes perfect sense to take advantage of that as long as it won't make
it any harder to dig ourselves out of the hole at such a time as it
becomes necessary.

> It's about correctness, not numbers. ;)

*I* never used numbers as an argument (except to demonstrate the fallacy
of your argument).

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org

Re: Unix-only code (was Re: svn commit: r10060 - trunk/subversion/libsvn_fs)

Posted by John Peacock <jp...@rowman.com>.

Greg Hudson wrote:
> On Thu, 2004-06-24 at 13:10, John Peacock wrote:
> 
>>Are there really that many people cross-compiling (if that is what you 
>>are talking about)?
> 
> 
> No, but are there really that many people trying to build Subversion on
> VMS?  It's about correctness, not numbers.
> 

But that is exactly my point.  Excluding codepaths based on the #define 
WIN32 as a binary test is demonstratably not correct; the world is much 
more complicated than just WIN32 and everything else, especially when 
you are trying to perform very Unix-specific bit-twiddling.  The fact 
that the existing code makes that overly broad assumption doesn't make 
it right.  It's about correctness, not numbers. ;)

John

-- 
John Peacock
Director of Information Research and Technology
Rowman & Littlefield Publishing Group
4501 Forbes Boulevard
Suite H
Lanham, MD  20706
301-459-3366 x.5010
fax 301-429-5748

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org

Re: Unix-only code (was Re: svn commit: r10060 - trunk/subversion/libsvn_fs)

Posted by Greg Hudson <gh...@MIT.EDU>.

On Thu, 2004-06-24 at 13:10, John Peacock wrote:
> Are there really that many people cross-compiling (if that is what you 
> are talking about)?

No, but are there really that many people trying to build Subversion on
VMS?  It's about correctness, not numbers.


---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org

Re: Unix-only code (was Re: svn commit: r10060 - trunk/subversion/libsvn_fs)

Posted by John Peacock <jp...@rowman.com>.

Greg Hudson wrote:

> On Thu, 2004-06-24 at 07:30, John Peacock wrote:
> 
>>Why not add a stanza to configure to probe for the feature?  This is always
>>the best method...
> 
> 
> That seems a little silly; as far as I know, configure only runs on
> systems which have this functionality.

I was actually proposing a more narrowly tailored #if test, that just so 
happens to work correctly where configure runs.  I fully expect to have 
to create a static configuration for VMS based on what configure would 
normally provide.

My main objection was to hiding #include's with WIN32, since that is too 
broad a brush to help anyone who isn't Unix OR Win32

> 
> I also don't know what a configure test for the sgid bit would look
> like.  (Remember that the host system isn't necessarily the same as the
> target system.)

Are there really that many people cross-compiling (if that is what you 
are talking about)?  I believe the only possible test (since this isn't 
a normally supported feature test) is to compile a stub program which 
attempts to perform the actions that your patch performs and then sees 
if it worked.

John

-- 
John Peacock
Director of Information Research and Technology
Rowman & Littlefield Publishing Group
4501 Forbes Boulevard
Suite H
Lanham, MD  20706
301-459-3366 x.5010
fax 301-429-5748

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org

Re: Unix-only code (was Re: svn commit: r10060 - trunk/subversion/libsvn_fs)

Posted by Joe Orton <jo...@manyfish.co.uk>.

On Fri, Jun 25, 2004 at 10:59:55AM -0400, Greg Hudson wrote:
> On Thu, 2004-06-24 at 17:47, Joe Orton wrote:
> > Support for the sticky/setgid/setuid bits has been in APR for a short
> > while, on both branches.
> 
> Thanks for pointing this out.  But you guys appear to have introduced a
> bad bug in the process: APR_OS_DEFAULT now includes APR_USETID,
> APR_GSETID, and APR_WSTICKY.  The first two bits are ignored on file and
> directory creation, but the third is not.

Ah good catch Greg.

Interestingly apr_file_open() interprets APR_OS_DEFAULT as meaning "use
0666" on Unix whereas apr_dir_make() really just maps the bits across.

I'll change the new constants to be 0x[248]000 to avoid this issue as
you suggest.  Thanks a lot.

> 
> Note that changing the definition of APR_OS_DEFAULT will not solve this
> problem, since programs compiled against apr 0.9.4 will have used the
> old APR_OS_DEFAULT value.  You need to use flag values outside of 0x0FFF
> for the new bits.
> 
> (All of the above refers to the 0.9 branch, but may also be true of the
> 1.0 branch.  Let me know if further action is required to make sure this
> gets fixed.)

Re: Unix-only code (was Re: svn commit: r10060 - trunk/subversion/libsvn_fs)

Posted by Joe Orton <jo...@manyfish.co.uk>.

On Fri, Jun 25, 2004 at 10:59:55AM -0400, Greg Hudson wrote:
> On Thu, 2004-06-24 at 17:47, Joe Orton wrote:
> > Support for the sticky/setgid/setuid bits has been in APR for a short
> > while, on both branches.
> 
> Thanks for pointing this out.  But you guys appear to have introduced a
> bad bug in the process: APR_OS_DEFAULT now includes APR_USETID,
> APR_GSETID, and APR_WSTICKY.  The first two bits are ignored on file and
> directory creation, but the third is not.

Ah good catch Greg.

Interestingly apr_file_open() interprets APR_OS_DEFAULT as meaning "use
0666" on Unix whereas apr_dir_make() really just maps the bits across.

I'll change the new constants to be 0x[248]000 to avoid this issue as
you suggest.  Thanks a lot.

> 
> Note that changing the definition of APR_OS_DEFAULT will not solve this
> problem, since programs compiled against apr 0.9.4 will have used the
> old APR_OS_DEFAULT value.  You need to use flag values outside of 0x0FFF
> for the new bits.
> 
> (All of the above refers to the 0.9 branch, but may also be true of the
> 1.0 branch.  Let me know if further action is required to make sure this
> gets fixed.)

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org

Re: Unix-only code (was Re: svn commit: r10060 - trunk/subversion/libsvn_fs)

Posted by Greg Hudson <gh...@MIT.EDU>.

On Thu, 2004-06-24 at 17:47, Joe Orton wrote:
> Support for the sticky/setgid/setuid bits has been in APR for a short
> while, on both branches.

Thanks for pointing this out.  But you guys appear to have introduced a
bad bug in the process: APR_OS_DEFAULT now includes APR_USETID,
APR_GSETID, and APR_WSTICKY.  The first two bits are ignored on file and
directory creation, but the third is not.

Note that changing the definition of APR_OS_DEFAULT will not solve this
problem, since programs compiled against apr 0.9.4 will have used the
old APR_OS_DEFAULT value.  You need to use flag values outside of 0x0FFF
for the new bits.

(All of the above refers to the 0.9 branch, but may also be true of the
1.0 branch.  Let me know if further action is required to make sure this
gets fixed.)

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org

Re: Unix-only code (was Re: svn commit: r10060 - trunk/subversion/libsvn_fs)

Posted by Joe Orton <jo...@manyfish.co.uk>.

On Thu, Jun 24, 2004 at 12:59:53PM -0400, Greg Hudson wrote:
> > Should be fairly safe then. Besides, those includes should be done
> > through apr_want.h.
> 
> Except those headers aren't in apr_want.h, and we can't add code to APR
> and use it in Subversion immediately.  (And if we were going to add code
> to APR, wouldn't we just add code to the file-attribute interface to
> allow setting the sgid bit?)

Support for the sticky/setgid/setuid bits has been in APR for a short
while, on both branches.

joe

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org

Unix-only code (was Re: svn commit: r10060 - trunk/subversion/libsvn_fs)

Posted by Greg Hudson <gh...@MIT.EDU>.

On Thu, 2004-06-24 at 07:30, John Peacock wrote:
> Doesn't make it the right method, though.  At some point, I plan on spending 
> some significant time getting Subversion working under VMS (I know, can't be 
> helped) and the more of these "If it's not WIN32 it _must_ be Unix" bodges I 
> have to dodge, the more difficult it is going to be.

Sorry.  As of r10065 the new one will be in the same place as most of
the others, at least.

> Why not add a stanza to configure to probe for the feature?  This is always
> the best method...

That seems a little silly; as far as I know, configure only runs on
systems which have this functionality.

I also don't know what a configure test for the sgid bit would look
like.  (Remember that the host system isn't necessarily the same as the
target system.)

brane wrote:
> Why not check for APR_HAVE_UNISTD_H instead?

Because the functionality has little to do with unistd.h.

What we could do is wrap each #include in a HAVE_FOO_H and then test for
S_ISGID.  I don't think that would really improve our portability
situation much, since it would still leave open the hypothetical case
where the system has S_ISGID but always fails when you try to set it on
a directory.  (Solaris has traditionally broken all sorts of autoconf
tests by providing functions and having them always return "not
implemented" error codes.)

> Should be fairly safe then. Besides, those includes should be done
> through apr_want.h.

Except those headers aren't in apr_want.h, and we can't add code to APR
and use it in Subversion immediately.  (And if we were going to add code
to APR, wouldn't we just add code to the file-attribute interface to
allow setting the sgid bit?)

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org