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

[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 <vicmarcal(a)hotmail.com>

>    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<http://msdn…)
>  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…
>
>  _______________________________________________
>  Ros-dev mailing list
>  Ros-dev(a)reactos.org
>  http://www.reactos.org/mailman/listinfo/ros-dev
>