March 2016 - Wine-Devel - list.winehq.org

Re: Need some advice on shell32

by Alexandre Julliard

Mathy Vanvoorden <mathy(a)vanvoorden.be> writes: > 2016-03-29 17:49 GMT+02:00 Vincent Povirk <madewokherd(a)gmail.com>: > > I don't know why shell32 does this at all, rather than going > through > kernel32/ntdll like the rest of Wine. > > > My best guess is that shell32 existed before ntdll? > > So should I rewrite it to use ntdll then? > > Anyone else that can comment on this? The goal of the shell Unix folder is to show a view of the filesystem as you would see it in a native Unix app, and it's using direct Unix calls in order to do that. It allows showing symlinks, device files, etc. even when we have to hide them in ntdll to avoid breaking apps. So I'm not sure that replicating ntdll behavior's would be the right thing here. -- Alexandre Julliard julliard(a)winehq.org

March 30, 2016

Re: ntoskrnl.exe: Make IoAllocateIrp not crash on negative values.

by Bernhard Übelacker

Hello Sebastian, Am 28.03.2016 um 08:02 schrieb Sebastian Lackner: > On 27.03.2016 20:27, Bernhard Übelacker wrote: >> https://bugs.winehq.org/show_bug.cgi?id=39734 >> >> Changes should avoid crash in acedrv11.sys. >> IoAllocateIrp is called with a stack_size of -128. >> >> Tested against Windows XP. >> (See the test based on wine-staging "driver testing framework".) > > For reference, here a testbot run which confirms that the change is correct (for XP/2003): > https://newtestbot.winehq.org/JobDetails.pl?Key=21722 > >> >> Signed-off-by: Bernhard Übelacker <bernhardu(a)vr-web.de> >> --- >> dlls/ntoskrnl.exe/ntoskrnl.c | 9 +++++++-- >> 1 file changed, 7 insertions(+), 2 deletions(-) >> >> diff --git a/dlls/ntoskrnl.exe/ntoskrnl.c b/dlls/ntoskrnl.exe/ntoskrnl.c >> index 36488a7..f2ccc61 100644 >> --- a/dlls/ntoskrnl.exe/ntoskrnl.c >> +++ b/dlls/ntoskrnl.exe/ntoskrnl.c >> @@ -592,15 +592,20 @@ PIRP WINAPI IoAllocateIrp( CCHAR stack_size, BOOLEAN charge_quota ) >> { >> SIZE_T size; >> PIRP irp; >> + CCHAR _stack_size = stack_size; >> >> TRACE( "%d, %d\n", stack_size, charge_quota ); >> >> - size = sizeof(IRP) + stack_size * sizeof(IO_STACK_LOCATION); >> + if (_stack_size <= 0 || (_stack_size > 1 && stack_size < 8)) >> + _stack_size = 8; > > I would suggest to use a different variable, its very easy to mix up "stack_size" > and "_stack_size". You even mixed it up yourself in the if() condition above. ;) > If you want, you can also simplify the condition to (stack_size < 8 && stack_size != 1). > >> + >> + size = sizeof(IRP) + _stack_size * sizeof(IO_STACK_LOCATION); >> irp = ExAllocatePool( NonPagedPool, size ); >> if (irp == NULL) >> return NULL; >> IoInitializeIrp( irp, size, stack_size ); >> - irp->AllocationFlags = IRP_ALLOCATED_FIXED_SIZE; >> + if (stack_size >= 1 && stack_size <= 8) >> + irp->AllocationFlags = IRP_ALLOCATED_FIXED_SIZE; >> if (charge_quota) >> irp->AllocationFlags |= IRP_LOOKASIDE_ALLOCATION; >> return irp; >> thanks for your review. I have sent an updated patch. Is the test attached to the bug worth to be included into wine-staging? Kind regards, Bernhard

March 29, 2016

Re: Need some advice on shell32

by Mathy Vanvoorden

