Ros-dev

ros-dev@reactos.org

1 participants
5790 discussions

Re: [ros-dev] Moving some screensavers to rosapps

by Aleksey Bragin

Hi, "reactos" should have only bare minimum, so I agree about 1 gdi and 1 opengl. Other must be moved to rosapps. WBR, Aleksey Bragin. On Oct 28, 2007, at 2:32 AM, Colin Finck wrote: > Hello everyone, > > Now that we have 5 screensavers in trunk, and they are all in the > "reactos" > directory, it might be a good idea to move some of them to "rosapps". > The big question is now: Which ones shall be kept in "reactos" and > which > ones shall be moved to "rosapps"? > >> From the technical view, it's probably the best idea to keep one >> OpenGL and > one GDI screensaver in "reactos" (for example 3dtext and logon). > Please share your ideas about that. > > Regards, > > Colin

17 years, 10 months

Moving some screensavers to rosapps

by Colin Finck

Hello everyone, Now that we have 5 screensavers in trunk, and they are all in the "reactos" directory, it might be a good idea to move some of them to "rosapps". The big question is now: Which ones shall be kept in "reactos" and which ones shall be moved to "rosapps"? >From the technical view, it's probably the best idea to keep one OpenGL and one GDI screensaver in "reactos" (for example 3dtext and logon). Please share your ideas about that. Regards, Colin

17 years, 10 months

Re: [ros-dev] [ros-diffs] 29826 ...

by tamlin＠algonet.se

Alex Ionescu wrote: >>> 1) Why implement new, complex features in the kernel instead of >>> fixing current bugs? >> >> You pay, you get a saying in what I work on. I pay, I decide. > >No, this is an open source project, everyone gets a say on what you >work on -- you're free to ignore such sayings at your leisure. Seems I was too imprecise (or again a language barrier/misunderstanding). Sorry. My intention was to communicate that if you pay, you get the option of (trying to) tell me what to work on. When I pay, I decide. Obviously anyone is at any time free to comment. >The quota code is called by the process functions, and maybe even by >drivers, but it doesn't affect their behavior. I'm going out on a limb here (as I haven't checked it), but can't a JOB (artificially) limit available resources? That's part of their intention, isn't it? (please read on) >Two choices: > >1) Implement a rather complex quota implementation, including commit >charges in Mm (which is several thousand lines of code in NT), I knew I got into a rather large area, but this sounds insane for something as seemingly "simple". > to a fragile kernel If it's that fragile it needs to be fixed. If you got a better idea to do it than implementing previously missing functionality to expose those fragile parts of it, especially as what I started to work on apparently exposed those fragile parts, I'm all ears. > & > Use up a valuable developer's time, probably a couple man-weeks Should that be how I chose to spend my time - what's the difference if I hadn't been around at all? None, except the end result might make Win32 GlobalMemoryStatus and VM_COUNTERS present useful and usable data. > & > Add new bugs to a hot path during process/object creation/ >deletion (I hope you'll agree all new code has bugs) Only by convention, not by definition. :-) >OR > >2) Continue working on bug fixing the fragile parts of the kernel and >OS || Implement critical missing functionality for a target driver/ >application & This actually started as fixing critical missing functionality (Wax: feel free to chime in). Currently ROS does not COMMIT on such allocation requests. It happily allows an exe in a 128MB machine with 32MB pagefile to COMMIT almost 2GB memory. The error is of course in the way ROS handles COMMIT requests to VirtualAlloc, in that it defers committing the memory until something touches it... (that is no joke) The first step to research this was (for me) to see the behaviour of GlobalMemoryStatus, and indeed it was broken - it was unimplemented... So what do I do? I "dig where I stand". I start an attempt implementing the missing functionality. So the reason I came to touch Ps, and am digging way deeper in Mm than I'd really like to, is simply the GlobalMemoryStatus(Ex) Win32 API function(s), to even be able to verify, from user-mode, behaviour. [...] >> On a sidenote, the only public reference to PS_QUOTA_TYPE I could find >> were from a PDB file transcript from ntdll, posted at a french site. > >It should be added to the NDK. Agreed. But as I didn't know if that's still your baby (SVN access or not) and only synched at intervals, or if it has been "adopted", and if adding such data types, even if suspected to be correct, from such few and vague information source was even acceptable... I played it safe and used just the indices. Thanks for sharing the typename. If concensus is that adding that data type introduces no problems, even if from such a vague source, I'm in favor of adding it (as it'd solve more than one immediate problem). >>> 7) The quota routines require interlocks >> >> Yep. Even requires a lock for checking and updating QuotaPeak. > >No it doesn't, you can use Interlocked Compare Exchange. Are you thinking of the, semantically non-obvious for maintenance programmers, OldVal = Interlocked*Add(&Quota, Addend); InterlockedCompareExchange(&Peak, OldVal + Addend, OldVal + Addend); ? I think using a spinlock for that would make it much more obvious. Should profiling display it had a measurable impact on execution, sure. But we should also keep in mind we are then trading a single spinlock for two *locked* (bus-snooping/cache-invalidating) instructions. I don't know which is worse (performance wise). >>> 8) They also require a global spinlock in the "give back" case. >> >> I don't follow. Why? I'd imagine the "give back" case to be simpler >> than the charge case, as the former only needs to decrement the usage >> (InterlockedExchangeAdd with a negative value), while the latter needs >> to increment, followed by comparing with and possibly update Peak. > >The global spinlock is required when inserting new quota blocks, >dereferencing them, expanding the quota and giving back the quota. Ah, right... the quota blocks. As already displayed I didn't touch them (I honestly don't even know what they are for yet). I only looked at the (immediate) process quota entries. The linked lists I never touched. >I still disagree with trunk-is-a-playground I have never disagreed with that, so I'm not sure what you're referring to. >and I think that pseudo-code should go in branches, but you're free to disagree. I suggest you take that discussion with the project coordinator, that I cleared that diff with and he explicitly stated a preference for it even after I mentioned "branch". I disagree with you re. comments. I think pseudo-code in comments (in trunk even) is to prefer, as it can explain behaviours and intentions way better than plain english or even documentation. If those comments help maintenance-programmers ten or more years from now, even if only one single person, the comments were well spent time and place. -- Mike

