[haiku-development] Re: dosfs fixes
- From: Jan Klötzke <jan.kloetzke@xxxxxxxxxx>
- To: haiku-development@xxxxxxxxxxxxx
- Date: Thu, 24 May 2007 09:43:58 +0200
Am Wednesday 23 May 2007 10:07 schrieb Axel Dörfler:
> Hi Jan,
>
[...]
> Thanks for the patch!
> I've applied it now, but I have some comments:
> * Your patch caused several warnings when compiled - you should always
> make sure that all warnings are resolved before submitting a patch.
Sure. I will check better next time.
> * while we inherited dos fs from Be, and it doesn't exactly follow our
> coding style, new code and changes should always do so - I've corrected
> your patch in this regard, and cleaned up dos fs a bit while I was at
> it
Maybe I should have a look at the coding style rules. :-)
>
> * and:
> > * Block cache transaction id returned by cache_start_transaction() my
> > wrap to
> > negative numbers.
> >
> > Looking at the function it seems that a negative return value should
> > indicate
> > a error code. Normal transaction id's should therefore be always
> > positive.
> > Furthermore it seems not every caller checks if this function failed.
>
> I've not applied this patch - while it solves the problem of
> transaction IDs getting negative, there are further problems that would
> have to be resolved when the IDs overlap; I think it should be either
> handled correctly everywhere (in that component) or not at all.
> The latter is even acceptable for now (and several kernel components do
> not handle this case gracefully); since Haiku is not a server OS, it
> would probably never get a chance to overflow these IDs anyway (bugs
> aside).
> So while patches that addresses these issues are always welcome, IMO
> it's acceptable for R1 to keep suffering from them.
I found this more by accident while browsing through the code. I agree that
this patch would make problems "more unlikely" but doesn't really solve the
issue. In the end there should be some kind of "id allocator". BTW, how is
the allocation of e.g. file handles done? I think this is the same kind of
problem.
>
> But anyway, thanks a lot!
NP
/Jan
Other related posts:
