-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
Hi,
I was on holiday when you sent these patches, sorry for the late review.
Am 2014-12-13 um 18:06 schrieb Jonathan Vollebregt:
> +static LSTATUS sane_path(const WCHAR *key)
> +{
> + if (key[0] == '\\' && key[1] == '\\' && key[2] != '\\')
> + return ERROR_NO_REMOTE;
> +
> + return ERROR_SUCCESS;
> +}
This adds (or rather extends) a possible out of bound array access.
Otherwise these patches look …
[View More]reasonable to me and worth review by others.
A general note on the error handling code for other reviewers: Native reg.exe has very limited error reporting. With a few exceptions it just prints a generic message like "ERROR: INVALID SYNTAX" or something similar.
I guess there are also some possible improvements on the output code as Bruno mentioned, but I think patch 3 is OK until (if ever) we have unified console output handling for all command line programs.
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2
iQIcBAEBAgAGBQJUrEB5AAoJEN0/YqbEcdMw8bUP/1dhlLDCE2kOZhLkhVXg0RVa
BckpiGygdwfIIvGoyCaj6ObtgvrnYr7OMzZsruytK/X5UUvl+KsObctp8g1zjNY1
dFPQeBsra9TRtbtKTMX2pIdZS1wSNwWaOYrdZcKn5IkQfDWN0rwdKNujT8pAf+Ik
8dpFJUYtMjd6N0avPqphglRbW8rFhLhqYOlYuPjvzuEtBM11vGfy9mAp+sPA1ekX
8sRqGtn65w1JsT3cg054DvtY2xHNmRE5eTnrsbL7gqD/oljdPyU470eY6Vwofo3Z
frAF8OKemJsU6MrxZLXe1US5GzyvvCRRpIBNcfrO6qzTTdio0/LBe77VDq9J0jAa
GUWYFUtxIXw7Ccw1tNrxYp1JPo9bZnk5yONMMEAPfw2ij2iQCTvpv0lY9Kk58mqF
kKzWrqzhVdi5aFGOGhnmrk7NZ4l8paA7Gk2S1c8IukWubww0hlOclrhrWDYpuEfJ
ZKyGG8jFUvSjW3iTEES7NHMnCd+lWgaf+3TODrSGD5J/wdP6GQT9S78/HZ+SbM8m
fxnvnDLylq9arQm3bh0wQy9ZoMCTMx4YtbTouQBpLZBDKAgZNK1BUC5sxP1NlO9P
eP5JAZFHTzpGXCADkL+4OI4+ImG9mAMWGYytHGEQ3kwvAucNwTWP00twzMbSjHEX
+8k+frQxyupijPZrL4vo
=PJFk
-----END PGP SIGNATURE-----
[View Less]
> On Jan 5, 2015, at 9:17 AM, Matteo Bruni <mbruni(a)codeweavers.com> wrote:
>
> As an aside, reported WGL extensions don't depend on the specific
> GL context (e.g. WGL_ARB_pbuffer is reported as supported even on core
> profile contexts).
Do real Windows drivers behave like this?
> @@ -1272,6 +1273,175 @@ static BOOL init_gl_info(void)
[…]
> +/**********************************************************************
> + * create_context
> + */
> +…
[View More]static BOOL create_context(struct wgl_context *context, CGLContextObj share, BOOL core)
> +{
[…]
> + attribs[n++] = kCGLPFAAuxBuffers;
> + attribs[n++] = pf->aux_buffers;
You must reject any pixel format with >0 auxiliary buffers when creating a core profile context. CGL will specifically fail the ChoosePixelFormat (with error 10000, kCGLBadAttribute) if you specify both a GL version >= 3.2 and a non-zero number of auxiliary buffers.
> +
> + attribs[n++] = kCGLPFAColorSize;
> + attribs[n++] = color_modes[pf->color_mode].color_bits;
> + attribs[n++] = kCGLPFAAlphaSize;
> + attribs[n++] = color_modes[pf->color_mode].alpha_bits;
> + if (color_modes[pf->color_mode].is_float)
> + attribs[n++] = kCGLPFAColorFloat;
> +
> + attribs[n++] = kCGLPFADepthSize;
> + attribs[n++] = pf->depth_bits;
> +
> + attribs[n++] = kCGLPFAStencilSize;
> + attribs[n++] = pf->stencil_bits;
> +
> + if (pf->stereo)
> + attribs[n++] = kCGLPFAStereo;
> +
> + if (pf->accum_mode)
> + {
> + attribs[n++] = kCGLPFAAccumSize;
> + attribs[n++] = color_modes[pf->accum_mode - 1].color_bits;
> + }
You must also reject any pixel format with an accumulation buffer when creating a core profile context, for the same reason.
> +
> + /* Explicitly requesting pbuffers in CGLChoosePixelFormat fails with core contexts. */
> + if (pf->pbuffer && !core)
> + attribs[n++] = kCGLPFAPBuffer;
> +
> + if (pf->sample_buffers && pf->samples)
> + {
> + attribs[n++] = kCGLPFASampleBuffers;
> + attribs[n++] = pf->sample_buffers;
> + attribs[n++] = kCGLPFASamples;
> + attribs[n++] = pf->samples;
> + }
> +
> + if (pf->backing_store)
> + attribs[n++] = kCGLPFABackingStore;
> +
> + if (core)
> + {
> + attribs[n++] = kCGLPFAOpenGLProfile;
> + attribs[n++] = (int)kCGLOGLPVersion_3_2_Core;
> + }
There’s a constant for requesting a 4.x core context, too. (But it’s only defined in the 10.9 and 10.10 SDKs.) You might consider using it if the requested version is >= 4.0. That way, creation will fail if the system doesn’t support it.
> +
> + attribs[n] = 0;
> +
> + err = CGLChoosePixelFormat(attribs, &pix, &virtualScreens);
> + if (err != kCGLNoError || !pix)
> + {
> + WARN("CGLChoosePixelFormat() failed with error %d %s\n", err, CGLErrorString(err));
> + SetLastError(ERROR_INVALID_OPERATION);
This is somewhat nitpicking, but one thing you might consider is setting the last error based on what CGL returned. For example, if you get the error kCGLBadAlloc, you could set the last error to ERROR_NO_SYSTEM_RESOURCES.
> + return FALSE;
> + }
> +
> + err = CGLCreateContext(pix, share, &context->cglcontext);
> + CGLReleasePixelFormat(pix);
> + if (err != kCGLNoError || !context->cglcontext)
> + {
> + context->cglcontext = NULL;
> + WARN("CGLCreateContext() failed with error %d %s\n", err, CGLErrorString(err));
> + SetLastError(ERROR_INVALID_OPERATION);
Ditto.
> @@ -2076,6 +2246,133 @@ cant_match:
[…]
> +/***********************************************************************
> + * macdrv_wglCreateContextAttribsARB
> + *
> + * WGL_ARB_create_context: wglCreateContextAttribsARB
> + */
> +static struct wgl_context *macdrv_wglCreateContextAttribsARB(HDC hdc,
> + struct wgl_context *share_context,
> + const int *attrib_list)
> +{
[…]
> + if (major > 3 || (major == 3 && minor >= 2))
> + {
> + if (!(flags & WGL_CONTEXT_FORWARD_COMPATIBLE_BIT_ARB))
> + {
> + WARN("OS X only supports forward-compatible 3.2+ contexts\n");
> + SetLastError(ERROR_INVALID_VERSION_ARB);
> + return NULL;
> + }
Just so you know, a side effect of this is that our GL 3.x tests get skipped here, because they don’t specify the FOWARD_COMPATIBLE bit.
Also, you should consider rejecting the DEBUG flag, if it’s set: OS X never returns that flag. (Or do you want to hook glGetInteger(3G) to return the debug flag if it’s set?)
> + if (profile != WGL_CONTEXT_CORE_PROFILE_BIT_ARB)
> + {
> + WARN("Compatibility profiles for GL version >= 3.2 not supported\n");
> + SetLastError(ERROR_INVALID_PROFILE_ARB);
> + return NULL;
> + }
> + core = TRUE;
> + }
> + else if (major == 3)
> + {
> + WARN("OS X doesn't support 3.0 or 3.1 contexts\n");
> + SetLastError(ERROR_INVALID_VERSION_ARB);
> + return NULL;
> + }
I think we can support requests for 3.1 contexts, if the FORWARD_COMPATIBLE bit is set; we just won’t advertise GL_ARB_compatibility.
Chip
[View Less]
Am 29.12.2014 um 23:03 schrieb André Hentschel:
> ---
> loader/main.c | 26 ++++++++++++++++++--------
> 1 file changed, 18 insertions(+), 8 deletions(-)
[After a small IRC discussion]
My main goal is to also get the 2G/2G vmsplit warning on ARM, too. (and potential other platforms)
There are still many ARM kernels out there with a 2G split.
The problem is that Wine can't start on 2G/2G, it seems there are too much assumptions in the code for a 3G split.
e.g. for shared user data. …
[View More]We can't allocate 0x7ffe0000-0x7fff0000 on 2G split, most likely something else is already there, idk
[View Less]
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
Hi,
You have style inconsistencies in the patch. I won't point them out individually and just focus on suggestions on the code itself.
Overall: Is there a way to remove components? I don't see a method that supports that, and AddComponent with a NULL pointer returns an error, so I guess components can't be removed.
Am 2015-01-06 um 08:59 schrieb Alistair Leslie-Hughes:> +static void add_component(IDirectPlay8AddressImpl *This, struct …
[View More]component *item)
> +{
> + if(!This->comp_count)
> + This->components = HeapAlloc( GetProcessHeap(), 0, sizeof(struct component *) * This->comp_count+1 );
> + else
> + This->components = HeapReAlloc( GetProcessHeap(), 0, This->components, sizeof(struct component *) * This->comp_count+1 );
How many components are usually added? If it is just a small number (say, < 10) this is OK. Otherwise it would be more efficient to grow the array by a factor (e.g. duplicate the array size each time you grow it), so you make growing an O(log(n)) instead of O(n).
> + int i;
> + struct component *entry;
>
> - LIST_FOR_EACH_ENTRY_SAFE(entry, entry2, &This->components, struct component, entry)
> + for(i=0; i < This->comp_count; i++)
Mismatching data type. This->comp_count is a DWORD, i is an int. Please use DWORD consistently. (Using DWORD for the component is mandated by the parameters to GetComponentByIndex and return value of GetNumComponents)
> + switch (entry->type)
> + {
> + case DPNA_DATATYPE_DWORD:
> + memcpy(pvBuffer, &entry->data.value, sizeof(DWORD));
> + break;
> + case DPNA_DATATYPE_GUID:
> + memcpy(pvBuffer, &entry->data.guid, sizeof(GUID));
> + break;
> + case DPNA_DATATYPE_STRING:
> + memcpy(pvBuffer, &entry->data.string, entry->size);
> + break;
> + case DPNA_DATATYPE_STRING_ANSI:
> + memcpy(pvBuffer, &entry->data.ansi, entry->size);
> + break;
> + case DPNA_DATATYPE_BINARY:
> + memcpy(pvBuffer, &entry->data.binary, entry->size);
> + break;
> + }
You can use assignments for DPNA_DATATYPE_DWORD and DPNA_DATATYPE_GUID. The same applies to the existing code in GetComponentByName. AddComponent already uses assignments.
> --- a/dlls/dpnet/dpnet_private.h
> +++ b/dlls/dpnet/dpnet_private.h
> @@ -94,7 +94,8 @@ struct IDirectPlay8AddressImpl
> GUID SP_guid;
> BOOL init;
>
> - struct list components;
> + struct component **components;
> + DWORD comp_count;
Do you need the extra indirection? If the number of components is small you can work without it I think.
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2
iQIcBAEBAgAGBQJUq/bOAAoJEN0/YqbEcdMwOnMP/RnnAWVfyTuK2ABnRqDLW9/1
DydTHhjeOpm1FDKJTy7ebzqU76HDL7YWJOqwSNYPhtr8e3omXt5nApptwVjOBst0
lpRxDju4sPGo9GYePy5w39TI/w78gPLEGLVHpIiV5GlMz+YRpgxKI05Q90jkGtfo
whPpQ9ns1xiXz+d07iJnPn8GnEjWeTkIN2NnY6Nj+ARQ8rmUNSn3YR4ZGtZfuzrt
nKUurOt04FUAqQzDNYNbdtpybYQq7SSVQr8vWpJAJOpedq7oNyCEdjC8Zl5Yitl5
bg6a4oTvl7PgGmYOSvNVzsh6L0dYv0HpjLZsfLUo97ULXRsCFgfX8yNogrCSq8o0
2KoZSWEhehn1RV+WCywMbxHxly5ZvkyEr9g9/h+8+7cpPrdg8TeoaMUjtjUxrqra
fLDfLtGnMGs4DGiFmleRR3lBFmr8UMVkP4/YLEJ1HPKNdAAwXfQso6/ODmdvC78W
5sRfY/pSlLEn93szGfx+hunpUX30c2XkGmuBElLJQsQgVeoijmsNhUNOLUdhTx9k
wPhslaQ+HCtR76KVghvQzN3iBHPuO+O0dfH4PWz5/7Grq5/pIhfbWcMCVJJM3d9u
CNjKAOkw+NzPPsdLyJZsmMWWvv+BJ039l15xjMTmWxG2uH/QgaiBtvDyD2/u3KxJ
MgsfOYAWEYondC/Jllaz
=uIjr
-----END PGP SIGNATURE-----
[View Less]
The original intent was that this could be used by d3dx9 to implement
reading of the DIB format, but I didn't have the knowledge of d3dx9 to
test that this really does what is needed.
Mark Harmstone <hellas(a)burntcomma.com> writes:
> diff --git a/dlls/xaudio2_7/xaudio2_7.spec b/dlls/xaudio2_7/xaudio2_7.spec
> new file mode 100644
> index 0000000..e69de29
Please list stubs for all the exported functions.
--
Alexandre Julliard
julliard(a)winehq.org
On 5 January 2015 at 17:17, Matteo Bruni <mbruni(a)codeweavers.com> wrote:
> static BOOL has_extension( const char *list, const char *ext, size_t len )
> {
> + if (!list)
> + {
> + const struct opengl_funcs *funcs = NtCurrentTeb()->glTable;
> + const char *gl_ext;
> + unsigned int i;
> + GLint extensions_count;
> +
> + glGetIntegerv(GL_NUM_EXTENSIONS, &extensions_count);
> + for (i = 0; i < …
[View More]extensions_count; ++i)
> + {
> + gl_ext = (const char *)funcs->ext.p_glGetStringi(GL_EXTENSIONS, i);
> + if (!strncmp(gl_ext, ext, len) && !gl_ext[len])
> + return TRUE;
> + }
> + return FALSE;
> + }
> +
If I'm reading this correctly, this effectively ignores
DisabledExtensions for anything newer than GL 3.0. (And at least as
far as wglGetProcAddress() is concerned it affects both compatibility
and core contexts.)
> + gl_version = (const char *)glGetString(GL_VERSION);
> + sscanf(gl_version, "%u", &major);
> + if (major >= 3)
> + {
It seems tempting to just flag the context in
wglCreateContextAttribsARB() under the appropriate conditions, perhaps
in a similar way to wgl_handle_type. Not sure if it would really make
it better though.
As an aside, note that winex11.drv also uses
"glGetString(GL_EXTENSIONS);" in X11DRV_WineGL_InitOpenglInfo().
[View Less]
On Jan 5, 2015, at 10:17 AM, Matteo Bruni <mbruni(a)codeweavers.com> wrote:
> +#if !defined(MAC_OS_X_VERSION_10_7) || MAC_OS_X_VERSION_MAX_ALLOWED < MAC_OS_X_VERSION_10_7
> + if (core)
> + {
> + WARN("OS X version >= 10.7 is required to be able to create core contexts\n",
> + err, CGLErrorString(err));
The "err, CGLErrorString(err)" arguments are superfluous.
> + return FALSE;
> + }
> + if (core)
> + {
> + …
[View More] attribs[n++] = kCGLPFAOpenGLProfile;
> + attribs[n++] = (int)kCGLOGLPVersion_3_2_Core;
> + }
These lines won't compile when the SDK is earlier than 10.7. So, they also need to be guarded with the appropriate #if.
> + case WGL_CONTEXT_PROFILE_MASK_ARB:
> + if ((value & ~VALID_PROFILE_BITS_MASK) || !value ||
> + (value & VALID_PROFILE_BITS_MASK) == VALID_PROFILE_BITS_MASK)
I think it would be better to simply do:
if (value != WGL_CONTEXT_CORE_PROFILE_BIT_ARB &&
value != WGL_CONTEXT_COMPATIBILITY_PROFILE_BIT_ARB)
Or did I miss something about the logic you used?
> + {
> + WARN("WGL_CONTEXT_PROFILE_MASK_ARB bits %#x invalid\n", value);
> + SetLastError(ERROR_INVALID_PROFILE_ARB);
> + return NULL;
> + }
> + profile = value;
> + break;
> + register_extension("WGL_ARB_create_context");
> + register_extension("WGL_ARB_create_context_profile");
> + opengl_funcs.ext.p_wglCreateContextAttribsARB = macdrv_wglCreateContextAttribsARB;
These should only be advertised when building against the 10.7 SDK or later. So, this should be guarded by the SDK #if directive.
Also, given that it's possible to build against a 10.7+ SDK but target deployment back to 10.6, I'm tempted to say we should determine if core profiles are actually available and only advertise those extensions if so. Basically, call CGLChoosePixelFormat() with the simplest set of attributes that specify the core profile and see if it succeeds.
-Ken
[View Less]
On 5 January 2015 at 17:17, Matteo Bruni <mbruni(a)codeweavers.com> wrote:
> +#define MAP_GL_FUNCTION(core_func, ext_func) \
> + { \
> + if (!gl_info->gl_ops.ext.p_##core_func) \
> + gl_info->gl_ops.ext.p_##core_func = gl_info->gl_ops.ext.p_##ext_func; \
> + }
> +
> + MAP_GL_FUNCTION(…
[View More]glActiveTexture, glActiveTextureARB);
> +#undef MAP_GL_FUNCTION
> +
This kind of thing doesn't exactly fill me with joy, but I guess the
alternatives wouldn't be much better.
Any reason this can't be part of load_gl_funcs()? (And while we're
touching load_gl_funcs(), I don't see much of a reason we couldn't
just inline GL_EXT_FUNCS_GEN these days.)
> + /* Newer core functions */ \
> + USE_GL_FUNC(glActiveTexture) \
It would probably be nice to note the exact GL version where the
function entered core GL.
[View Less]