Re: svn commit: r1326688 - /subversion/trunk/subversion/tests/libsvn_subr/named_atomic-test.c

[Greg Stein <gs...@gmail.com>]

On Apr 16, 2012 1:25 PM, <st...@apache.org> wrote:
>
> Author: stefan2
> Date: Mon Apr 16 17:25:18 2012
> New Revision: 1326688
>
> URL: http://svn.apache.org/viewvc?rev=1326688&view=rev
> Log:
> Skip named_atomics tests if the user does not have the necessary
> privileges under Windows.
>
> * subversion/tests/libsvn_subr/named_atomic-test.c
>  (has_sufficient_privileges): new function
>  (test_basics, test_bignums, test_multiple_atomics,
>   test_namespaces, test_multithreaded, test_multiprocess): call it

Don't we have something like SVN_TEST_SKIP(foo,
has_sufficient_privileges()) ?

That will produce "skipped" in the output. At a minimum the test should
return the ERR_SKIPPED "error" to produce the right output.

I also find the need for the has_sufficient_privileges() function to be
problematic. It seems to indicate you have not encapsulated this stuff
properly.

Cheers,
-g

Re: svn commit: r1326688 - /subversion/trunk/subversion/tests/libsvn_subr/named_atomic-test.c

[Stefan Fuhrmann <eq...@web.de>]

Greg Stein wrote:
>
>
> On Apr 22, 2012 2:09 PM, "Stefan Fuhrmann" <eqfox@web.de 
> <ma...@web.de>> wrote:
> >
> > Am 16.04.2012 19:45, schrieb Greg Stein:
> >>...
> >> I also find the need for the has_sufficient_privileges() function 
> to be problematic. It seems to indicate you have not encapsulated this 
> stuff properly.
> >
> > Indeed, one might see this as an encapsulation issue. With APR.
>
> No doubt. But that doesn't mean you propagate the problem into SVN. I 
> would suggest a helper function in libsvn_subr that does this 
> platform-specific stuff so that the app (or test, in this case) can 
> call that helper. That encapsulates stuff away from the app, even 
> though our library may still need to compensate for APR problems.
>

Addressed by r1331760.
Thanks for the review!

-- Stefan^2.

Re: svn commit: r1326688 - /subversion/trunk/subversion/tests/libsvn_subr/named_atomic-test.c

[Greg Stein <gs...@gmail.com>]

On Apr 22, 2012 2:09 PM, "Stefan Fuhrmann" <eq...@web.de> wrote:
>
> Am 16.04.2012 19:45, schrieb Greg Stein:
>>...
>> I also find the need for the has_sufficient_privileges() function to be
problematic. It seems to indicate you have not encapsulated this stuff
properly.
>
> Indeed, one might see this as an encapsulation issue. With APR.

No doubt. But that doesn't mean you propagate the problem into SVN. I would
suggest a helper function in libsvn_subr that does this platform-specific
stuff so that the app (or test, in this case) can call that helper. That
encapsulates stuff away from the app, even though our library may still
need to compensate for APR problems.

Cheers,
-g

Re: svn commit: r1326688 - /subversion/trunk/subversion/tests/libsvn_subr/named_atomic-test.c

[Stefan Fuhrmann <eq...@web.de>]

Am 16.04.2012 19:45, schrieb Greg Stein:
>
>
> On Apr 16, 2012 1:25 PM, <stefan2@apache.org 
> <ma...@apache.org>> wrote:
> >
> > Author: stefan2
> > Date: Mon Apr 16 17:25:18 2012
> > New Revision: 1326688
> >
> > URL: http://svn.apache.org/viewvc?rev=1326688&view=rev 
> <http://svn.apache.org/viewvc?rev=1326688&view=rev>
> > Log:
> > Skip named_atomics tests if the user does not have the necessary
> > privileges under Windows.
> >
> > * subversion/tests/libsvn_subr/named_atomic-test.c
> >  (has_sufficient_privileges): new function
> >  (test_basics, test_bignums, test_multiple_atomics,
> >   test_namespaces, test_multithreaded, test_multiprocess): call it
>
> Don't we have something like SVN_TEST_SKIP(foo, 
> has_sufficient_privileges()) ?
>
> That will produce "skipped" in the output. At a minimum the test 
> should return the ERR_SKIPPED "error" to produce the right output.
>

Changed in r1327444.
>
> I also find the need for the has_sufficient_privileges() function to 
> be problematic. It seems to indicate you have not encapsulated this 
> stuff properly.
>
Indeed, one might see this as an encapsulation issue. With APR.
Its SHM implementation requires specific privileges that most
services (i.e. the Apache server) etc. automatically get but typical
user accounts don't. Bottom line is, if you don't have those rights,
APR will not let you create SHMs.

The issue is mostly limited to Vista & later because users don't
work in session 0 anymore by default. To make SHMs visible
to all sessions, APR adds a "Global\" prefix and that is causing
the trouble. Although supported internally, we can't pass the
"local" flag through the APR API, which would prevent the error.

-- Stefan^2.