Patch to fix bug 34324

[Zhenbo Li]

Hello,
I've been working on http://bugs.winehq.org/show_bug.cgi?id=34324.
Till now, I have found two solutions, one is simple and one is
complex. Both of them would fix #34324, and I don't know which one is
better.

As Alexandre Julliard mentioned[1], I would use the value of
GetLastError() as the return (error) value in delete_files().

The simple patch would fix part of the 'todo_wine' marks.
Meanwhile, the complex patch would fix *all* todo marks about return
value in test_delete().

I really appreciate your help (especially for Dmitry Timoshkov, thank
you very much). And I'm looking for your feedbacks.

Thank you.

[1]: http://markmail.org/message/fblet7gadeusyauv#query:+page:1+mid:6eu2a72uuchhm...

[Zhenbo Li]

2014-02-19 17:47 GMT+08:00 Zhenbo Li :
> Hello,
> I've been working on http://bugs.winehq.org/show_bug.cgi?id=34324.
> Till now, I have found two solutions, one is simple and one is
> complex. Both of them would fix #34324, and I don't know which one is
> better.
>
> As Alexandre Julliard mentioned[1], I would use the value of
> GetLastError() as the return (error) value in delete_files().
>
> The simple patch would fix part of the 'todo_wine' marks.
> Meanwhile, the complex patch would fix *all* todo marks about return
> value in test_delete().
>
> I really appreciate your help (especially for Dmitry Timoshkov, thank
> you very much). And I'm looking for your feedbacks.
>
> Thank you.

Sorry to bother you.
I don't know which patch is preferred, could someone give me any suggestions?

Also, I'm sorry for the format problems. I'll re-check them before sending them
to wine-patches.

Any suggestions are greatly appreciated.
Thank you.

[Bruno Jesus]

On Sat, Feb 22, 2014 at 12:19 AM, Zhenbo Li  wrote:
> Sorry to bother you.
> I don't know which patch is preferred, could someone give me any suggestions?
>
> Also, I'm sorry for the format problems. I'll re-check them before sending them
> to wine-patches.
>
> Any suggestions are greatly appreciated.
> Thank you.
> --
> Have a nice day!
> Zhenbo Li

In my opinion the complex patch is better.

+ if (hFind == INVALID_HANDLE_VALUE){
+      if (GetLastError() == ERROR_PATH_NOT_FOUND)
+       SetLastError(0x7c);  /* DE_INVALIDFILES */
+    return FALSE;
+    }
+    FindClose(hFind);

You are not following the {} styles in the surrounding code.
Why using a magic value (0x7c) if there is a define(DE_INVALIDFILES)?

Best wishes,
Bruno

[Zhenbo Li]

Thank you for checking it.

2014-02-24 23:39 GMT+08:00 Bruno Jesus :
> In my opinion the complex patch is better.
> - if (hFind == INVALID_HANDLE_VALUE){
> -
>  if (GetLastError() == ERROR_PATH_NOT_FOUND)
> -
>   SetLastError(0x7c);  /* DE_INVALIDFILES */
> - return FALSE;
> - }
> - FindClose(hFind);
>
> You are not following the {} styles in the surrounding code.

Sorry, it's my fault. I was confused by old code style.
I've sent a new patch[1] to bugzilla[2], you may check that.
> Why using a magic value (0x7c) if there is a define(DE_INVALIDFILES)?

This is related to legacy. MSDN said[3],
"These are pre-Win32 error codes and are no longer supported or
defined in any public header file."
And DE_INVALIDFILES is not defined in wine's shell32 code.
Maybe to define it is better than to use a magic number? I'm not sure.

BTW, in irc channel, Stefand suggested me to avoid SetLastError().
If I have to do that, could I change SHELL_DeleteDirectoryW()
from BOOL to DWORD?

Thank you very much.

[1]: http://bugs.winehq.org/attachment.cgi?id=47626&action=diff
[2]: http://bugs.winehq.org/show_bug.cgi?id=34324
[3]: http://msdn.microsoft.com/en-us/library/windows/desktop/bb762164%28v=vs.85%2...

[Bruno Jesus]

On Tue, Feb 25, 2014 at 1:49 AM, Zhenbo Li  wrote:
> > Why using a magic value (0x7c) if there is a define(DE_INVALIDFILES)?
>
> This is related to legacy. MSDN said[3],
> "These are pre-Win32 error codes and are no longer supported or
> defined in any public header file."
> And DE_INVALIDFILES is not defined in wine's shell32 code.
> Maybe to define it is better than to use a magic number? I'm not sure.

I see, I guess it's ok then. Never saw that situation before so I
can't really tell what is best.
> BTW, in irc channel, Stefand suggested me to avoid SetLastError().
> If I have to do that, could I change SHELL_DeleteDirectoryW()
> from BOOL to DWORD?

If BOOL is no longer enough and the function is internal I see no
problem in changing it, but there are other calls to this function so
you would have to review and update then accordingly. And yes,
avoiding the SetLastError is better as Windows may not set it too and
the application may not be expecting that.

Best wishes,
Bruno

[Zhenbo Li]

2014-02-25 20:33 GMT+08:00 Bruno Jesus :
> > BTW, in irc channel, Stefand suggested me to avoid SetLastError().
> > If I have to do that, could I change SHELL_DeleteDirectoryW()
> > from BOOL to DWORD?
>
> If BOOL is no longer enough and the function is internal I see no
> problem in changing it, but there are other calls to this function so
> you would have to review and update then accordingly. And yes,
> avoiding the SetLastError is better as Windows may not set it too and
> the application may not be expecting that.

Thank you for your suggest.
I checked wine's code again. As SHELL_DeleteDirectoryW() is a
recursive function, using DWORD instead of BOOL would cause a big change.

So I modified the end of delete_files().

if (!bPathExists)
-            return ERROR_PATH_NOT_FOUND;
+            return GetLastError() == ERROR_PATH_NOT_FOUND?
+                0x7c:               /* DE_INVALIDFILES , which is a
legacy in Windows*/
+                GetLastError();

In this new patch[1], it would fix *all* todo marks
about return value in test_delete(), too.

I appreciate your feedbacks.
(Thank you very much, Bruno Jesus)

[1]: http://bugs.winehq.org/attachment.cgi?id=47644&action=diff