Re: [PATCH 4/6]: msvcp110: Add tr2_sys__Stat implementation and test.

[Piotr Caban]

On 06/27/15 19:50, YongHaoHu wrote:
> +static file_type stat_set_error(int *err_code)
> +{
> - file_type ret;
> - *err_code = ERROR_SUCCESS;
> -
> - switch(GetLastError()) {
> -
>    case ERROR_FILE_NOT_FOUND:
> -
>    case ERROR_BAD_NETPATH:
> -
>    case ERROR_INVALID_NAME:
> -
>    case ERROR_BAD_PATHNAME:
> -
>    case ERROR_PATH_NOT_FOUND:
> -
>        ret = file_not_found;
> -
>        break;
> -
>    case ERROR_BAD_DEV_TYPE:
> -
>        ret = regular_file;
> -
>        break;
> -
>    default:
> -
>        ret = status_unknown;

Shouldn't err_code be set to GetLastError in default case? You end up 
setting it to ERROR_SUCCESS in every case except if path is NULL.

Why are you handling ERROR_BAD_DEV_TYPE error here?
> - TRACE("(%s %p)\n", debugstr_a(path), err_code);
> - DWORD attr;

This will produce a compilation warning.

Cheers,
Piotr

[PATCH 4/6]: msvcp110: Add tr2_sys__Stat implementation and test.

[YongHaoHu]

On 15/6/29 下午5:55, Piotr Caban wrote:
> On 06/27/15 19:50, YongHaoHu wrote:
> > +static file_type stat_set_error(int *err_code)
> > +{
> > - file_type ret;
> > - *err_code = ERROR_SUCCESS;
> > -
> > - switch(GetLastError()) {
> > -
> >    case ERROR_FILE_NOT_FOUND:
> > -
> >    case ERROR_BAD_NETPATH:
> > -
> >    case ERROR_INVALID_NAME:
> > -
> >    case ERROR_BAD_PATHNAME:
> > -
> >    case ERROR_PATH_NOT_FOUND:
> > -
> >        ret = file_not_found;
> > -
> >        break;
> > -
> >    case ERROR_BAD_DEV_TYPE:
> > -
> >        ret = regular_file;
> > -
> >        break;
> > -
> >    default:
> > -
> >        ret = status_unknown;
>
> Shouldn't err_code be set to GetLastError in default case? You end up 
> setting it to ERROR_SUCCESS in every case except if path is NULL.

Yeah, you are right.
> Why are you handling ERROR_BAD_DEV_TYPE error here?

I don't have special reason, just base on the test result on testbot.
> > - TRACE("(%s %p)\n", debugstr_a(path), err_code);
> > - DWORD attr;
>
> This will produce a compilation warning.

Got it.

Thank you for your comment. :)

[PATCH 4/6]: msvcp110: Add tr2_sys__Stat implementation and test.

[Piotr Caban]

On 06/29/15 16:42, YongHaoHu wrote:
> > Why are you handling ERROR_BAD_DEV_TYPE error here?
>
> I don't have special reason, just base on the test result on testbot.

It looks wrong. Could you please send the test here and point on what 
testbot machine it happens?

Thanks,
Piotr

[PATCH 4/6]: msvcp110: Add tr2_sys__Stat implementation and test.

[YongHaoHu]

On 15/6/29 下午10:54, Piotr Caban wrote:
> On 06/29/15 16:42, YongHaoHu wrote:
> > > Why are you handling ERROR_BAD_DEV_TYPE error here?
> >
> > I don't have special reason, just base on the test result on testbot.
>
> It looks wrong. Could you please send the test here and point on what 
> testbot machine it happens?

Yes, it's wrong. According to your advice, I tested GetFileAttributesA 
and found that its last error handling was also not correct.
(It means that many ntdll functions have problems with last_error handling.)
The code I used is : GetFileAttributesA("\\.\PiPe\tests_pipe.c");  
printf("getlast err is %d\n", GetLastError());
The output in wine is "getlast err is 66"  while "|getlast err is 0" in 
testbot.

Here is the test result: 
https://testbot.winehq.org/JobDetails.pl?Key=14832&log_101=1#k101 
Plesase ignore the failed msg.
|

[PATCH 4/6]: msvcp110: Add tr2_sys__Stat implementation and test.

[Piotr Caban]

On 06/29/15 18:46, YongHaoHu wrote:
> On 15/6/29 下午10:54, Piotr Caban wrote:
> > On 06/29/15 16:42, YongHaoHu wrote:
> > > > Why are you handling ERROR_BAD_DEV_TYPE error here?
> > >
> > > I don't have special reason, just base on the test result on testbot.
> >
> > It looks wrong. Could you please send the test here and point on what
> > testbot machine it happens?
>
> Yes, it's wrong. According to your advice, I tested GetFileAttributesA
> and found that its last error handling was also not correct.
> (It means that many ntdll functions have problems with last_error handling.)
> The code I used is : GetFileAttributesA("\\.\PiPe\tests_pipe.c");
> printf("getlast err is %d\n", GetLastError());
> The output in wine is "getlast err is 66"  while "|getlast err is 0" in
> testbot.
>
> Here is the test result:
> https://testbot.winehq.org/JobDetails.pl?Key=14832&log_101=1#k101
> Plesase ignore the failed msg.

In this case test should be marked with todo_wine and fail in wine. 
Other option is to fix GetFileAttributes function.