Re: svn commit: r1004286 - in /subversion/trunk: ./ subversion/libsvn_subr/io.c

Daniel Shahaf wrote on Tue, Jan 11, 2011 at 18:34:57 +0200: > Ramkumar Ramachandra wrote on Tue, Jan 11, 2011 at 21:33:33 +0530: > > - /* Get the perms for a newly created file to find out what bits > > -should be set. > > - > > -Normally del_on_close can be problematic because

Re: svn commit: r1004286 - in /subversion/trunk: ./ subversion/libsvn_subr/io.c

On Tue, 2011-01-11, Daniel Shahaf wrote: > I believe this is semantically correct. > > Ramkumar, Julian: is the struct still needed? IIRC the reasons for > originally introducing (namely the 'volatile' qualifier) it have since > disappeared. It's not strictly needed but it's a good way to struct

Re: svn commit: r1004286 - in /subversion/trunk: ./ subversion/libsvn_subr/io.c

I believe this is semantically correct. Ramkumar, Julian: is the struct still needed? IIRC the reasons for originally introducing (namely the 'volatile' qualifier) it have since disappeared. Daniel Ramkumar Ramachandra wrote on Tue, Jan 11, 2011 at 21:33:33 +0530: > Hi Julian and Daniel, > >

Re: svn commit: r1004286 - in /subversion/trunk: ./ subversion/libsvn_subr/io.c

Hi Julian and Daniel, Here's another revision after your suggestions on IRC. Thanks. [[[ Determine default perms in an elegant thread-safe way, not racily. * subversion/libsvn_subr/io.c (default_perms_baton, perms_init_state): New struct, variable. (get_default_file_perms): Remove all func

Re: svn commit: r1004286 - in /subversion/trunk: ./ subversion/libsvn_subr/io.c

Hi Julian and Daniel, Julian Foad writes: > Just a few comments on the implementation from me... Thanks for the wonderful review! I've also included a few of Daniel's suggestions (on IRC)- I hope I've got it right this time. I'm not sure the volatile qualifier is necessary either- I'm just using

Re: svn commit: r1004286 - in /subversion/trunk: ./ subversion/libsvn_subr/io.c

Just a few comments on the implementation from me... Daniel Shahaf wrote: > I looked at this patch, I'm not happy with it, but my issues are so > specific (eg: variable's initialization has problem) that > I feel I'd be *dictating* a rewrite of the patch if I reviewed it. > > And that is bad f

Re: svn commit: r1004286 - in /subversion/trunk: ./ subversion/libsvn_subr/io.c

I looked at this patch, I'm not happy with it, but my issues are so specific (eg: variable's initialization has problem) that I feel I'd be *dictating* a rewrite of the patch if I reviewed it. And that is bad form, so I'm not going to do that... Sorry, Daniel Ramkumar Ramachandra wrote on S

Re: svn commit: r1004286 - in /subversion/trunk: ./ subversion/libsvn_subr/io.c

Hi Daniel, Daniel Shahaf writes: > If you send another patch that indents/dedents a block, could you please > attach a 'svn diff -x-w' version of the patch too? It would make > reviewing easier. Sure. Here's the (hopefully) final patch. Sorry about the slopiness earlier -- I was in a hurry to g

Re: svn commit: r1004286 - in /subversion/trunk: ./ subversion/libsvn_subr/io.c

Another bug in the baton usage: Daniel Shahaf wrote on Sat, Jan 08, 2011 at 14:00:04 +0200: > Ramkumar Ramachandra wrote on Sat, Jan 08, 2011 at 16:06:01 +0530: > > +/* the default permissions as read from the temp folder */ > > +static volatile apr_fileperms_t default_perms = 0; > > +static volat

Re: svn commit: r1004286 - in /subversion/trunk: ./ subversion/libsvn_subr/io.c

Ramkumar Ramachandra wrote on Sat, Jan 08, 2011 at 16:06:01 +0530: > Hi Daniel, > > Daniel Shahaf writes: > > Ping, this doesn't seem to have been fixed yet? > > Thanks for the ping, and sorry about the delay. I think this should > work -- I'll write a commit message if you think this is okay. >

Re: svn commit: r1004286 - in /subversion/trunk: ./ subversion/libsvn_subr/io.c

Hi Daniel, Daniel Shahaf writes: > Ping, this doesn't seem to have been fixed yet? Thanks for the ping, and sorry about the delay. I think this should work -- I'll write a commit message if you think this is okay. Index: subversion/libsvn_subr/io.c ===

Re: svn commit: r1004286 - in /subversion/trunk: ./ subversion/libsvn_subr/io.c

e.org > > Subject: svn commit: r1004286 - in /subversion/trunk: ./ > > subversion/libsvn_subr/io.c > > > > Author: artagnon > > Date: Mon Oct 4 15:26:44 2010 > > New Revision: 1004286 > > > > URL: http://svn.apache.org/viewvc?rev=100

Re: svn commit: r1004286 - in /subversion/trunk: ./ subversion/libsvn_subr/io.c

Ramkumar Ramachandra writes: > Hi Philip, > > Philip Martin writes: >> Most of the functions don't share any state between threads, so they >> have no atomic issues. svn_io_temp_dir is one of the few that does, >> so it uses svn_atomic__init_once. I think the file perms stuff should >> also use

Re: svn commit: r1004286 - in /subversion/trunk: ./ subversion/libsvn_subr/io.c

Hi Philip, Philip Martin writes: > Most of the functions don't share any state between threads, so they > have no atomic issues. svn_io_temp_dir is one of the few that does, > so it uses svn_atomic__init_once. I think the file perms stuff should > also use it. I see. Could you point me to some

