[ros-diff]rev 46907.gschneider[SHELL32] - Simplify checks for success of ShellExecuteEx

[victor martinez]

Hi, i have been giving a look to this patch and i have some doubts(just learning :) )
The code is:
@@ -1298,8 +1298,8 @@ static HRESULT WINAPI ICPanel_IContextMenu2_InvokeCommand(
        sei.hwnd = lpcmi->hwnd;
        sei.nShow = SW_SHOWNORMAL;
        sei.lpVerb = L"open";
-       ShellExecuteExW(&sei);
-       if (sei.hInstApp <= (HINSTANCE)32)
+
+       if (ShellExecuteExW(&sei) == FALSE)
           return E_FAIL;

MSDN says that if ShellExecuteExW fails it sets hInstApp to a value lower than 32 and,also, returns FALSE.
(MSDN: http://msdn.microsoft.com/en-us/library/bb762154(VS.85).aspx ) 
So both lines of codes(buggy and patched) are,at first sight,doing the same.Just using a different way.
The Commit says: 
- Simplify checks for success of ShellExecuteEx, field hInst may be an unreliable indicator according to 
http://msdn.microsoft.com/en-us/library/bb759784%28v=VS.85%29.aspx

Why is it unreliable? :) I am not saying it is reliable, just that i dont find the reason there...my skills are limited.. :(
I have just found: "Although hInstApp is declared as an HINSTANCE for compatibility with 16-bit Windows applications, it is not a true HINSTANCE. It can be cast only to an int and compared to either 32 or the following SE_ERR_XXX error codes ".

So my question is: Is the Bug in the cast to (HINSTANCE) instead to (INT)?I mean,do this solve the bug too?:
-       if (sei.hInstApp <= (HINSTANCE)32)
+       if ((INT)sei.hInstApp <= 32)

As you see i am just learning, so thanks in advance.
And yes, i prefer the new way that patch is using.Using the returned the value seems much more logic than using a "collateral damage". :)
Btw, thanks Gregor for your hunting-fixing week ;)

_________________________________________________________________
¡Citas! ¡Ligues! ¿Salimos? ¿Cómo es tu pareja ideal? Búscala en el sitio nº1… ¡Regístrate ya!
http://contactos.es.msn.com/?mtcmk=015352

[Gregor Schneider]

Hi Victor,

you are perfectly right here, the situation is complicated. In my commit msg
I provided a link, which specifies the following behaviour:
*
*

hInstApp HINSTANCE
[out] If SEE_MASK_NOCLOSEPROCESS is set and the ShellExecuteEx call
succeeds, it sets this member to a value greater than 32. If the function
fails, it is set to an SE_ERR_XXX error value that indicates the cause of
the failure.

That made me think: what happens to this field if the ShellExecuteEx call
succeeds and the flag SEE_MASK_NOCLOSEPROCESS is not set? Since this check
was thought to forward the actual result of the execution i switched to a
simpler and (in my eyes) more reliable code. Plus the original code did not
use the flag noted in the specification.
The link you provided on the other hand makes it look like ShellExecuteEx
always sets this field indendent of any flags passed to the function.

To find out the truth one would have to write test cases. It might well be
that the implementation we use is partially incorrect. Especially when
looking at testman (it works again :-O ) @ shell32_winetest shlexec which
still has some failures.

Greg

2010/4/19 victor martinez [e-mail]
> Hi, i have been giving a look to this patch and i have some doubts(just
> learning :) )
> The code is:
> @@ -1298,8 +1298,8 @@ static HRESULT WINAPI
> ICPanel_IContextMenu2_InvokeCommand(
>         sei.hwnd = lpcmi->hwnd;
>         sei.nShow = SW_SHOWNORMAL;
>         sei.lpVerb = L"open";
>   ShellExecuteExW(&sei);
>   if (sei.hInstApp <= (HINSTANCE)32)
>   if (ShellExecuteExW(&sei) == FALSE)
>      return E_FAIL;
>
>
> MSDN says that if ShellExecuteExW fails it sets hInstApp to a value lower
> than 32 and,also, returns FALSE.
> (MSDN: http://msdn.microsoft.com/en-us/library/bb762154(VS.85).aspxhttp://msdn.microsoft.com/en-us/library/bb762154%28VS.85%29.aspx)
> So both lines of codes(buggy and patched) are,at first sight,doing the
> same.Just using a different way.
> The Commit says:
> Simplify checks for success of ShellExecuteEx, field hInst may be an
>
> unreliable indicator according to
> http://msdn.microsoft.com/en-us/library/bb759784%28v=VS.85%29.aspx
>
> Why is it unreliable? :) I am not saying it is reliable, just that i dont
> find the reason there...my skills are limited.. :(
> I have just found: "Although hInstApp is declared as an HINSTANCE for
> compatibility with 16-bit Windows applications, it is not a true HINSTANCE.
> It can be cast only to an int and compared to either 32 or the following
> SE_ERR_XXX error codes ".
>
> So my question is: Is the Bug in the cast to (HINSTANCE) instead to (INT)?I
> mean,do this solve the bug too?:
>   if (sei.hInstApp <= (HINSTANCE)32)
>   if ((INT)sei.hInstApp <= 32)
>
> As you see i am just learning, so thanks in advance.
> And yes, i prefer the new way that patch is using.Using the returned the
> value seems much more logic than using a "collateral damage". :)
> Btw, thanks Gregor for your hunting-fixing week ;)
>
> Tus datos personales, más seguros con Internet Explorer 8. ¡Descárgatelo
> gratis!http://www.microsoft.com/spain/windows/internet-explorer/default.aspx
>
> Ros-dev mailing list
> [e-mail]
> http://www.reactos.org/mailman/listinfo/ros-dev

[victor martinez]

Thanks for your answer :)
I was just worried because maybe there is a bug in our API when saving the value in hInstApp HINSTANCE(as you also pointed in your answer) when the function fails.If so,then any check against one of the SE_ERR_XXX error codes will fail as it checks against the hInstApp value.

I will try to give a look to the function code and its Winetest related.Your commit have motivated me...Thanks ;)
    	 	   		  
_________________________________________________________________
Aprende los trucos de Windows 7 con la gente que ya lo han probado Windows 7.
http://www.sietesunpueblodeexpertos.com/index_windows7.html