17 years, 10 months

Re: [ros-diffs] 29826 ...

by tamlin＠algonet.se

Alex Ionescu wrote: > 1) Why implement new, complex features in the kernel instead of > fixing current bugs? You pay, you get a saying in what I work on. I pay, I decide. > I had already written all these functions, but deleted them, Wasn't that a bonehead thing to do? > because nobody needed them, ... at the time. > and the Mm part was too complex to do. Guess what; I'm currently trying to actually implement it for Mm. Ps was a first step in that direction. Yes, Mm is complex, and yes I don't understand it enough yet - not by a longshot. But checking in incremental improvements, so that the code doesn't get lost, and disabled by default to not affect the behaviour of trunk - that's a good thing. It means people can both review and test the incremental changes, and perhaps even suggest improvements - like you did in pt. 5 below. > 2) Why are you bothering with PspPoolQuotaIndexFromPoolType? Ignorance of: > In case you haven't noticed, there's something called pool masks PAGED_POOL_MASK. Got it. > 5) Instead of duplicating code, perhaps consider calling a main > worker function such as PspChargeQuota? A sound refactoring suggestion. > 6) Consider using an enum instead of magic numbers inside the > quota entries, I believe it's called PS_QUOTA_TYPE. I just followed the lead of existing code. Had there been any mention of an enumeration for nonpaged, paged and pagefile, I would obviously have used those names. On a sidenote, the only public reference to PS_QUOTA_TYPE I could find were from a PDB file transcript from ntdll, posted at a french site. > 7) The quota routines require interlocks Yep. Even requires a lock for checking and updating QuotaPeak. > 8) They also require a global spinlock in the "give back" case. I don't follow. Why? I'd imagine the "give back" case to be simpler than the charge case, as the former only needs to decrement the usage (InterlockedExchangeAdd with a negative value), while the latter needs to increment, followed by comparing with and possibly update Peak. > 9) Committing code that won't even build, even with the define on, > isn't very appropriate. I agree. > If someone wants to actually test this, they won't be able to build it That's an interesting statement. Care to elaborate on how you reached such a factually wrong conclusion? > because of things like "refuse" Some people prefer pseudo-code over nothing at all. Some prefer void. Some people are able to spot C comment blocks. Some are not. Me, I checked that in with both the approval and a statement of preference for this kind of committs by the project coordinator. It not only compiles, it partially works too. > SVN should contain code that builds at any time. It's > not hard to write /* TODO */.. You really need (new?) glasses. You even quoted it yourself! "/* TODO: Check with Process->QuotaBlock if this can be satisfied, */" -- Mike

