Re: ntoskrnl: Implement server side of get_device

[Vitaliy Margolen]

Peter Dons Tychsen wrote:
> Implement server side of get_device to complete the
> IoGetDeviceObjectPointer() function.
>
> /pedro

My first question: what will you do with that pointer if that driver is
loaded in the separate instance of ntoskrnl? Driver that calls
IoGetDeviceObjectPointer will most likely try to dereference the pointer it
got back.
> +/* locate a device */
> +DECL_HANDLER(get_device)
> +{
> - struct unicode_str name;
> - get_req_unicode_str( &name );
> -
> - /* lookup the device */
> - struct device *device = open_object_dir(NULL, &name, 0, &device_ops);
> - if(!device)
> - {
> -
>    return;
> - }
> -
> - /* fill reply */
> - reply->handle = alloc_handle(current->process, device, req->access, 0);....
> - reply->user_ptr = device->user_ptr;
> -

You leaking reference to device here. Need release_object( device );

In your patch please include changes to server/protocol.def only and not any
other automatically generated files. Make it a separate commit in your tree
and just don't send it to wine-patches.

Vitaliy

ntoskrnl: Implement server side of get_device

[Peter Dons Tychsen]

Hi Vitaliy.

Thanks for your comments,
> My first question: what will you do with that pointer if that driver is
> loaded in the separate instance of ntoskrnl? Driver that calls
> IoGetDeviceObjectPointer will most likely try to dereference the pointer it
> got back.

You are right. Something is not right here. I will investigate it
further. Maybe i need to take a copy of the handle which is suitable for
the context.
> You leaking reference to device here. Need release_object( device );

Roger.
> In your patch please include changes to server/protocol.def only and not any
> other automatically generated files. Make it a separate commit in your tree
> and just don't send it to wine-patches.

I totally agree. I don't even think these files should be in git *at
all*. Auto-generated files do not belong in the revision control system.
In this case they should be generated by the build system. I also
noticed that there are no dependencies for these files, so they are not
re-generated when protocol.def changes. This should be fixed.

But Alexandre does it the same way i did it. That is why i did it.
Look at this:
http://source.winehq.org/git/wine.git/?a=commitdiff;h=05b4181cac1f567f786a0c...

/pedro

ntoskrnl: Implement server side of get_device

[Vitaliy Margolen]

Peter Dons Tychsen wrote:
> Hi Vitaliy.
>
> Thanks for your comments,
> > My first question: what will you do with that pointer if that driver is
> > loaded in the separate instance of ntoskrnl? Driver that calls
> > IoGetDeviceObjectPointer will most likely try to dereference the pointer it
> > got back.
>
> You are right. Something is not right here. I will investigate it
> further. Maybe i need to take a copy of the handle which is suitable for
> the context.

Well I'm not sure what good will it be? The caller asks for the pointer to
the DEVICE_OBJECT to do something with that structure. You can't just give
it some handle. It needs to be complete structure. And not just any
structure but the one for that device.

But that's where it all starts. When another driver will ask to add itself
to the chain of this device what will happen? Wine needs to modify this
object, so that means passing it through the server back? What about
actually calling this chain?
> But Alexandre does it the same way i did it. That is why i did it.
> Look at this:
> http://source.winehq.org/git/wine.git/?a=commitdiff;h=05b4181cac1f567f786a0c...

Alexandre is a special case :) He is the maintainer and keeps the tree in
the proper state. When you submit patches, they go through his hands. So you
need to do what's required for Alexandre to process your patch without
making his work any more difficult.

In short - just don't include automatically generated pieces in your patches.

Vitaliy

ntoskrnl: Implement server side of get_device

[Peter Dons Tychsen]

Hi V.
> Well I'm not sure what good will it be? The caller asks for the pointer to
> the DEVICE_OBJECT to do something with that structure. You can't just give
> it some handle. It needs to be complete structure. And not just any
> structure but the one for that device.

Sorry. I did not mean the handle, but the device pointer.

I have done some more research on this function. The implementation i
made was totally off track!

IoGetDeviceObjectPointer is allot like calling NtOpenFile(), except it
does not return the handle. Instead it return a file-pointer, which is
of type FILE_OBJECT. A list of these would probably have to be kept for
each process, with some kind of map to the handle. We will also need it
when implementing ObReferenceObjectByHandle().

Apart from that i am not totally sure how to get the process-dependent
device pointer. Maybe it should be copied... needs more investigation.
> Alexandre is a special case :) He is the maintainer and keeps the tree in
> the proper state. When you submit patches, they go through his hands. So you
> need to do what's required for Alexandre to process your patch without
> making his work any more difficult.
>
> In short - just don't include automatically generated pieces in your patches.

Roger.

I guess my patch was a bit premature... not to mention totally nuts!

/pedro