August 2014 - Wine-Devel - list.winehq.org

Re: [PATCH 2/5] dxgi: Free the wined3d object when device creation fails (Valgrind).

by Henri Verbeet

On 27 August 2014 18:40, Matteo Bruni <mbruni(a)codeweavers.com> wrote: > --- > dlls/dxgi/device.c | 8 +++++++- > 1 file changed, 7 insertions(+), 1 deletion(-) > > diff --git a/dlls/dxgi/device.c b/dlls/dxgi/device.c > index 837c57f..0f3ae7d 100644 > --- a/dlls/dxgi/device.c > +++ b/dlls/dxgi/device.c > @@ -350,7 +350,7 @@ HRESULT dxgi_device_init(struct dxgi_device *device, struct dxgi_device_layer *l > IWineDXGIDeviceParent *dxgi_device_parent; > IWineDXGIAdapter *wine_adapter; > UINT adapter_ordinal; > - struct wined3d *wined3d; > + struct wined3d *wined3d = NULL; > void *layer_base; > HRESULT hr; > WINED3DCAPS caps; > @@ -430,6 +430,12 @@ fail: > wined3d_device_decref(device->wined3d_device); > LeaveCriticalSection(&dxgi_cs); > } > + else if (wined3d) > + { > + EnterCriticalSection(&dxgi_cs); > + wined3d_decref(wined3d); > + LeaveCriticalSection(&dxgi_cs); > + } I don't think this works as intended. For example, if IDXGIAdapter_QueryInterface() fails, you'd call wined3d_decref() twice. There's also a wined3d_decref() after wined3d_device_create(), although it's indeed missing for IWineDXGIDevice_QueryInterface() and wined3d_get_device_caps().

Aug. 27, 2014

Re: wpcap: Load libpcap dynamically

by Scott Ritchie

On Tue, Aug 26, 2014 at 1:55 PM, Scott Ritchie <scottritchie(a)ubuntu.com> wrote: > On Debian/Ubuntu, libpcap0.8-dev will need to be added to package > build dependencies or a user will never be able to turn this feature > on. This will cause Wine to statically link a dll, and for debhelper > to then detect that static link, figure out it comes from the > libpcap0.8 package, and add that dependency to the {$shlibs:Depends} > substitution variable. This requires modifying the package rules file > to override dh_shlibdeps to either 1) exclude calculating the > particular DLL in question, or 2) manually remove libpcap0.8 > afterwards. > > Both those are fairly straightforward in deb-land. But, so is > applying Andre's patch to a distro package. I don't know about > tooling for other distros. As an update for the Ubuntu packages, I have opted for modifying the package rules rather than patching wine. As a general rule I like to minimize the number of patches to upstream in my packages. I would still recommend applying Andre's patch upstream as it provides some consistency with the way the rest of Wine operates, but it's no big deal.

Aug. 27, 2014

Re: wpcap: Load libpcap dynamically

by Scott Ritchie

On Tue, Aug 26, 2014 at 4:36 AM, Alexandre Julliard <julliard(a)winehq.org> wrote: > Apart from making the code more complicated, the end result is the same, > either way the dependency is required for the library to work, so I > don't see why it would have to be treated differently when packaging. > If the packaging tools insist on making it a requirement just because > they see a dll linked to it, I'd suggest fixing the tools. > So, just to be clear here about the tradeoffs of the two solutions "fix the packaging tooling" or "make it dynamic in wine": On Debian/Ubuntu, libpcap0.8-dev will need to be added to package build dependencies or a user will never be able to turn this feature on. This will cause Wine to statically link a dll, and for debhelper to then detect that static link, figure out it comes from the libpcap0.8 package, and add that dependency to the {$shlibs:Depends} substitution variable. This requires modifying the package rules file to override dh_shlibdeps to either 1) exclude calculating the particular DLL in question, or 2) manually remove libpcap0.8 afterwards. Both those are fairly straightforward in deb-land. But, so is applying Andre's patch to a distro package. I don't know about tooling for other distros.

Aug. 26, 2014

Re: wpcap: Load libpcap dynamically

by André Hentschel

Am 26.08.2014 um 11:55 schrieb Alexandre Julliard: > André Hentschel <nerv(a)dawncrow.de> writes: > >> This is intended for packagers, without this patch libpcap easily becomes a fixed dependency of wine. >> Something we don't want i guess. > > It's only used for wpcap, which can't work without it anyway, so I don't > see any need to make it dynamic. > Hi, well, my package manager doesn't force me to install libcapi when installing wine, that's good, i don't need ISDN support, if i'd try using it in wine, i'd quickly find out what i need to make it work with wine... wpcap could be handled the same way with the patch... Up to you, just my two cents