17 years, 10 months

Re: [ros-diffs] 29702 ...

by tamlin＠algonet.se

Alex Ionescu wrote: > Windows uses the FILE_READ_DATA flag here, not FILE_READ_ATTRIBUTES. Yes? And? Let's study the code and the open modes used here. This code tries to open Device\HarddiskN\Partition0 (N = 0 ...) for the sole purpose of checking for availability, and immediately close the handle if the open succeeded. Why would one use FILE_READ_DATA if one never intended to read anything? The code also uses SYNCHRONIZE. Why would one use SYNCHRONIZE if one never intended to wait on the handle? As I see it, that's just requesting extra work for the purpose of nothing. The purpose of that open call is only to see if the disk exists, and if so create the symbolic link PhysicalDriveN -> HarddiskN\Partition0. I don't buy "Windows does this" without backing it up with any arguments whatsoever as to why Windows does this. Perhaps Windows does more with the returned handle than immediately closing it, something that actually requires READ_DATA? > all this is doing is hiding the bug (which is somewhere in vfat) I'm looking forward to the explanation of how opening HarddiskN\Partition0 (not "Partition1", not "Partition1\", but "Partition0" - aka the whole disk) would involve any filesystem activity whatsoever? > Because it's now gone, it'll probably happen in some mysterious > application 2 years from now and nobody will know why. Hahaha, you really are a joker. An app? Calling xHalIoAssignDriveLetters, that is called with a frickin' LoaderBlock? That function is used exactly once, during system startup. Health reasons suggest not holding your breath while waiting for such an application. :-) -- Mike

17 years, 10 months

Re: [ros-dev] [ros-diffs] [mnordell] 29702: Don't try to open a harddisk for reading when checking for it to create the PhysicalDriveN links. Instead, request FILE_READ_ATTRIBUTES. This silences a hack-warning in IopParseDevice, that now possibly can be removed.

by Alex Ionescu

Windows uses the FILE_READ_DATA flag here, not FILE_READ_ATTRIBUTES. Silencing a warning does not mean fixing it -- all this is doing is hiding the bug (which is somewhere in vfat) in a place that's quite exposed. Because it's now gone, it'll probably happen in some mysterious application 2 years from now and nobody will know why. -- Best regards, Alex Ionescu On 20-Oct-07, at 3:36 AM, mnordell(a)svn.reactos.org wrote: > Author: mnordell > Date: Sat Oct 20 11:36:17 2007 > New Revision: 29702 > > URL: http://svn.reactos.org/svn/reactos?rev=29702&view=rev > Log: > Don't try to open a harddisk for reading when checking for it to > create the PhysicalDriveN links. Instead, request > FILE_READ_ATTRIBUTES. This silences a hack-warning in > IopParseDevice, that now possibly can be removed. > > Modified: > trunk/reactos/ntoskrnl/fstub/disksup.c > trunk/reactos/ntoskrnl/io/iomgr/file.c > > Modified: trunk/reactos/ntoskrnl/fstub/disksup.c > URL: http://svn.reactos.org/svn/reactos/trunk/reactos/ntoskrnl/ > fstub/disksup.c?rev=29702&r1=29701&r2=29702&view=diff > ====================================================================== > ======== > --- trunk/reactos/ntoskrnl/fstub/disksup.c (original) > +++ trunk/reactos/ntoskrnl/fstub/disksup.c Sat Oct 20 11:36:17 2007 > @@ -481,7 +481,7 @@ > NULL); > > Status = ZwOpenFile(&FileHandle, > - FILE_READ_DATA | SYNCHRONIZE, > + FILE_READ_ATTRIBUTES | SYNCHRONIZE, > &ObjectAttributes, > &StatusBlock, > FILE_SHARE_READ, > > Modified: trunk/reactos/ntoskrnl/io/iomgr/file.c > URL: http://svn.reactos.org/svn/reactos/trunk/reactos/ntoskrnl/io/ > iomgr/file.c?rev=29702&r1=29701&r2=29702&view=diff > ====================================================================== > ======== > --- trunk/reactos/ntoskrnl/io/iomgr/file.c (original) > +++ trunk/reactos/ntoskrnl/io/iomgr/file.c Sat Oct 20 11:36:17 2007 > @@ -404,6 +404,9 @@ > > /* FIXME: Small hack still exists, have to check why... > * This is triggered multiple times by usetup and then once > per boot. > + * TMN: NOTE: It might have been fixed now, by changing the > requested > + * openmode in xHalIoAssignDriveLetters from FILE_READ_DATA to > + * FILE_READ_ATTRIBUTES. If verified, this hack should be > removed. > */ > if (!(DirectOpen) && > !(RemainingName->Length) && > >

17 years, 10 months

Re: [ros-dev] [ros-diffs] [mnordell] 29826: First small attempt at implementing process memory quota. Currently disabled without explicit code modification (enabled by macro) to not modify behaviour of trunk.

by Alex Ionescu

Couple of comments: 1) Why implement new, complex features in the kernel instead of fixing current bugs? I know it's fun to start hacking on new code, but priorities should be to implement stuff that's required not to break other apps/add functionality, not internal features which will take ages to implement (the Ps functions for quota are "easy", it's the Mm part that's bad). I had already written all these functions, but deleted them, because nobody needed them, and the Mm part was too complex to do. 2) Why are you bothering with PspPoolQuotaIndexFromPoolType? In case you haven't noticed, there's something called pool masks which make the operation very easy without requiring a function call (such as POOL_TYPE_MASK or BASE_POOL_MASK, forgot how it's called). 3) INT is not an acceptable kernel type. 4) We don't use "static" in the kernel. 5) Instead of duplicating code, perhaps consider calling a main worker function such as PspChargeQuota? 6) Consider using an enum instead of magic numbers inside the quota entries, I believe it's called PS_QUOTA_TYPE. 7) The quota routines require interlocks, otherwise they will not be thread safe. 8) They also require a global spinlock in the "give back" case. 9) Committing code that won't even build, even with the define on, isn't very appropriate. Trunk isn't a playground. If someone wants to actually test this, they won't be able to build it because of things like "refuse". SVN should contain code that builds at any time. It's not hard to write /* TODO */.. Just my 2 cents -- Best regards, Alex Ionescu On 23-Oct-07, at 6:05 AM, mnordell(a)svn.reactos.org wrote: > Author: mnordell > Date: Tue Oct 23 14:05:40 2007 > New Revision: 29826 > > URL: http://svn.reactos.org/svn/reactos?rev=29826&view=rev > Log: > First small attempt at implementing process memory quota. Currently > disabled without explicit code modification (enabled by macro) to > not modify behaviour of trunk. > > Modified: > trunk/reactos/ntoskrnl/ps/quota.c > > Modified: trunk/reactos/ntoskrnl/ps/quota.c > URL: http://svn.reactos.org/svn/reactos/trunk/reactos/ntoskrnl/ps/ > quota.c?rev=29826&r1=29825&r2=29826&view=diff > ====================================================================== > ======== > --- trunk/reactos/ntoskrnl/ps/quota.c (original) > +++ trunk/reactos/ntoskrnl/ps/quota.c Tue Oct 23 14:05:40 2007 > @@ -15,6 +15,12 @@ > > EPROCESS_QUOTA_BLOCK PspDefaultQuotaBlock; > > + > +/* Define this macro to enable quota code testing. Once quota code > is */ > +/* stable and verified, remove this macro and checks for it. */ > +/*#define PS_QUOTA_ENABLE_QUOTA_CODE*/ > + > + > /* FUNCTIONS > ***************************************************************/ > > VOID > @@ -66,9 +72,29 @@ > { > /* Don't do anything for the system process */ > if (Process == PsInitialSystemProcess) return STATUS_SUCCESS; > - > + > +#ifdef PS_QUOTA_ENABLE_QUOTA_CODE > + if (Process) > + { > + /* TODO: Check with Process->QuotaBlock if this can be > satisfied, */ > + /* assuming this indeed is the place to check it. */ > + /* Probably something like: > + if (Process->QuotaUsage[2] + Amount > > + Process->QuotaBlock->QuotaEntry[2].Limit) > + { > + refuse > + } > + */ > + Process->QuotaUsage[2] += Amount; > + if (Process->QuotaPeak[2] < Process->QuotaUsage[2]) > + { > + Process->QuotaPeak[2] = Process->QuotaUsage[2]; > + } > + } > +#else > /* Otherwise, not implemented */ > UNIMPLEMENTED; > +#endif > return STATUS_SUCCESS; > } > > @@ -115,6 +141,38 @@ > return PsChargeProcessPoolQuota(Process, PagedPool, Amount); > } > > +#ifdef PS_QUOTA_ENABLE_QUOTA_CODE > +/* > + * Internal helper function. > + * Returns the index of the Quota* member in EPROCESS for > + * a specified pool type, or -1 on failure. > + */ > +static > +INT > +PspPoolQuotaIndexFromPoolType(POOL_TYPE PoolType) > +{ > + switch (PoolType) > + { > + case NonPagedPool: > + case NonPagedPoolMustSucceed: > + case NonPagedPoolCacheAligned: > + case NonPagedPoolCacheAlignedMustS: > + case NonPagedPoolSession: > + case NonPagedPoolMustSucceedSession: > + case NonPagedPoolCacheAlignedSession: > + case NonPagedPoolCacheAlignedMustSSession: > + return 1; > + case PagedPool: > + case PagedPoolCacheAligned: > + case PagedPoolSession: > + case PagedPoolCacheAlignedSession: > + return 0; > + default: > + return -1; > + } > +} > +#endif > + > /* > * @implemented > */ > @@ -124,10 +182,32 @@ > IN POOL_TYPE PoolType, > IN ULONG Amount) > { > + INT PoolIndex; > /* Don't do anything for the system process */ > if (Process == PsInitialSystemProcess) return STATUS_SUCCESS; > > - UNIMPLEMENTED; > +#ifdef PS_QUOTA_ENABLE_QUOTA_CODE > + PoolIndex = PspPoolQuotaIndexFromPoolType(PoolType); > + if (Process && PoolIndex != -1) > + { > + /* TODO: Check with Process->QuotaBlock if this can be > satisfied, */ > + /* assuming this indeed is the place to check it. */ > + /* Probably something like: > + if (Process->QuotaUsage[PoolIndex] + Amount > > + Process->QuotaBlock->QuotaEntry[PoolIndex].Limit) > + { > + refuse > + } > + */ > + Process->QuotaUsage[PoolIndex] += Amount; > + if (Process->QuotaPeak[PoolIndex] < Process->QuotaUsage > [PoolIndex]) > + { > + Process->QuotaPeak[PoolIndex] = Process->QuotaUsage > [PoolIndex]; > + } > + } > +#else > + UNIMPLEMENTED; > +#endif > return STATUS_SUCCESS; > } > > @@ -140,10 +220,26 @@ > IN POOL_TYPE PoolType, > IN ULONG_PTR Amount) > { > + INT PoolIndex; > /* Don't do anything for the system process */ > if (Process == PsInitialSystemProcess) return; > > - UNIMPLEMENTED; > +#ifdef PS_QUOTA_ENABLE_QUOTA_CODE > + PoolIndex = PspPoolQuotaIndexFromPoolType(PoolType); > + if (Process && PoolIndex != -1) > + { > + if (Process->QuotaUsage[PoolIndex] < Amount) > + { > + DPRINT1("WARNING: Process->QuotaUsage sanity check > failed.\n"); > + } > + else > + { > + Process->QuotaUsage[PoolIndex] -= Amount; > + } > + } > +#else > + UNIMPLEMENTED; > +#endif > } > > /* > @@ -157,7 +253,11 @@ > /* Don't do anything for the system process */ > if (Process == PsInitialSystemProcess) return; > > - UNIMPLEMENTED; > +#ifdef PS_QUOTA_ENABLE_QUOTA_CODE > + PsReturnPoolQuota(Process, NonPagedPool, Amount); > +#else > + UNIMPLEMENTED; > +#endif > } > > /* > @@ -171,7 +271,11 @@ > /* Don't do anything for the system process */ > if (Process == PsInitialSystemProcess) return; > > - UNIMPLEMENTED; > +#ifdef PS_QUOTA_ENABLE_QUOTA_CODE > + PsReturnPoolQuota(Process, PagedPool, Amount); > +#else > + UNIMPLEMENTED; > +#endif > } > > NTSTATUS > @@ -182,8 +286,22 @@ > /* Don't do anything for the system process */ > if (Process == PsInitialSystemProcess) return STATUS_SUCCESS; > > +#ifdef PS_QUOTA_ENABLE_QUOTA_CODE > + if (Process) > + { > + if (Process->QuotaUsage[2] < Amount) > + { > + DPRINT1("WARNING: Process PageFileQuotaUsage sanity > check failed.\n"); > + } > + else > + { > + Process->QuotaUsage[2] -= Amount; > + } > + } > +#else > /* Otherwise, not implemented */ > UNIMPLEMENTED; > +#endif > return STATUS_SUCCESS; > } > > >