2016-03-29 17:49 GMT+02:00 Vincent Povirk <madewokherd(a)gmail.com>: > I don't know why shell32 does this at all, rather than going through > kernel32/ntdll like the rest of Wine. > My best guess is that shell32 existed before ntdll? So should I rewrite it to use ntdll then? Anyone else that can comment on this? Mathy

March 29, 2016

Re: [PATCH] d3drm: Partially Implement IDirect3DRM*::LoadTexture(v3).

by Henri Verbeet

On 29 March 2016 at 18:43, Aaryaman Vasishta <jem456.vasishta(a)gmail.com> wrote: > AddDestroyCallback would need to be implemened as seperate patches for > IDirect3DRMTexture*. Yeah, but it mostly comes down to adding a struct d3drm_object field to struct d3drm_texture, calling d3drm_object_add_destroy_callback() and d3drm_object_delete_destroy_callback() in the appropriate places, and some initialisation and cleanup code. Shouldn't be hard. > So the overall order of implementations/tests patches will be so: > > CreateObject implementation and test > InitFromImage implementation and test > CreateTexture implementation and test > IDirect3DRMObject function implementations for IDirect3DRMTexture*. > InitFromFile implementation and test > LoadTexture implementations > Yeah, that makes sense. The IDirect3DRMObject only need to be before InitFromFile(), so you could send them e.g. before the CreateObject() implementation as well, if you like.

March 29, 2016

Re: [PATCH] d3drm: Partially Implement IDirect3DRM*::LoadTexture(v3).

by Aaryaman Vasishta

AddDestroyCallback would need to be implemened as seperate patches for IDirect3DRMTexture*. Using the BOOL avoids that, but I have a feeling these callbacks might be useful in games as well, so I'll implement these first before LoadTexture patches. So the overall order of implementations/tests patches will be so: CreateObject implementation and test InitFromImage implementation and test CreateTexture implementation and test IDirect3DRMObject function implementations for IDirect3DRMTexture*. InitFromFile implementation and test LoadTexture implementations If the order is not proper, do let me know. Thanks for the review! Cheers, Aaryaman On Tue, Mar 29, 2016 at 8:52 PM, Henri Verbeet <hverbeet(a)gmail.com> wrote: > On 29 March 2016 at 17:01, Aaryaman Vasishta <jem456.vasishta(a)gmail.com> > wrote: > > Are there any existing examples within wine where a destroy callback was > > used in similar cases? > If you mean AddDestroyCallback() specifically, that's pretty specific > to d3drm and you'd be limited to test_Viewport() in the d3drm tests. > The concept of a callback that gets called on destruction is much more > generic of course, and in that case you should be able to find plenty. > E.g. surface_wined3d_object_destroyed() in d3d9. >

March 29, 2016

Re: Need some advice on shell32

by Vincent Povirk

I don't know why shell32 does this at all, rather than going through kernel32/ntdll like the rest of Wine.

March 29, 2016

Re: [PATCH] d3drm: Partially Implement IDirect3DRM*::LoadTexture(v3).

by Henri Verbeet

On 29 March 2016 at 17:01, Aaryaman Vasishta <jem456.vasishta(a)gmail.com> wrote: > Are there any existing examples within wine where a destroy callback was > used in similar cases? If you mean AddDestroyCallback() specifically, that's pretty specific to d3drm and you'd be limited to test_Viewport() in the d3drm tests. The concept of a callback that gets called on destruction is much more generic of course, and in that case you should be able to find plenty. E.g. surface_wined3d_object_destroyed() in d3d9.

March 29, 2016

Re: [PATCH] d3drm: Partially Implement IDirect3DRM*::LoadTexture(v3).

by Aaryaman Vasishta