Aug. 26, 2014

Re: [PATCH 5/5] d3dx9: Correctly handle out of memory conditions while reallocating children array.

by Matteo Bruni

2014-08-26 16:58 GMT+02:00 Matteo Bruni <matteo.mystral(a)gmail.com>: > 2014-08-26 16:30 GMT+02:00 Henri Verbeet <hverbeet(a)gmail.com>: >> On 26 August 2014 16:13, Matteo Bruni <mbruni(a)codeweavers.com> wrote: >>> @@ -338,15 +338,20 @@ static HRESULT d3dx9_file_data_create(IDirectXFileObject *dxfile_object, ID3DXFi >>> >>> while (SUCCEEDED(ret = IDirectXFileData_GetNextObject(object->dxfile_data, &data_object))) >> ... >>> + new_children = HeapReAlloc(GetProcessHeap(), 0, object->children, >>> + sizeof(*object->children) * (object->nb_children + 1)); >> While I suspect this is hardly the only thing wrong with xfile.c, it >> seems suboptimal to grow the array by 1 each time you go through the >> loop. > > Yeah, it should start at some size greater than 1 (no idea about > typical children count here though) and then grow exponentially. I > guess that can be a separate patch, but I can also add it to this one > and resend if preferred. Actually, merging the two results in a bit less churn. So, this patch replaces the original one. I manually generated this patch via "git diff" (and kept the individual commits locally just in case), if this doesn't work right you can either commit the original one as-is or just leave it out.

Aug. 26, 2014

Re: [PATCH 5/5] d3dx9: Correctly handle out of memory conditions while reallocating children array.

by Matteo Bruni

2014-08-26 16:30 GMT+02:00 Henri Verbeet <hverbeet(a)gmail.com>: > On 26 August 2014 16:13, Matteo Bruni <mbruni(a)codeweavers.com> wrote: >> @@ -338,15 +338,20 @@ static HRESULT d3dx9_file_data_create(IDirectXFileObject *dxfile_object, ID3DXFi >> >> while (SUCCEEDED(ret = IDirectXFileData_GetNextObject(object->dxfile_data, &data_object))) > ... >> + new_children = HeapReAlloc(GetProcessHeap(), 0, object->children, >> + sizeof(*object->children) * (object->nb_children + 1)); > While I suspect this is hardly the only thing wrong with xfile.c, it > seems suboptimal to grow the array by 1 each time you go through the > loop. Yeah, it should start at some size greater than 1 (no idea about typical children count here though) and then grow exponentially. I guess that can be a separate patch, but I can also add it to this one and resend if preferred.

Aug. 26, 2014

Re: [PATCH 5/5] d3dx9: Correctly handle out of memory conditions while reallocating children array.

by Henri Verbeet

On 26 August 2014 16:13, Matteo Bruni <mbruni(a)codeweavers.com> wrote: > @@ -338,15 +338,20 @@ static HRESULT d3dx9_file_data_create(IDirectXFileObject *dxfile_object, ID3DXFi > > while (SUCCEEDED(ret = IDirectXFileData_GetNextObject(object->dxfile_data, &data_object))) ... > + new_children = HeapReAlloc(GetProcessHeap(), 0, object->children, > + sizeof(*object->children) * (object->nb_children + 1)); While I suspect this is hardly the only thing wrong with xfile.c, it seems suboptimal to grow the array by 1 each time you go through the loop.

Aug. 26, 2014

Re: wpcap: Load libpcap dynamically

by Sebastian Lackner

Am 26.08.2014 um 13:22 schrieb Michael Stefaniuc: > On 08/26/2014 12:47 PM, Sebastian Lackner wrote: >> Linking it dynamically has a great advantage for distributions and >> package maintainers providing precompiled wine builds. I would also >> like to provide my wine builds with wpcap support (some users might >> want to use it) - but when I do that a fixed dependency is >> required. This is especially confusing when wine is only used as a >> dependency, like in case of pipelight, where users are already >> wondering why we now depend on libpcap (those are wine builds where >> it was already enabled): >> https://answers.launchpad.net/pipelight/+question/253591 >> >> I guess other package maintainers might probably think the same, >> but most haven't updated to 1.7.25 yet, and are not aware of this >> problem. Just converting the static dependency to a dynamic >> dependency would be the easiest approach to solve this - the >> alternative would be to split wine into (more) packages, and avoid >> the libpcap dependency on the main one... or just leave this >> feature exclusively for source-based distros where users have >> fine-grained control about which features they want. :/ > Fedora has subpackages for all those components requiring extra > dependencies. In this case I guess they will go for a wine-pcap rpm. > Installing the wine package will pull everything in. But the people > that do care about the dependencies they have the granularity required. @Michael: I know that, we're also maintaining Fedora packages, and for those thats easy to solve - but basically all other distributions (intentionally) don't do this, because they don't want to end up with thousands of packages, and almost all dependencies are required anyway. When providing precompiled packages, the goal is that they work without further effort for the average use-case, but still allow more complicated setups with some manual effort. For components that are required by a lot of applications splitting doesn't make sense, and all other dependencies are dynamically linked so far, and can easily be marked as optional dependency. For the user its very easy to enable additional features in Wine by just installing the optional dependencies, no manual rebuild required. I know that different distributions might have different opinions on that, but thats basically the way it works for a lot of them (for example all Archlinux and Debian builds, most likely also a lot more distributions). Wpcap is a bit special, because only few applications require it, and moreover it even requires the user to _manually adjust permissions_ on the wine-preloader executable. I would like the users to be able to do that, but without recompiling wine. @Julliard: Basically every build tool chain complains when linking statically against a library, which is not marked as dependency. But its your decision of course, I'm pretty sure I'll find a solution, I was just hoping for a general way to solve this upstream. Thanks anway. Regards, Sebastian > > bye > michael > >> Am 26.08.2014 um 11:55 schrieb Alexandre Julliard: >>> André Hentschel <nerv(a)dawncrow.de> writes: >>> >>>> This is intended for packagers, without this patch libpcap >>>> easily becomes a fixed dependency of wine. Something we don't >>>> want i guess. >>> >>> It's only used for wpcap, which can't work without it anyway, so >>> I don't see any need to make it dynamic. > >

Aug. 26, 2014

Re: wpcap: Load libpcap dynamically

by Alexandre Julliard

Sebastian Lackner <sebastian(a)fds-team.de> writes: > Linking it dynamically has a great advantage for distributions and > package maintainers providing precompiled wine builds. I would also > like to provide my wine builds with wpcap support (some users might > want to use it) - but when I do that a fixed dependency is > required. This is especially confusing when wine is only used as a > dependency, like in case of pipelight, where users are already > wondering why we now depend on libpcap (those are wine builds where it > was already enabled): > https://answers.launchpad.net/pipelight/+question/253591 Apart from making the code more complicated, the end result is the same, either way the dependency is required for the library to work, so I don't see why it would have to be treated differently when packaging. If the packaging tools insist on making it a requirement just because they see a dll linked to it, I'd suggest fixing the tools. -- Alexandre Julliard julliard(a)winehq.org

Aug. 26, 2014

Re: wpcap: Load libpcap dynamically

by Michael Stefaniuc

On 08/26/2014 12:47 PM, Sebastian Lackner wrote: > Linking it dynamically has a great advantage for distributions and > package maintainers providing precompiled wine builds. I would also > like to provide my wine builds with wpcap support (some users might > want to use it) - but when I do that a fixed dependency is > required. This is especially confusing when wine is only used as a > dependency, like in case of pipelight, where users are already > wondering why we now depend on libpcap (those are wine builds where > it was already enabled): > https://answers.launchpad.net/pipelight/+question/253591 > > I guess other package maintainers might probably think the same, > but most haven't updated to 1.7.25 yet, and are not aware of this > problem. Just converting the static dependency to a dynamic > dependency would be the easiest approach to solve this - the > alternative would be to split wine into (more) packages, and avoid > the libpcap dependency on the main one... or just leave this > feature exclusively for source-based distros where users have > fine-grained control about which features they want. :/ Fedora has subpackages for all those components requiring extra dependencies. In this case I guess they will go for a wine-pcap rpm. Installing the wine package will pull everything in. But the people that do care about the dependencies they have the granularity required. bye michael > Am 26.08.2014 um 11:55 schrieb Alexandre Julliard: >> André Hentschel <nerv(a)dawncrow.de> writes: >> >>> This is intended for packagers, without this patch libpcap >>> easily becomes a fixed dependency of wine. Something we don't >>> want i guess. >> >> It's only used for wpcap, which can't work without it anyway, so >> I don't see any need to make it dynamic.

Aug. 26, 2014