Re: [1/3] scrrun: Implement filesys_DriveExists.

[Nikolay Sivov]

On 15.10.2015 13:56, Joachim Priesner wrote:
> - if (!pfExists) return E_POINTER;
> - *pfExists = VARIANT_FALSE;
> - len = SysStringLen(DriveSpec);
> - if (len >= 1 && DriveSpec[0] >= 'A' && DriveSpec[0] <= 'Z'
> -
>        && (len < 2 || DriveSpec[1] == ':')
> -
>        && (len < 3 || DriveSpec[2] == '\\')) {
> -
>    WCHAR root[] = {toupperW(DriveSpec[0]), ':', '\\', 0};
> -
>    *pfExists = GetDriveTypeW(root) != DRIVE_NO_ROOT_DIR ? VARIANT_TRUE : VARIANT_FALSE;
> - }

Not necessary wrong, but it'd be better if all that validation was 
GetDriveTypeW responsibility. I don't see many tests for it in 
kernel32/tests, could you please add some, so we can get rid of this 
extra logic? I haven't looked much at its implementation but it feels 
like DRIVE_UNKNOWN would also qualify as non-existent drive.

[1/3] scrrun: Implement filesys_DriveExists.

[Joachim Priesner]

Thanks for having a look at the code, will fix the method wrapper issue and resend later.
> Not necessary wrong, but it'd be better if all that validation was
> GetDriveTypeW responsibility. I don't see many tests for it in
> kernel32/tests, could you please add some, so we can get rid of this
> extra logic?

GetDriveTypeW does not accept strings consisting of only a single drive letter, but filesys_DriveExists does. So we cannot rely on GetDriveTypeW's validation and have to do our own.

I'll send the GetDriveType tests in a separate patch, good to have them anyway :)
> I haven't looked much at its implementation but it feels 
> like DRIVE_UNKNOWN would also qualify as non-existent drive.

The Windows implementation of DriveExists returns true for DRIVE_UNKNOWN. (I tested using a native scrrun.dll by patching GetDriveTypeW to always return DRIVE_UNKNOWN.)
> > -
> >    DriveSpec[0] = toupperW(DriveSpec[0]);
> > -
> >    if (DriveSpec[0] < 'A' || DriveSpec[0] > 'Z'
> > -
> >            || (len >= 2 && DriveSpec[1] != ':')
> > -
> >            || (len == 3 && DriveSpec[2] != '\\'))
> > -
> >        return E_INVALIDARG;
> > -
> >    hr = filesys_DriveExists(iface, DriveSpec, &drive_exists);
>
> I'm not sure about that sanity check, the reason is that DriveExists
> doesn't fail on invalid drivespec?

Correct, we cannot distinguish between a nonexisting drive and a malformed parameter just by looking at hr.

Joachim

[1/3] scrrun: Implement filesys_DriveExists.

[Nikolay Sivov]

On 15.10.2015 20:39, Joachim Priesner wrote:
> Thanks for having a look at the code, will fix the method wrapper issue and resend later.
> > Not necessary wrong, but it'd be better if all that validation was
> > GetDriveTypeW responsibility. I don't see many tests for it in
> > kernel32/tests, could you please add some, so we can get rid of this
> > extra logic?
>
> GetDriveTypeW does not accept strings consisting of only a single drive letter, but filesys_DriveExists does. So we cannot rely on GetDriveTypeW's validation and have to do our own.
>
> I'll send the GetDriveType tests in a separate patch, good to have them anyway :)

Thanks.
> > I haven't looked much at its implementation but it feels
> > like DRIVE_UNKNOWN would also qualify as non-existent drive.
>
> The Windows implementation of DriveExists returns true for DRIVE_UNKNOWN. (I tested using a native scrrun.dll by patching GetDriveTypeW to always return DRIVE_UNKNOWN.)

It was a bad idea to test it in such way I think, it's essentially the 
same as +relay-ing native dlls - we don't want to look into 
implementation details, and the fact that DriveExists uses GetDriveTypeW 
is such implementation detail. If it's possible to make GetDriveTypeW 
fail with DRIVE_UNKNOWN we should just add such tests for it and for 
DriveExists. If it's not testable for some reason I think it's better to 
differ and consider DRIVE_UNKNOWN as non-existent drive.