Re: MSI: indentation and style cleanup
Alexandre Julliard wrote: Does your coding style also forbid proper error checking, or is there another reason for removing that check? I thought that crashing on out of memory conditions was a valid method of error handling in the Julliard handbook of coding, so I removed a pointless check :) If we're going to mandate proper checking of memory allocation, then we need to make a janitorial task to do so, as there's many, many places in the code it's not done. Mike
Re: MSI: indentation and style cleanup
Mike McCormack <[EMAIL PROTECTED]> writes:
> -if(szFilePath) {
> +if( szFilePath )
> +{
> len = MultiByteToWideChar( CP_ACP, 0, szFilePath, -1, NULL, 0 );
> szwFilePath = HeapAlloc( GetProcessHeap(), 0, len*sizeof(WCHAR) );
> -if( !szwFilePath)
> -return ERROR_OUTOFMEMORY;
> MultiByteToWideChar( CP_ACP, 0, szFilePath, -1, szwFilePath, len );
> }
Does your coding style also forbid proper error checking, or is there
another reason for removing that check?
--
Alexandre Julliard
[EMAIL PROTECTED]
Re: MSI: indentation and style cleanup
Mike McCormack <[EMAIL PROTECTED]> writes: > I thought that crashing on out of memory conditions was a valid method > of error handling in the Julliard handbook of coding, so I removed a > pointless check :) > > If we're going to mandate proper checking of memory allocation, then > we need to make a janitorial task to do so, as there's many, many > places in the code it's not done. Well yes, ultimately we will need to have proper checks everywhere. It's not really high priority, and I don't think there's a need to make an effort in this direction at this point. But at least we can avoid removing the checks that are already in place... -- Alexandre Julliard [EMAIL PROTECTED]
Re: MSI: indentation and style cleanup
Alexandre Julliard wrote: Mike McCormack <[EMAIL PROTECTED]> writes: I thought that crashing on out of memory conditions was a valid method of error handling in the Julliard handbook of coding, so I removed a pointless check :) If we're going to mandate proper checking of memory allocation, then we need to make a janitorial task to do so, as there's many, many places in the code it's not done. Well yes, ultimately we will need to have proper checks everywhere. It's not really high priority, and I don't think there's a need to make an effort in this direction at this point. But at least we can Well, it should be pretty easy to write a short smatch script to find that occurences. Afair the smatch guys wrote one for the Linux kernel which would need only small adaptations. And coincidently i have this weekend a long flight trip on my schedule ... . And we seem to have some people now doing janitorial work. I figure 2-3 people would maybe jump on it, maybe even new people as the task isn't hard at all. If we get new people it's nice cause they get to see a lot of wine code and get used to the patch acceptance policy ;). Maybe they get motivated to try there luck on other wine tasks too. avoid removing the checks that are already in place... bye michael -- Michael Stefaniuc Tel.: +49-711-96437-199 System Administration Fax.: +49-711-96437-111 Red Hat GmbHEmail: [EMAIL PROTECTED] Hauptstaetterstr. 58http://www.redhat.de/ D-70178 Stuttgart
Re: MSI: indentation and style cleanup
On Fri, 21 Jan 2005 22:02:00 +0100, Michael Stefaniuc wrote: > Well, it should be pretty easy to write a short smatch script to find > that occurences. Afair the smatch guys wrote one for the Linux kernel > which would need only small adaptations. OOM safety is a bit complicated, you have to properly unwind the stack and restore state as you go - for instance the last patch I submitted fixed a bug where OOM would not cause the loop to terminate, but I forgot to free some data as we returned up the stack. Given that it can be quite complex and introduce new bugs, and given that it's really quite a useless feature IMHO as modern Linux boxes will hang themselves in swap hell before returning NULL from malloc I don't think this should be a janitorial project. Just my humble opinion, of course ;) thanks -mike
Re: MSI: indentation and style cleanup
Mike Hearn wrote: On Fri, 21 Jan 2005 22:02:00 +0100, Michael Stefaniuc wrote: Well, it should be pretty easy to write a short smatch script to find that occurences. Afair the smatch guys wrote one for the Linux kernel which would need only small adaptations. OOM safety is a bit complicated, you have to properly unwind the stack and restore state as you go - for instance the last patch I submitted fixed a bug where OOM would not cause the loop to terminate, but I forgot to free some data as we returned up the stack. Given that it can be quite complex and introduce new bugs, and given that it's really quite a useless feature IMHO as modern Linux boxes will hang themselves in swap hell before returning NULL from malloc I don't think this should be a janitorial project. I thought more of checking the return value of HeapAlloc/HeapRealloc to make sure it's not NULL. This would be easy to do. What you proposed is too complicated. bye michael -- Michael Stefaniuc Tel.: +49-711-96437-199 System Administration Fax.: +49-711-96437-111 Red Hat GmbHEmail: [EMAIL PROTECTED] Hauptstaetterstr. 58http://www.redhat.de/ D-70178 Stuttgart
Re: MSI: indentation and style cleanup
On Sun, 23 Jan 2005 10:33:38 +0100, Michael Stefaniuc wrote: > I thought more of checking the return value of HeapAlloc/HeapRealloc to > make sure it's not NULL. This would be easy to do. What you proposed is > too complicated. Well, what happens if it is NULL? You can print an error and then return ... but then the code looks OOM safe even though it's actually not. I guess that's similar to my thoughts on the Interlocked changes before though, it makes code that isn't thread safe look like it is. thanks -mike
Re: MSI: indentation and style cleanup
On Sat, Jan 22, 2005 at 05:16:45PM +, Mike Hearn wrote: > Given that it can be quite complex and introduce new bugs, and given that > it's really quite a useless feature IMHO as modern Linux boxes will hang > themselves in swap hell before returning NULL from malloc I don't think > this should be a janitorial project. I suspect that some of the cases will be simple, some a bit more complicated, a few quite involved. However, it's not that difficult to 'get' what you need to do, so if someone picks up the task, they'll get it within a few patches. ANd they can start with the simple stuff, BTW. At the very list, I think it's worth having the script. And while we're at it, we might as well have the list, for curiosity's sake. :) -- Dimi.
Re: MSI: indentation and style cleanup
Mike Hearn wrote: OOM safety is a bit complicated, you have to properly unwind the stack and restore state as you go - for instance the last patch I submitted fixed a bug where OOM would not cause the loop to terminate, but I forgot to free some data as we returned up the stack. Given that it can be quite complex and introduce new bugs, and given that it's really quite a useless feature IMHO as modern Linux boxes will hang themselves in swap hell before returning NULL from malloc I don't think this should be a janitorial project. You can get NULL with a corrupted heap too. Rob
Re: MSI: indentation and style cleanup
On Sat, Jan 22, 2005 at 05:16:45PM +, Mike Hearn wrote: > Given that it can be quite complex and introduce new bugs, and given that > it's really quite a useless feature IMHO as modern Linux boxes will hang > themselves in swap hell before returning NULL from malloc I don't think > this should be a janitorial project. While I agree this may be not an easy thing to do without looking at the whole code very throughly I think saying that it doesn't help at all because on Linux you will never get this problem is a little shortsighted. Wine doesn't only compile for Linux and there might be other systems with a very straightforward swapping system which will run out of memory before the disk dies from overactivity. Also who says that the next version of Linux or some sort of stress test extension, a security hardened kernel, or a memory quota system on user basis or whatever, will not add an easy to get into out of memory situation. Couldn't Solaris for instance limit the memory available to users on a user base eventhough there are loads and loads of physical memory still available? Rolf Kalbermatter
Re: MSI: indentation and style cleanup
Hi, On Mon, Jan 24, 2005 at 09:02:56AM +0100, Rolf Kalbermatter wrote: > On Sat, Jan 22, 2005 at 05:16:45PM +, Mike Hearn wrote: > > > Given that it can be quite complex and introduce new bugs, and given that > > it's really quite a useless feature IMHO as modern Linux boxes will hang > > themselves in swap hell before returning NULL from malloc I don't think > > this should be a janitorial project. > > While I agree this may be not an easy thing to do without looking at the > whole code very throughly I think saying that it doesn't help at all because > on Linux you will never get this problem is a little shortsighted. Wine > doesn't only compile for Linux and there might be other systems with a very > straightforward swapping system which will run out of memory before the disk > dies from overactivity. Also who says that the next version of Linux or some > sort of stress test extension, a security hardened kernel, or a memory quota > system on user basis or whatever, will not add an easy to get into out of > memory > situation. Couldn't Solaris for instance limit the memory available to users > on a user base eventhough there are loads and loads of physical memory still > available? Why Solaris? A ulimit -S -m 1000 should do the very same thing, no? Hmm, maybe not on Wine, since Wine uses "special" memory management... Andreas Mohr
Re: MSI: indentation and style cleanup
Michael Stefaniuc <[EMAIL PROTECTED]> writes: > Well, it should be pretty easy to write a short smatch script to find > that occurences. Afair the smatch guys wrote one for the Linux kernel > which would need only small adaptations. And coincidently i have this > weekend a long flight trip on my schedule ... . And we seem to have > some people now doing janitorial work. I figure 2-3 people would maybe > jump on it, maybe even new people as the task isn't hard at all. If we > get new people it's nice cause they get to see a lot of wine code and > get used to the patch acceptance policy ;). Maybe they get motivated > to try there luck on other wine tasks too. Yes, but please let's wait until some of the current janitorial tasks are finished before starting new ones. We don't want to have real changes drowned out by a ton of janitorial changes all over the code base. -- Alexandre Julliard [EMAIL PROTECTED]