17 years, 10 months

[ros-diffs] [greatlrd] 29788: rename dc.h to win32kdc.h rename driver.h to win32kdriver.h

by James Tabor

Hi! Good job! But this is all kinds of type of wrong! There are internal calls for win32k dc.c and dcutils.c in there. What you should have done was move the DC object structure to ntgdihdl.h. So, please revert or but back dc.h and then move the DC object to ntgdihld.h where it belongs. Thanks, James

17 years, 10 months

[RE:] ReactOS 0.3.4 Hardware Support

by James Tabor

Hi! What was the revision for the floppy driver that made it inoperative? You told me on IRC. I did not take my notes to work. > Hello, > > I think that 0.3.4 should have most hardware regressions (floppy and > vga) fixed. I know I'm not the one to say this but, we are already > closer to this goal than we were with 0.3.3. Fixing the floppy > regression should only involve finding the spot in which floppy is not > reporting correctly to ntoskrnl. I did some tests on this issue and it > fails on line 596-598 of ntoskrnl\io\iomgr\volume.c and floppy was never > mounted as the status reported was STATUS_IO_DEVICE_ERROR from line > 1017-1021 in drivers\storage\floppy\floppy.c. I don't know much about > the vga regression but I'd think that it may only require a few days > work to find and fix the issue. We already have SATA support included > with trunk (uniata.sys) and SCSI is also working now. > > -aicommander > Thanks, James

17 years, 10 months

Re: [ros-dev] [ros-diffs] [fireball] 29714: Dmitry Chapyshev <lentind@yandex.ru> - Fully implement tapiui.dll, compatible with Windows. - Icons come from SUSE's yast2-industrial; 203,202.ico and 301,302.bmp are done by me based on Tango.

by Ged

These resources have been lifted straight out of the Windows dll. Absolutley no attempt has been made to change anything. IMO, this should be removed until someone manually implements it. Ged. fireball(a)svn.reactos.org wrote: > Author: fireball > Date: Sat Oct 20 21:16:30 2007 > New Revision: 29714 > > URL: http://svn.reactos.org/svn/reactos?rev=29714&view=rev > Log: > Dmitry Chapyshev <lentind(a)yandex.ru> > - Fully implement tapiui.dll, compatible with Windows. > - Icons come from SUSE's yast2-industrial; 203,202.ico and 301,302.bmp are done by me based on Tango > > >

17 years, 10 months

Jump to page:

2025

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

2012

2011

2010

2009

2008

2007

2006

2005

2004

Ros-dev