Re: [ros-dev] win32k C++ suggestion

2015-02-21 Thread Aleksey Bragin 
I always liked C++, and it was my main development language before I 
joined ReactOS. Indeed let's not rush, think/discuss through all 
possible things and then start gradually going for it for code pieces 
which are good (which means GDI first of all).


Regards,
Aleksey

On 21.02.2015 17:05, Timo Kreuzer wrote:

Hi

I'd like to propose introducing C++ to win32k. Don't worry, this is 
not a suggestion to rewrite everything from scratch in C++, but to 
gradually introduce C++.
The reason is not "That's what Windows does", but the fact that 
especially GDI would heavily benefit in terms of code simplicity, 
clarily and quality from using C++.
A lot of the interfaces are already in a C++ object style way, but 
still code can bypass interfaces and directly modify structure 
members, even if it is not supposed to be done that way.
Unless there are strong objections, I'd post a design / style / 
roadmap suggestion soon.
If people feel strongly about it, we can defer this to after a 0.4.0 
release.


Regards,
Timo



___
Ros-dev mailing list
Ros-dev@reactos.org
http://www.reactos.org/mailman/listinfo/ros-dev

Re: [ros-dev] [ros-diffs] [jgardou] 66334: [NTOSKRNL/MM] - MiIsEntireRangeCommitted: Ensure the PTE we are checking is really faulted in. - Prefer MiPteToPde and MiPdeToPte (which should really be cal

2015-02-21 Thread Timo Kreuzer 


I was thinking that a VAD with the Committed flag set would be entirely 
committed, so we could avoid all PTE checking. But I guess the flag 
stays set, even if we decommit ranges inside it. In that case it's clear 
that we need to check for decommitted pages.


Regarding your change, I see the issue now (I was misinterpreting it before)
Your fix looks correct to me.

Timo

Am 21.02.2015 um 16:41 schrieb Jerome Gardou:
It is possible that the entire VAD range is not committed, so we have 
to sweep over all the PTEs in the sub-range to check. If one of the 
PTEs is zero AND the entire VAD is not marked as committed, then we're 
sure that the range is not committed. Also, if the PTE is marked as 
decommitted, same thing happen.


So, to check the PTEs, we have to make sure the underlying PDE is 
there (== not paged out). Otherwise, by accessing a PTE, the CPU might 
raise a page fault while we're holding the process working set lock 
--> failed ASSERT.
Notice that if the PDE is zero, it's not faulted in and we check for 
the VAD commit flag. (like we do for the PTEs later in the loop iteration)


If I misunderstood something, I'd be happy to be given some hint as 
why this patch is incorrect.


Regards
Jérôme

Le 20/02/2015 22:00, Timo Kreuzer a écrit :


Why does the PDE need to be faulted in? The function is called 
MiIsEntireRangeCommitted, not 
MiCheckIfEntireRangeIsCommittedAndMakePdeValid.

I also wonder why one would even walk the PTEs if the VAD is MEM_COMMIT.



Am 19.02.2015 um 11:19 schrieb Jérôme Gardou:

If it takes me 6 months to understand why, then yes.

Hint: you can make this delay shorter.

Also the title is a bit misleading, it's the PDE that is not faulted in
in case we don't terminate the loop iteration early.

Le 19/02/2015 05:46, Alex Ionescu a écrit :
it might fix an assert but the patch is incorrect. will this also 
take 6

months to revert?

Best regards,
Alex Ionescu

On Tue, Feb 17, 2015 at 6:19 AM,  wrote:


Author: jgardou
Date: Tue Feb 17 14:19:05 2015
New Revision: 66334

URL: http://svn.reactos.org/svn/reactos?rev=66334&view=rev
Log:
[NTOSKRNL/MM]
  - MiIsEntireRangeCommitted: Ensure the PTE we are checking is 
really

faulted in.
  - Prefer MiPteToPde and MiPdeToPte (which should really be called
MiFirstPteInPde) instead of MiAddressToPte and MiPteToAddress
Fixes weird failed ASSERT in page fault handler when using DPH.

Modified:
 trunk/reactos/ntoskrnl/mm/ARM3/virtual.c

Modified: trunk/reactos/ntoskrnl/mm/ARM3/virtual.c
URL:
http://svn.reactos.org/svn/reactos/trunk/reactos/ntoskrnl/mm/ARM3/virtual.c?rev=66334&r1=66333&r2=66334&view=diff 



== 


--- trunk/reactos/ntoskrnl/mm/ARM3/virtual.c [iso-8859-1] (original)
+++ trunk/reactos/ntoskrnl/mm/ARM3/virtual.c [iso-8859-1] Tue Feb 17
14:19:05 2015
@@ -1994,14 +1994,13 @@
  if (OnBoundary)
  {
  /* Is this PDE demand zero? */
-PointerPde = MiAddressToPte(PointerPte);
+PointerPde = MiPteToPde(PointerPte);
  if (PointerPde->u.Long != 0)
  {
  /* It isn't -- is it valid? */
  if (PointerPde->u.Hard.Valid == 0)
  {
  /* Nope, fault it in */
-PointerPte = MiPteToAddress(PointerPde);
  MiMakeSystemAddressValid(PointerPte, Process);
  }
  }
@@ -2009,13 +2008,13 @@
  {
  /* The PTE was already valid, so move to the 
next one */

  PointerPde++;
-PointerPte = MiPteToAddress(PointerPde);
+PointerPte = MiPdeToPte(PointerPde);

  /* Is the entire VAD committed? If not, fail */
  if (!Vad->u.VadFlags.MemCommit) return FALSE;

-/* Everything is committed so far past the range, 
return

true */
-if (PointerPte > LastPte) return TRUE;
+/* New loop iteration with our new, on-boundary 
PTE. */

+continue;
  }
  }







___
Ros-dev mailing list
Ros-dev@reactos.org
http://www.reactos.org/mailman/listinfo/ros-dev


___
Ros-dev mailing list
Ros-dev@reactos.org
http://www.reactos.org/mailman/listinfo/ros-dev






___
Ros-dev mailing list
Ros-dev@reactos.org
http://www.reactos.org/mailman/listinfo/ros-dev




___
Ros-dev mailing list
Ros-dev@reactos.org
http://www.reactos.org/mailman/listinfo/ros-dev




smime.p7s
Description: S/MIME Cryptographic Signature
___
Ros-dev mailing list
Ros-dev@reactos.org
http://www.reactos.org/mailman/listinfo/ros-dev

Re: [ros-dev] [ros-diffs] [jgardou] 66334: [NTOSKRNL/MM] - MiIsEntireRangeCommitted: Ensure the PTE we are checking is really faulted in. - Prefer MiPteToPde and MiPdeToPte (which should really be cal

2015-02-21 Thread Jerome Gardou 
It is possible that the entire VAD range is not committed, so we have to 
sweep over all the PTEs in the sub-range to check. If one of the PTEs is 
zero AND the entire VAD is not marked as committed, then we're sure that 
the range is not committed. Also, if the PTE is marked as decommitted, 
same thing happen.


So, to check the PTEs, we have to make sure the underlying PDE is there 
(== not paged out). Otherwise, by accessing a PTE, the CPU might raise a 
page fault while we're holding the process working set lock --> failed 
ASSERT.
Notice that if the PDE is zero, it's not faulted in and we check for the 
VAD commit flag. (like we do for the PTEs later in the loop iteration)


If I misunderstood something, I'd be happy to be given some hint as why 
this patch is incorrect.


Regards
Jérôme

Le 20/02/2015 22:00, Timo Kreuzer a écrit :


Why does the PDE need to be faulted in? The function is called 
MiIsEntireRangeCommitted, not 
MiCheckIfEntireRangeIsCommittedAndMakePdeValid.

I also wonder why one would even walk the PTEs if the VAD is MEM_COMMIT.



Am 19.02.2015 um 11:19 schrieb Jérôme Gardou:

If it takes me 6 months to understand why, then yes.

Hint: you can make this delay shorter.

Also the title is a bit misleading, it's the PDE that is not faulted in
in case we don't terminate the loop iteration early.

Le 19/02/2015 05:46, Alex Ionescu a écrit :
it might fix an assert but the patch is incorrect. will this also 
take 6

months to revert?

Best regards,
Alex Ionescu

On Tue, Feb 17, 2015 at 6:19 AM,  wrote:


Author: jgardou
Date: Tue Feb 17 14:19:05 2015
New Revision: 66334

URL: http://svn.reactos.org/svn/reactos?rev=66334&view=rev
Log:
[NTOSKRNL/MM]
  - MiIsEntireRangeCommitted: Ensure the PTE we are checking is really
faulted in.
  - Prefer MiPteToPde and MiPdeToPte (which should really be called
MiFirstPteInPde) instead of MiAddressToPte and MiPteToAddress
Fixes weird failed ASSERT in page fault handler when using DPH.

Modified:
 trunk/reactos/ntoskrnl/mm/ARM3/virtual.c

Modified: trunk/reactos/ntoskrnl/mm/ARM3/virtual.c
URL:
http://svn.reactos.org/svn/reactos/trunk/reactos/ntoskrnl/mm/ARM3/virtual.c?rev=66334&r1=66333&r2=66334&view=diff 



== 

--- trunk/reactos/ntoskrnl/mm/ARM3/virtual.c[iso-8859-1] 
(original)
+++ trunk/reactos/ntoskrnl/mm/ARM3/virtual.c[iso-8859-1] Tue 
Feb 17

14:19:05 2015
@@ -1994,14 +1994,13 @@
  if (OnBoundary)
  {
  /* Is this PDE demand zero? */
-PointerPde = MiAddressToPte(PointerPte);
+PointerPde = MiPteToPde(PointerPte);
  if (PointerPde->u.Long != 0)
  {
  /* It isn't -- is it valid? */
  if (PointerPde->u.Hard.Valid == 0)
  {
  /* Nope, fault it in */
-PointerPte = MiPteToAddress(PointerPde);
  MiMakeSystemAddressValid(PointerPte, Process);
  }
  }
@@ -2009,13 +2008,13 @@
  {
  /* The PTE was already valid, so move to the next 
one */

  PointerPde++;
-PointerPte = MiPteToAddress(PointerPde);
+PointerPte = MiPdeToPte(PointerPde);

  /* Is the entire VAD committed? If not, fail */
  if (!Vad->u.VadFlags.MemCommit) return FALSE;

-/* Everything is committed so far past the range, 
return

true */
-if (PointerPte > LastPte) return TRUE;
+/* New loop iteration with our new, on-boundary 
PTE. */

+continue;
  }
  }







___
Ros-dev mailing list
Ros-dev@reactos.org
http://www.reactos.org/mailman/listinfo/ros-dev


___
Ros-dev mailing list
Ros-dev@reactos.org
http://www.reactos.org/mailman/listinfo/ros-dev






___
Ros-dev mailing list
Ros-dev@reactos.org
http://www.reactos.org/mailman/listinfo/ros-dev


___
Ros-dev mailing list
Ros-dev@reactos.org
http://www.reactos.org/mailman/listinfo/ros-dev

Re: [ros-dev] [ros-diffs] [tfaber] 66365: [KERNEL32] - Make BaseSetLastNTError return the converted Win32 error code. This will determine the upper 24 bits of EAX in functions that return BOOLEAN FALS

2015-02-21 Thread Thomas Faber 
I agree and I'm not super happy with this. The two reasons I did it
this hackish way anyway are
* Changing the return types of these functions in our implementation is
  pretty painful and confusing, it'll create differences between
  headers and implementation and/or require redirects in the spec file
  etc.
* I did not want to go through the trouble of locating all such
  functions (maybe it's only the one, who knows), and am not
  particularly keen on having to keep in mind this strategy as new
  functions get added.

Basically, I just wanted to make the app work instead of developing a
well-designed scheme to properly handle this situation.
In fact it seems pretty hard to design one... looks like you'd have to
stop using BOOLEAN return completely and use BOOL instead, but then
there could be applications with the opposite bug where they _only_
work because they interpret the return value incorrectly.
I'd simply prefer not having to debug a second issue of this kind any
time soon because we missed something or our design was flawed. However
if you see a good solution that I'm missing, I'm be happy to go with it.


On 2015-02-21 00:17, Timo Kreuzer wrote:
> 
> For me this looks like a hack. It might "do what Windows does", but the 
> result is more based on luck / random compiler behaviour rather than 
> deterministic behavior. We should think about returning a full ULONG in 
> all functions that rely on this (like Wow64EnableWow64FsRedirection), 
> containing the correct value, rather than relying on random stuff.
> 
> 
> Am 20.02.2015 um 08:03 schrieb tfa...@svn.reactos.org:
>> Author: tfaber
>> Date: Fri Feb 20 07:03:00 2015
>> New Revision: 66365
>>
>> URL: http://svn.reactos.org/svn/reactos?rev=66365&view=rev
>> Log:
>> [KERNEL32]
>> - Make BaseSetLastNTError return the converted Win32 error code. This will 
>> determine the upper 24 bits of EAX in functions that return BOOLEAN FALSE 
>> right after calling BaseSetLastNTError, e.g. Wow64EnableWow64FsRedirection. 
>> Fixes installers using WiX Toolset (e.g. VS2012 redist) on MSVC builds.
>> See http://wixtoolset.org/issues/4681/ for the WiX bug that causes this.
>> CORE-8010
>>
>> Modified:
>>  trunk/reactos/dll/win32/kernel32/client/except.c
>>  trunk/reactos/dll/win32/kernel32/include/kernel32.h
>>
>> Modified: trunk/reactos/dll/win32/kernel32/client/except.c
>> URL: 
>> http://svn.reactos.org/svn/reactos/trunk/reactos/dll/win32/kernel32/client/except.c?rev=66365&r1=66364&r2=66365&view=diff
>> ==
>> --- trunk/reactos/dll/win32/kernel32/client/except.c [iso-8859-1] (original)
>> +++ trunk/reactos/dll/win32/kernel32/client/except.c [iso-8859-1] Fri Feb 20 
>> 07:03:00 2015
>> @@ -682,12 +682,16 @@
>>   /*
>>* @implemented
>>*/
>> -VOID
>> +DWORD
>>   WINAPI
>>   BaseSetLastNTError(IN NTSTATUS Status)
>>   {
>> +DWORD dwErrCode;
>> +
>>   /* Convert from NT to Win32, then set */
>> -SetLastError(RtlNtStatusToDosError(Status));
>> +dwErrCode = RtlNtStatusToDosError(Status);
>> +SetLastError(dwErrCode);
>> +return dwErrCode;
>>   }
>>   
>>   /*
>>
>> Modified: trunk/reactos/dll/win32/kernel32/include/kernel32.h
>> URL: 
>> http://svn.reactos.org/svn/reactos/trunk/reactos/dll/win32/kernel32/include/kernel32.h?rev=66365&r1=66364&r2=66365&view=diff
>> ==
>> --- trunk/reactos/dll/win32/kernel32/include/kernel32.h  [iso-8859-1] 
>> (original)
>> +++ trunk/reactos/dll/win32/kernel32/include/kernel32.h  [iso-8859-1] 
>> Fri Feb 20 07:03:00 2015
>> @@ -353,7 +353,7 @@
>>   WINAPI
>>   InitCommandLines(VOID);
>>   
>> -VOID
>> +DWORD
>>   WINAPI
>>   BaseSetLastNTError(IN NTSTATUS Status);
>>   
>>
>>
>>


___
Ros-dev mailing list
Ros-dev@reactos.org
http://www.reactos.org/mailman/listinfo/ros-dev

Re: [ros-dev] win32k C++ suggestion

2015-02-21 Thread Jerome Gardou 

Hi Timo,

I'd say go for it. I have no strong objection to introduce some changes 
before 0.4.0, as long as it's not too intrusive or add some real added 
value (unimplemented modules/parts).


I'd like to have it for more than for simplification sake. Some parts of 
win32k are in a sorry state (nothing new, sadly), but 
improving/implementing some modules with C++ would be a nice proof of 
concept. Font engine comes to mind. (Hint, hint ;-)) However, if you 
feel this is too much of an effort for a beginning, feel free to do 
whatever you see fit.


Regards,
Jérôme

PS: Admit it, you want to revive the yarotows branch.


Le 21/02/2015 15:05, Timo Kreuzer a écrit :

Hi

I'd like to propose introducing C++ to win32k. Don't worry, this is 
not a suggestion to rewrite everything from scratch in C++, but to 
gradually introduce C++.
The reason is not "That's what Windows does", but the fact that 
especially GDI would heavily benefit in terms of code simplicity, 
clarily and quality from using C++.
A lot of the interfaces are already in a C++ object style way, but 
still code can bypass interfaces and directly modify structure 
members, even if it is not supposed to be done that way.
Unless there are strong objections, I'd post a design / style / 
roadmap suggestion soon.
If people feel strongly about it, we can defer this to after a 0.4.0 
release.


Regards,
Timo




___
Ros-dev mailing list
Ros-dev@reactos.org
http://www.reactos.org/mailman/listinfo/ros-dev


___
Ros-dev mailing list
Ros-dev@reactos.org
http://www.reactos.org/mailman/listinfo/ros-dev

[ros-dev] win32k C++ suggestion

2015-02-21 Thread Timo Kreuzer 

Hi

I'd like to propose introducing C++ to win32k. Don't worry, this is not 
a suggestion to rewrite everything from scratch in C++, but to gradually 
introduce C++.
The reason is not "That's what Windows does", but the fact that 
especially GDI would heavily benefit in terms of code simplicity, 
clarily and quality from using C++.
A lot of the interfaces are already in a C++ object style way, but still 
code can bypass interfaces and directly modify structure members, even 
if it is not supposed to be done that way.
Unless there are strong objections, I'd post a design / style / roadmap 
suggestion soon.
If people feel strongly about it, we can defer this to after a 0.4.0 
release.


Regards,
Timo




smime.p7s
Description: S/MIME Cryptographic Signature
___
Ros-dev mailing list
Ros-dev@reactos.org
http://www.reactos.org/mailman/listinfo/ros-dev

Re: [ros-dev] [ros-diffs] [gadamopoulos] 66383: [SHELL32] - Implement progress dialogs for SHFileOperation - Patch by Hwu Davies CORE-4476

2015-02-21 Thread Pierre Schweitzer 
In such situations, please update all the langages files and not only
English. This makes work easier for translators to track changes.

On 21/02/2015 13:52, gadamopou...@svn.reactos.org wrote:
> Author: gadamopoulos
> Date: Sat Feb 21 12:52:58 2015
> New Revision: 66383
> 
> URL: http://svn.reactos.org/svn/reactos?rev=66383&view=rev
> Log:
> [SHELL32]
> - Implement progress dialogs for SHFileOperation
> - Patch by Hwu Davies
> CORE-4476
> 
> Modified:
> trunk/reactos/dll/win32/shell32/lang/en-US.rc
> 
> Modified: trunk/reactos/dll/win32/shell32/lang/en-US.rc
> URL: 
> http://svn.reactos.org/svn/reactos/trunk/reactos/dll/win32/shell32/lang/en-US.rc?rev=66383&r1=66382&r2=66383&view=diff
> ==
> --- trunk/reactos/dll/win32/shell32/lang/en-US.rc [iso-8859-1] (original)
> +++ trunk/reactos/dll/win32/shell32/lang/en-US.rc [iso-8859-1] Sat Feb 21 
> 12:52:58 2015
> @@ -693,6 +693,13 @@
>  IDS_OVERWRITEFILE_CAPTION "Confirm file overwrite"
>  IDS_OVERWRITEFOLDER_TEXT "This folder already contains a folder named 
> '%1'.\n\nIf the files in the destination folder have the same names as files 
> in the\nselected folder they will be replaced. Do you still want to move or 
> copy\nthe folder?"
>  
> +IDS_FILEOOP_COPYING   "Copying"
> +IDS_FILEOOP_MOVING"Moving"
> +IDS_FILEOOP_DELETING  "Deleting"
> +IDS_FILEOOP_FROM_TO   "From %1 to %2"
> +IDS_FILEOOP_FROM  "From %1"
> +IDS_FILEOOP_PREFLIGHT "Preflight"
> +
>  /* message box strings */
>  IDS_RESTART_TITLE "Restart"
>  IDS_RESTART_PROMPT "Do you want to restart the system?"
> 


-- 
Pierre Schweitzer 
System & Network Administrator
Senior Kernel Developer
ReactOS Deutschland e.V.



smime.p7s
Description: S/MIME Cryptographic Signature
___
Ros-dev mailing list
Ros-dev@reactos.org
http://www.reactos.org/mailman/listinfo/ros-dev