[PATCH v6 3/5] ntoskrnl.exe: Implement volume information queries for device files.
Zebediah Figura (she/her)
zfigura at codeweavers.com
Thu Feb 11 12:20:48 CST 2021
On 2/11/21 12:19 PM, Zebediah Figura (she/her) wrote:
> On 2/11/21 12:17 PM, Erich E. Hoover wrote:
>> On Wed, Feb 10, 2021 at 9:33 PM Zebediah Figura (she/her)
>> <zfigura at codeweavers.com> wrote:
>>>
>>> Taking a closer look since the last time I looked at this patch...
>>>
>>>> From: "Erich E. Hoover" <erich.e.hoover at gmail.com>
>>>> Subject: [PATCH v6 3/5] ntoskrnl.exe: Implement volume information queries for device files.
>>>> Message-Id: <CAEU2+vqrdb5u4fTQe80yUHBciFLwoF0RmiXMsFZ_s_wbndm+jQ at mail.gmail.com>
>>>> Date: Sat, 6 Feb 2021 11:25:22 -0700
>>>> ...
>>>> + if (out_size)
>>>> + {
>>>> + if (out_size > context->in_size)
>>>> + {
>>>> + if (!(out_buff = HeapAlloc( GetProcessHeap(), 0, out_size ))) return STATUS_NO_MEMORY;
>>>> + memcpy( out_buff, context->in_buff, context->in_size );
>>>> + to_free = context->in_buff;
>>>> + context->in_buff = out_buff;
>>>> + }
>>>> + else
>>>> + out_buff = context->in_buff;
>>>> + }
>>>> +
>>>
>>> This looks wrong. No data is ever passed in to this IRP, so it should
>>> probably just always allocate an output buffer, like dispatch_read().
>>
>> This was copied from dispatch_ioctl under the assumption that some
>> future usage might need it, but you are right - according to the
>> documentation this IRP has no need for this. I'll go ahead and match
>> the behavior in dispatch_read.
>>
>>> [There's probably an argument to reusing context.in_buff already
>>> allocated in dispatch_read() and here, but we'd need to store the
>>> capacity of the buffer somewhere. This patch will never reuse it, as
>>> in_size should always be 0.]
>>
>> It looks to me like dispatch_read doesn't use context->in_buff, am I
>> missing something here? (Looks to me like it always does a heap
>> allocation.)
>>
>
> Sorry, that was probably confusing phrasing, I mean it was already
> allocated in __wine_ntoskrnl_main_loop(), and we could potentially use
> it in dispatch_read() and here.
>
And to further clarify, I doubt that should be a part of this patch or
this series, if it's even worth doing at all.
>>>> ...
>>>> + FILE_FS_VOLUME_INFORMATION *info = irp->AssociatedIrp.SystemBuffer;
>>>> + static WCHAR label[] = L"WineTestDriver";
>>>
>>> Missing "const"?
>>
>> Yup, sorry about that.
>>
>>>> + ULONG serial = 0xdeadbeef;
>>>> +
>>>> + if (length < sizeof(FILE_FS_VOLUME_INFORMATION))
>>>> + {
>>>> + ret = STATUS_INFO_LENGTH_MISMATCH;
>>>> + break;
>>>> + }
>>>> +
>>>> + info->VolumeCreationTime.QuadPart = 0; /* FIXME */
>>>
>>> "FIXME" seems a bit odd in a test like this, was that a copy/paste error?
>>
>> Yes, that's a copy/paste error from patch 4.
>>
>>>> ...
>
>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: OpenPGP_signature
Type: application/pgp-signature
Size: 495 bytes
Desc: OpenPGP digital signature
URL: <http://www.winehq.org/pipermail/wine-devel/attachments/20210211/4cc74d27/attachment-0001.sig>
More information about the wine-devel
mailing list