Re: svn commit: r1004286 - in /subversion/trunk: ./ subversion/libsvn_subr/io.c

Ramkumar Ramachandra writes: > Hi Philip, > > [sorry about the delayed reply; was ill] > > Philip Martin writes: >> Ramkumar Ramachandra writes: >> > Hm. I read up a little more about this, but what confuses me is- >> > shouldn't the rest of the code already be needing this? >> >> I don't under

Re: svn commit: r1004286 - in /subversion/trunk: ./ subversion/libsvn_subr/io.c

Hi Philip, [sorry about the delayed reply; was ill] Philip Martin writes: > Ramkumar Ramachandra writes: > > Hm. I read up a little more about this, but what confuses me is- > > shouldn't the rest of the code already be needing this? > > I don't understand your questions. To what does "rest of

Re: svn commit: r1004286 - in /subversion/trunk: ./ subversion/libsvn_subr/io.c

Ramkumar Ramachandra writes: > Hi Philip, > > [sorry about the delayed reply: I had a bad internet connection] > > Philip Martin writes: >> If we are going to use the APR atomic interface then the two reads >> should use apr_atomic_read32. >> >> It would be better to use svn_atomic__init_once.

Re: svn commit: r1004286 - in /subversion/trunk: ./ subversion/libsvn_subr/io.c

Hi Philip, [sorry about the delayed reply: I had a bad internet connection] Philip Martin writes: > If we are going to use the APR atomic interface then the two reads > should use apr_atomic_read32. > > It would be better to use svn_atomic__init_once. It's a clear > indication that we are doing

Re: svn commit: r1004286 - in /subversion/trunk: ./ subversion/libsvn_subr/io.c

Ramkumar Ramachandra writes: > Right, got it. Can I get a +1 for the following patch? If we are going to use the APR atomic interface then the two reads should use apr_atomic_read32. It would be better to use svn_atomic__init_once. It's a clear indication that we are doing once only initialisa

Re: svn commit: r1004286 - in /subversion/trunk: ./ subversion/libsvn_subr/io.c

Hi Bert, Bert Huijben writes: > > -Original Message- > > From: Ramkumar Ramachandra [mailto:artag...@gmail.com] > > > > Index: subversion/libsvn_subr/io.c > > === > > --- subversion/libsvn_subr/io.c (revision 1005706) > >

RE: svn commit: r1004286 - in /subversion/trunk: ./ subversion/libsvn_subr/io.c

> -Original Message- > From: Ramkumar Ramachandra [mailto:artag...@gmail.com] > Sent: maandag 11 oktober 2010 14:54 > To: Bert Huijben > Cc: dev@subversion.apache.org; 'Greg Stein'; 'Julian Foad' > Subject: Re: svn commit: r1004286 - in /subversion/t

Re: svn commit: r1004286 - in /subversion/trunk: ./ subversion/libsvn_subr/io.c

Hi Bert and Julian, Bert Huijben writes: > > -Original Message- > > From: Julian Foad [mailto:julian.f...@wandisco.com] > > On Mon, 2010-10-11, Julian Foad wrote: > > > > an-int-atomic>. > > > "On an IA32 a correctly a

RE: svn commit: r1004286 - in /subversion/trunk: ./ subversion/libsvn_subr/io.c

> -Original Message- > From: Julian Foad [mailto:julian.f...@wandisco.com] > Sent: maandag 11 oktober 2010 11:46 > To: Bert Huijben > Cc: 'Greg Stein'; dev@subversion.apache.org; artag...@apache.org > Subject: RE: svn commit: r1004286 - in /subversion/trunk:

RE: svn commit: r1004286 - in /subversion/trunk: ./ subversion/libsvn_subr/io.c

On Mon, 2010-10-11, Julian Foad wrote: > On Mon, 2010-10-11, Bert Huijben wrote: > > > -Original Message- > > > From: Greg Stein [mailto:gst...@gmail.com] > > > On Mon, Oct 11, 2010 at 04:58, Bert Huijben wrote: > > > >> -Original Message- > > > >> From: artag...@apache.org [mailto

RE: svn commit: r1004286 - in /subversion/trunk: ./ subversion/libsvn_subr/io.c

gt; > >> Sent: maandag 4 oktober 2010 17:27 > > >> To: comm...@subversion.apache.org > > >> Subject: svn commit: r1004286 - in /subversion/trunk: ./ > > >> subversion/libsvn_subr/io.c > > >> > > >> Author: artagnon > > >> D

RE: svn commit: r1004286 - in /subversion/trunk: ./ subversion/libsvn_subr/io.c

> -Original Message- > From: Greg Stein [mailto:gst...@gmail.com] > Sent: maandag 11 oktober 2010 11:03 > To: Bert Huijben > Cc: dev@subversion.apache.org; artag...@apache.org > Subject: Re: svn commit: r1004286 - in /subversion/trunk: ./ > subversion/libsvn_subr/io.

Re: svn commit: r1004286 - in /subversion/trunk: ./ subversion/libsvn_subr/io.c

On Mon, Oct 11, 2010 at 04:58, Bert Huijben wrote: >> -Original Message- >> From: artag...@apache.org [mailto:artag...@apache.org] >> Sent: maandag 4 oktober 2010 17:27 >> To: comm...@subversion.apache.org >> Subject: svn commit: r1004286 - in /subv

RE: svn commit: r1004286 - in /subversion/trunk: ./ subversion/libsvn_subr/io.c

> -Original Message- > From: artag...@apache.org [mailto:artag...@apache.org] > Sent: maandag 4 oktober 2010 17:27 > To: comm...@subversion.apache.org > Subject: svn commit: r1004286 - in /subversion/trunk: ./ > subversion/libsvn_subr/io.c > > Author: artagnon >