On Tue, Mar 29, 2016 at 4:59 PM, Henri Verbeet <hverbeet(a)gmail.com> wrote: > On 28 March 2016 at 20:02, Aaryaman Vasishta <jem456.vasishta(a)gmail.com> > wrote: > > v3: Add bitmap verification, use InitFromFile and other minor > improvements. > > > Implementing InitFromFile() should be a separate patch from > implementing LoadTexture(). > > > if (FAILED(hr = d3drm_texture_create(&object))) > > return hr; > > > > + if (FAILED(hr = > IDirect3DRMTexture_InitFromFile(&object->IDirect3DRMTexture_iface, > filename))) > > + { > > + d3drm_texture_destroy(object); > > + return hr; > > + } > > + > > + object->d3drm = iface; > > + IDirect3DRM_AddRef(iface); > It would be good to back this up with tests. It would seem more > natural to pass the IDirect3DRM interface to d3drm_texture_create(). > If that's how it's supposed to work, you can make that change in a > separate patch. > Right. > > > @@ -39,6 +40,9 @@ struct d3drm_texture > > IDirect3DRMTexture IDirect3DRMTexture_iface; > > IDirect3DRMTexture2 IDirect3DRMTexture2_iface; > > IDirect3DRMTexture3 IDirect3DRMTexture3_iface; > > + IDirect3DRM *d3drm; > > + D3DRMIMAGE *image; > > + BOOL internal_image_created; > You don't really need internal_image_created, you can probably just > add a destroy callback. > Are there any existing examples within wine where a destroy callback was used in similar cases? > This is very suspicious, since you always assign "object->d3drm" after > calling InitFromFile(). It's not backed up by tests either. > I think I had put that while debugging a segfault while the tests ran on wine. LoadTexture called InitFromFile first, which then AddRef'd IDirect3DRM. Since IDirect3DRM member wasn't initialized in the object by then, the if check is needed. Also since LoadTexture needs to AddRef only after IDirect3DRM after it's successfully loaded, I put the AddRef below d3drm_texture_load. I agree with all the other changes you mentioned. The leaks and signed local vars are definitely a problem. I'll fix them, divide the patches up and begin work on CreateObject first before submitting these again. Cheers, Aaryaman

March 29, 2016

Re: [v7 2/7] d3dx9: Implement preshader parsing.

by Matteo Bruni

2016-03-29 15:33 GMT+02:00 Paul Gofman <gofmanp(a)gmail.com>: > I removed all the return value checks from creating param evaluator to > let the program continue and not to fail effect creation, exactly as it > did without preshaders before. I have nothing to do with the return > value at the creation right now. At the same time, having such a > function with parsing and a lot of possible errors without return value > would seem very strange. Maybe we could keep the return value in case we > will need this in future? E. g. we may want to decide to fail shaders > creation on preshader parsing failure. Maybe we'll want to change that in the future but for now returning a value which isn't checked from an internal function doesn't seem very useful. I'd just drop the return value from d3dx_create_param_eval(), reintroducing that if and when necessary should be trivial. I'm fine with the other things you mentioned. Hopefully the various additional length checks won't make the code significantly more convoluted (but if you judge they do and resend without them it's okay too).

March 29, 2016

Re: [v7 2/7] d3dx9: Implement preshader parsing.

by Paul Gofman

On 03/29/2016 04:25 PM, Matteo Bruni wrote: > > I'm okay with printing the pointer or similar info but that kind of > trace is pretty ugly. You could trace the pointer when you allocate > the struct, or together with the struct state pointer / the state > parameter in the caller. I guess you can also leave the trace there, > if it says something like: > > TRACE("Created parameter evaluator %p.\n", *peval_out); This is almost exactly what I am suggesting in the next e-mail I've just sent. > BTW, notice that you are never checking the return value from > d3dx_create_param_eval(). You should either do that or remove the > return value from the function altogether. > I removed all the return value checks from creating param evaluator to let the program continue and not to fail effect creation, exactly as it did without preshaders before. I have nothing to do with the return value at the creation right now. At the same time, having such a function with parsing and a lot of possible errors without return value would seem very strange. Maybe we could keep the return value in case we will need this in future? E. g. we may want to decide to fail shaders creation on preshader parsing failure.

March 29, 2016