patch review wanted (ticket #9433)

[rndblnch]

hi all,

i've recently encounter a bug, submitted a ticket, was asked to solve
the problem myself in an initial comment :)
i submitted a patch that fix the bug, and could be potentially adapted
to fix another one (see discussion in <http://code.djangoproject.com/
ticket/9433>).
i didn't get feedback for the past 4 weeks and the last django release
reverted my path on my own install :(.

so, could someone have a quick look (it's a very short patch) ?
i know the problem it fix is rather uncommon (it concerns file upload
on a macosx box on a volume mounted through afp), but i'm sure i'm not
the only one facing it :)

thanks for any feedback !

the ticket :
<http://code.djangoproject.com/ticket/9433>

renaud

[Malcolm Tredinnick]

On Thu, 2008-11-20 at 07:10 -0800, rndblnch wrote:
> hi all,
>
> i've recently encounter a bug, submitted a ticket, was asked to solve
> the problem myself in an initial comment :)
> i submitted a patch that fix the bug, and could be potentially adapted
> to fix another one (see discussion in <http://code.djangoproject.com/
> ticket/9433>).
> i didn't get feedback for the past 4 weeks and the last django release
> reverted my path on my own install :(.
>
> so, could someone have a quick look (it's a very short patch) ?
> i know the problem it fix is rather uncommon (it concerns file upload
> on a macosx box on a volume mounted through afp), but i'm sure i'm not
> the only one facing it :)

It has been getting some review -- I've been thinking about it quite a
bit. It's not clear that your proposed solution is a good one, since it
makes file saving unsafe. It might, in fact, be preferable to just say
you cannot save to filesystems that don't support proper file locking
semantics, for example. That's obviously not ideal and I'm trying to
think of a more intermediate approach. Just ignoring locking errors is
very sub-optimal, though, since they can occur for other reasons, and
that's essentially what your patch is doing (for example, the NFS case
that was reported and which you've tried to bundle up as "solved" by the
same problem is almost certainly an actual bug in the NFS client/server
configuration being reported, since NFS does support POSIX locking. So
ignoring the problem there is bad).

Getting this right is important. We have so far had exactly one report
of the problem -- you. So I'm not going to be putting AFP into the list
of "most commonly used filesystems" just yet. Fixing that bug will be
useful. Fixing it properly without inadvertent side-affects that harm
(or disguise) other setups is paramount, however.

Regards,
Malcolm

[rndblnch]

On Nov 21, 12:34 am, Malcolm Tredinnick <malc...@pointy-stick.com>
wrote:

> On Thu, 2008-11-20 at 07:10 -0800, rndblnch wrote:
> > hi all,
>
> > i've recently encounter a bug, submitted a ticket, was asked to solve
> > the problem myself in an initial comment :)
> > i submitted a patch that fix the bug, and could be potentially adapted
> > to fix another one (see discussion in <http://code.djangoproject.com/
> > ticket/9433>).
> > i didn't get feedback for the past 4 weeks and the last django release
> > reverted my path on my own install :(.
>
> > so, could someone have a quick look (it's a very short patch) ?
> > i know the problem it fix is rather uncommon (it concerns file upload
> > on a macosx box on a volume mounted through afp), but i'm sure i'm not
> > the only one facing it :)
>
> It has been getting some review -- I've been thinking about it quite a
> bit.

great, thanks !

> It's not clear that your proposed solution is a good one, since it
> makes file saving unsafe. It might, in fact, be preferable to just say
> you cannot save to filesystems that don't support proper file locking
> semantics, for example. That's obviously not ideal and I'm trying to
> think of a more intermediate approach. Just ignoring locking errors is
> very sub-optimal, though, since they can occur for other reasons, and
> that's essentially what your patch is doing (for example, the NFS case
> that was reported and which you've tried to bundle up as "solved" by the
> same problem is almost certainly an actual bug in the NFS client/server
> configuration being reported, since NFS does support POSIX locking. So
> ignoring the problem there is bad).

i understand.
but in the proposed patch, only the specific error "IOError: [Errno
45] Operation not supported" triggers the fallback to "no
locking" (which is already the fallback for system not supporting
locking).
other errors are not silenced, so the behaviour is left as is for
system supporting locking.

> Getting this right is important. We have so far had exactly one report
> of the problem -- you. So I'm not going to be putting AFP into the list
> of "most commonly used filesystems" just yet. Fixing that bug will be
> useful. Fixing it properly without inadvertent side-affects that harm
> (or disguise) other setups is paramount, however.

ok, i understand.
i will let you work on that and try to find a hack in my application
code so that my fix don't get reverted when i upgrade to a new version
a django.
thanks a lot for your answer and fill free to ask me if you have some
solution to test, since i'm the only one having the test case :)

> Regards,
> Malcolm

regards,
renaud

[rndblnch]

i've just proposed a new approach to solve this bug at the ticket page
<http://code.djangoproject.com/ticket/9433>.
the idea is to just add to django the hooks that make it possible to
fix the bug from the outside.

renaud