[RFC PATCH 2/9] setupapi: Implement SetupDiSetDevicePropertyW.

Zhiyi Zhang zzhang at codeweavers.com
Sun Jan 27 01:58:57 CST 2019


On 23/01/2019 23:47, Zebediah Figura wrote:
> On 1/23/19 1:39 AM, Zhiyi Zhang wrote:
>>
>> On 23/01/2019 10:17, Zebediah Figura wrote:
>>> On 12/18/18 10:20 AM, Zhiyi Zhang wrote:
>>>> Signed-off-by: Zhiyi Zhang <zzhang at codeweavers.com>
>>>> ---
>>>>    dlls/setupapi/devinst.c       | 139 ++++++++++++++++++++++++
>>>>    dlls/setupapi/setupapi.spec   |   2 +-
>>>>    dlls/setupapi/tests/devinst.c | 194 +++++++++++++++++++++++++++++++++-
>>>>    include/setupapi.h            |   2 +
>>>>    4 files changed, 335 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/dlls/setupapi/devinst.c b/dlls/setupapi/devinst.c
>>>> index 55746562c8..556bba4684 100644
>>>> --- a/dlls/setupapi/devinst.c
>>>> +++ b/dlls/setupapi/devinst.c
>>>> @@ -327,6 +327,28 @@ static WCHAR *get_refstr_key_path(struct
>>>> device_iface *iface)
>>>>        return path;
>>>>    }
>>>>    +static BOOL is_valid_property_type(DEVPROPTYPE prop_type)
>>>> +{
>>>> +    DWORD type = prop_type & DEVPROP_MASK_TYPE;
>>>> +    DWORD typemod = prop_type & DEVPROP_MASK_TYPEMOD;
>>>> +
>>>> +    if (type > MAX_DEVPROP_TYPE)
>>>> +        return FALSE;
>>>> +    if (typemod > MAX_DEVPROP_TYPEMOD)
>>>> +        return FALSE;
>>>> +
>>>> +    if (typemod == DEVPROP_TYPEMOD_ARRAY
>>>> +        && (type == DEVPROP_TYPE_EMPTY || type == DEVPROP_TYPE_NULL
>>>> || type == DEVPROP_TYPE_STRING
>>>> +            || type == DEVPROP_TYPE_SECURITY_DESCRIPTOR_STRING))
>>>> +        return FALSE;
>>>> +
>>>> +    if (typemod == DEVPROP_TYPEMOD_LIST
>>>> +        && !(type == DEVPROP_TYPE_STRING || type ==
>>>> DEVPROP_TYPE_SECURITY_DESCRIPTOR_STRING))
>>>> +        return FALSE;
>>>> +
>>>> +    return TRUE;
>>>> +}
>>>> +
>>>
>>> This level of extensive error checking is probably not a bad idea,
>>> but—the current state of most of SetupDi notwithstanding—I'm not sure
>>> it's a good one either (and it's certainly a little troublesome to
>>> review). I won't stand opposed to it if pressed, but is it really
>>> necessary?
>>
>> I would rather do the extensive error/parameter checking now than fixing
>> the crash caused by not checking later. I feel like there are already so
>> many bugs because of not enough checking.
>>
>> As long as they are covered by tests. They shouldn't be too much an issue.
>>
>
> I'm not sure I agree, honestly. I don't see many applications that break on invalid parameters (after all, why would they?), and those that do are generally rather easy to debug.

Well, there are many patches that fix crashes just by adding simple parameter checks because somehow there are invalid parameters.

And yes, they are usually easy to debug, I prefer not having to debug these kind of things at all.


>
> If you'd really prefer to leave these in, I'd at least advocate for removing the tests (and perhaps corresponding checks) that don't fail consistently across versions.
>
>
I'll remove some of the inconsistent tests.



More information about the wine-devel mailing list