Re: [1/3] WineD3D: check the GL flags in CheckDepthStencilCapability, not the static info
On 24 March 2010 23:53, Stefan Dösinger ste...@codeweavers.com wrote: These flags will be unset if the format is not supported by GL and is consistent with the other depth stencil check functions. So what happens if the GL implementation doesn't support ARB_depth_texture, for example?
Re: [1/3] WineD3D: check the GL flags in CheckDepthStencilCapability, not the static info
On Thu, Mar 25, 2010 at 9:10 AM, Henri Verbeet hverb...@gmail.com wrote: On 24 March 2010 23:53, Stefan Dösinger ste...@codeweavers.com wrote: These flags will be unset if the format is not supported by GL and is consistent with the other depth stencil check functions. So what happens if the GL implementation doesn't support ARB_depth_texture, for example? My suggestion was to extend 'init_format_texture_info' and set 'wined3d_format_desc-supported' when the extension is around. Roderick
Re: [1/3] WineD3D: check the GL flags in CheckDepthStencilCapability, not the static info
On 25 March 2010 09:17, Roderick Colenbrander thunderbir...@gmail.com wrote: My suggestion was to extend 'init_format_texture_info' and set 'wined3d_format_desc-supported' when the extension is around. That doesn't help, the problem is with fairly old cards that don't support depth textures. They obviously won't be able to do fancy stuff like FBO offscreen rendering, but you should still be able to create a depth/stencil buffer. The current code on the other hand will incorrectly report floating point depth formats as supported even if the relevant extension isn't supported.
Re: [PATCH] user32: end scrollbar tracking if the mouse capture is lost (retry)
On 03/25/2010 09:15 AM, Sébastien Ramage wrote: --- dlls/user32/scroll.c | 5 + dlls/user32/tests/scroll.c | 16 +++- 2 files changed, 16 insertions(+), 5 deletions(-) Hi You can see at http://source.winehq.org/patches/ that your patch '59740' had an apply failure. And fair enough: $ patch -p 1 -i /home/paul/0001-PATCH-user32-end-scrollbar-tracking-if-the-mouse-captu.txt (Stripping trailing CRs from patch.) patching file dlls/user32/scroll.c (Stripping trailing CRs from patch.) patching file dlls/user32/tests/scroll.c You have DOS line endings in your patch. A simple dos2unix fixes this. -- Cheers, Paul.
Re: [PATCH] [WineDbg]: allow to specify which format to be used for integers in backtraces
Eric Pouech eric.pou...@orange.fr wrote: my point was not just to say that most of the information put in current TRACE/WARN/FIXME/ERR statements contain more decimal than hexadecimal formats so obviously, not everybody expects hexa all over the place Of course each TRACE statements serve the pupose of each particular place in the code it's placed in. A backtrace is meaningful only in the context of a relay trace, so it should match the format of relay traces in the first place (besides other reasons I mentioned earlier). -- Dmitry.
Re: [PATCH 2/8] msvcr90: Implement _encode_pointer and _decode_pointer (try4)
Piotr Caban pi...@codeweavers.com writes: --- dlls/msvcr80/msvcr80.spec|4 ++-- dlls/msvcr90/msvcr90.c | 21 + dlls/msvcr90/msvcr90.spec|4 ++-- dlls/msvcr90/tests/msvcr90.c | 26 +- 4 files changed, 50 insertions(+), 5 deletions(-) Still doesn't build on Mingw I'm afraid: make[1]: Entering directory `/home/julliard/wine/build/obj-pe64/dlls/msvcr80' ../../../obj-elf64/tools/winegcc/winegcc -b x86_64-pc-mingw32 -m64 -B../../../obj-elf64/tools/winebuild --sysroot=../.. -shared ../../../wine/dlls/msvcr80/msvcr80.spec msvcr80.o-o msvcr80.dll -lmsvcrt -lmsvcr90 -lkernel32 ../../libs/port/libwine_port.a /home/julliard/wine/mingw/mingw64/bin/../lib/gcc/x86_64-pc-mingw32/4.5.0/../../../../x86_64-pc-mingw32/lib/libmingw32.a(lib64_libmingw32_a-mingw_helpers.o): In function `_decode_pointer': /home/buildbot/mingwslave/linux-x86_64/build/mingw/obj/../mingw-w64-crt/crt/mingw_helpers.c:20: multiple definition of `__decode_pointer' ../../dlls/msvcr90/libmsvcr90.a(dwaes00111.o):(.text+0x0): first defined here /home/julliard/wine/mingw/mingw64/bin/../lib/gcc/x86_64-pc-mingw32/4.5.0/../../../../x86_64-pc-mingw32/lib/libmingw32.a(lib64_libmingw32_a-mingw_helpers.o): In function `_encode_pointer': /home/buildbot/mingwslave/linux-x86_64/build/mingw/obj/../mingw-w64-crt/crt/mingw_helpers.c:26: multiple definition of `__encode_pointer' ../../dlls/msvcr90/libmsvcr90.a(dwaes00118.o):(.text+0x0): first defined here collect2: ld returned 1 exit status x86_64-pc-mingw32-dllwrap: x86_64-pc-mingw32-gcc exited with status 1 winegcc: x86_64-pc-mingw32-dllwrap failed make[1]: *** [msvcr80.dll] Error 2 -- Alexandre Julliard julli...@winehq.org
Re: [1/3] WineD3D: check the GL flags in CheckDepthStencilCapability, not the static info
Am Donnerstag 25 März 2010 09:10:19 schrieb Henri Verbeet: So what happens if the GL implementation doesn't support ARB_depth_texture, for example? Good point. Maybe we should only look at those flags when we're using FBOs? I think the other format checking functions handle it that way, I wondered why. Patch 3 would still prevent float formats from being advertised without FBOs. GL_EXT_framebuffer_object doesn't require GL_ARB_depth_texure, so technically we might end up using FBOs without depth texture support. Possibility 1 is that DEPTH_COMPONENT render buffers are still supposed to work, possibility 2 is that FBO rendering can't support depth buffers, which would make them useless for our purpose.
Re: [1/3] WineD3D: check the GL flags in CheckDepthStencilCapability, not the static info
On Thu, Mar 25, 2010 at 9:26 AM, Henri Verbeet hverb...@gmail.com wrote: On 25 March 2010 09:17, Roderick Colenbrander thunderbir...@gmail.com wrote: My suggestion was to extend 'init_format_texture_info' and set 'wined3d_format_desc-supported' when the extension is around. That doesn't help, the problem is with fairly old cards that don't support depth textures. They obviously won't be able to do fancy stuff like FBO offscreen rendering, but you should still be able to create a depth/stencil buffer. The current code on the other hand will incorrectly report floating point depth formats as supported even if the relevant extension isn't supported. Somehow I thought (perhaps because that's what I discussed with Stefan) that he added an explicit FBO check to CheckDepthStencilCapability to separate FBO and classic WGL. In the FBO path he can use the detection he uses now augmented with a 'supported' check. At least that's what I had in mind. I prefer to limit the use of IsPixelF* to wgl stuff like checking whether an adapter format is supported and for the non-FBO case. Float checks would help in the non-FBO case to prevent getColorBits warnings. Roderick
Re: [1/3] WineD3D: check the GL flags in CheckDepthStencilCapability, not the static info
On Thu, Mar 25, 2010 at 10:02 AM, Stefan Dösinger ste...@codeweavers.com wrote: Am Donnerstag 25 März 2010 09:10:19 schrieb Henri Verbeet: So what happens if the GL implementation doesn't support ARB_depth_texture, for example? Good point. Maybe we should only look at those flags when we're using FBOs? I think the other format checking functions handle it that way, I wondered why. Patch 3 would still prevent float formats from being advertised without FBOs. GL_EXT_framebuffer_object doesn't require GL_ARB_depth_texure, so technically we might end up using FBOs without depth texture support. Possibility 1 is that DEPTH_COMPONENT render buffers are still supposed to work, possibility 2 is that FBO rendering can't support depth buffers, which would make them useless for our purpose. Note I don't know much about the current FBO extension situation but since FBOs are finally quite common, can't we expect some features to be around? If this functionality is missing, perhaps we should just not work and let vendors fix their drivers. I guess AMD and Nvidia are quite decent and perhaps Mesa also supports the needed stuff. Not sure about Apple. Roderick
Re: [1/3] WineD3D: check the GL flags in CheckDepthStencilCapability, not the static info
On 25 March 2010 10:02, Stefan Dösinger ste...@codeweavers.com wrote: Good point. Maybe we should only look at those flags when we're using FBOs? I think the other format checking functions handle it that way, I wondered why. Probably. GL_EXT_framebuffer_object doesn't require GL_ARB_depth_texure, so technically we might end up using FBOs without depth texture support. Possibility 1 is that DEPTH_COMPONENT render buffers are still supposed to work, possibility 2 is that FBO rendering can't support depth buffers, which would make them useless for our purpose. I think render buffers would still depend on formats exposed ARB_depth_texture. We could simply disable FBOs if neither ARB_depth_texture or any of the packed_depth_stencil variants is suppoerted, but it's probably unlikely enough that we can just worry about that if we ever hit it.
Re: [PATCH] user32: end scrollbar tracking if the mouse capture is lost (retry)
On 03/25/2010 09:56 AM, Sébastien Ramage wrote: You have DOS line endings in your patch. A simple dos2unix fixes this. Thank you Attached the patch with unix line endings Seb Patch should go to wine-patches. Please mark as [Try x] and tell what the difference to the previous version is. -- Cheers, Paul.
Re: [1/3] WineD3D: check the GL flags in CheckDepthStencilCapability, not the static info
Am Donnerstag 25 März 2010 10:06:21 schrieb Roderick Colenbrander: Note I don't know much about the current FBO extension situation but since FBOs are finally quite common, can't we expect some features to be around? If this functionality is missing, perhaps we should just not work and let vendors fix their drivers. I guess AMD and Nvidia are quite decent and perhaps Mesa also supports the needed stuff. Not sure about Apple. Mesa supports FBOs these days, but not on radeon cards prior to r200 chips. Nouveau and Intel should be ok, I don't know about matrox and other odd cards. I think we could justify killing backbuffer offscreen rendering(so we only have FBOs left), but we cannot justify not running at all without FBOs. Then thinking is that pretty much all apps that use offscreen rendering are dx8+ ones, so games that need offscreen rendering are unlikely to run on a Radeon 7500 anyway. There's of course the situation that KMS-based Mesa currently has sucky performance and many users still have old drivers installed.
Re: [PATCH 9/9] ws2_32/tests: add some socket event tests
Hi Mike, From: Mike Kaplinskiy I recently discovered that winetestbot doesn't run on win98 by default - will keep that in mind in the future. Is there any reason for that? Not really, apart from a certain amount of hate on my side towards Win9x. I've added W98SE (which you might have guessed runs Windows 98 Second Edition) to the default set. Ge. smime.p7s Description: S/MIME cryptographic signature
64bit bit Wine users, please test patch
Hi, you guys running Wine64 on 64bit UNIX, please test the patch attached to bug #22146 http://bugs.winehq.org/show_bug.cgi?id=22146 The patch is available from http://bugs.winehq.org/attachment.cgi?id=27027 Test code that you can use for reporting is at: http://bugs.winehq.org/attachment.cgi?id=27028 I'm afraid my patch does not apply cleanly in Wine-1.1.41. You'll likely need to skip, remove or correct the one hunk about TRACE(...data[]...). It's not essential. Actually, I've removed %s formatting from the 64bit trace because I don't know how Wine behaves when given a bad pointer -- TRACE(%s,debugstr(random_location)) could even cause a core dump, couldn't it? Thank you very much, Jörg Höhle
Re: [PATCH] user32: end scrollbar tracking if the mouse capture is lost (retry)
You have DOS line endings in your patch. A simple dos2unix fixes this.
Thank you
Attached the patch with unix line endings
Seb
From 44bf3ca4f53864d70d6ae89ec705db83e51e9796 Mon Sep 17 00:00:00 2001
From: =?utf-8?q?S=C3=A9bastien=20Ramage?= sebastien.ram...@gmail.com
Date: Thu, 25 Mar 2010 09:00:29 +0100
Subject: [PATCH] user32: end scrollbar tracking if the mouse capture is lost
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary=1.6.3.3
This is a multi-part message in MIME format.
--1.6.3.3
Content-Type: text/plain; charset=UTF-8; format=fixed
Content-Transfer-Encoding: 8bit
---
dlls/user32/scroll.c |5 +
dlls/user32/tests/scroll.c | 16 +++-
2 files changed, 16 insertions(+), 5 deletions(-)
--1.6.3.3
Content-Type: text/x-patch;
name=0001-PATCH-user32-end-scrollbar-tracking-if-the-mouse-captu.txt
Content-Transfer-Encoding: 8bit
Content-Disposition: attachment;
filename=0001-PATCH-user32-end-scrollbar-tracking-if-the-mouse-captu.txt
diff --git a/dlls/user32/scroll.c b/dlls/user32/scroll.c
index 423dea1..ab21489 100644
--- a/dlls/user32/scroll.c
+++ b/dlls/user32/scroll.c
@@ -1125,11 +1125,8 @@ void SCROLL_TrackScrollBar( HWND hwnd, INT scrollbar,
POINT pt )
DispatchMessageW( msg );
}
if (!IsWindow( hwnd ))
-{
ReleaseCapture();
-break;
-}
-} while (msg.message != WM_LBUTTONUP);
+} while (msg.message != WM_LBUTTONUP GetCapture() == hwnd);
}
diff --git a/dlls/user32/tests/scroll.c b/dlls/user32/tests/scroll.c
index 0695639..e3f6938 100644
--- a/dlls/user32/tests/scroll.c
+++ b/dlls/user32/tests/scroll.c
@@ -42,12 +42,25 @@ static LRESULT CALLBACK MyWndProc( HWND hWnd, UINT msg,
WPARAM wParam, LPARAM lP
case WM_DESTROY:
PostQuitMessage(0);
break;
-
+case WM_HSCROLL:
+case WM_VSCROLL:
+/* stop tracking */
+ReleaseCapture();
+return 0;
default:
return DefWindowProcA(hWnd, msg, wParam, lParam);
}
return 0;
}
+static void scrollbar_test_track(void)
+{
+/* test that scrollbar tracking is terminated when
+ * the control looses mouse capture */
+SendMessage( hScroll, WM_LBUTTONDOWN, 0, MAKELPARAM( 1, 1));
+/* a normal return from the sendmessage */
+/* not normal for instance by closing the windws */
+ok( IsWindow( hScroll), Scrollbar has gone!\n);
+}
static void scrollbar_test1(void)
{
@@ -420,6 +433,7 @@ START_TEST ( scroll )
scrollbar_test2();
scrollbar_test3();
scrollbar_test4();
+scrollbar_test_track();
/* Some test results vary depending of theming being active or not */
hUxtheme = LoadLibraryA(uxtheme.dll);
--1.6.3.3--
Re: 64bit bit Wine users, please test patch
joerg-cyril.hoe...@t-systems.com schrieb: Hi, you guys running Wine64 on 64bit UNIX, please test the patch attached to bug #22146 http://bugs.winehq.org/show_bug.cgi?id=22146 The patch is available from http://bugs.winehq.org/attachment.cgi?id=27027 Test code that you can use for reporting is at: http://bugs.winehq.org/attachment.cgi?id=27028 I'm afraid my patch does not apply cleanly in Wine-1.1.41. You'll likely need to skip, remove or correct the one hunk about TRACE(...data[]...). It's not essential. Actually, I've removed %s formatting from the 64bit trace because I don't know how Wine behaves when given a bad pointer -- TRACE(%s,debugstr(random_location)) could even cause a core dump, couldn't it? Thank you very much, Jörg Höhle It seems you missed something in your patch, i get: gcc -c -I../../../../dlls/winmm/tests -I. -I../../../../include -I../../../include -DWINE_STRICT_PROTOTYPES -D_REENTRANT -fPIC -Wall -pipe -fno-strict-aliasing -Wdeclaration-after-statement -Wstrict-prototypes -Wtype-limits -Wwrite-strings -Wpointer-arith -g -O2 -o mci.o ../../../../dlls/winmm/tests/mci.c ../../../../dlls/winmm/tests/mci.c: In function ‘test_mciParser’: ../../../../dlls/winmm/tests/mci.c:183: warning: format ‘%u’ expects type ‘unsigned int’, but argument 3 has type ‘long unsigned int’ ../../../../dlls/winmm/tests/mci.c:184: warning: format ‘%u’ expects type ‘unsigned int’, but argument 3 has type ‘long unsigned int’ ../../../../dlls/winmm/tests/mci.c:187: warning: statement with no effect ../../../../dlls/winmm/tests/mci.c:193: warning: statement with no effect ../../../../dlls/winmm/tests/mci.c:202: error: ‘MCI_PARMS_UNION’ has no member named ‘caps’ ../../../../dlls/winmm/tests/mci.c:203: error: ‘MCI_PARMS_UNION’ has no member named ‘caps’ ../../../../dlls/winmm/tests/mci.c:206: error: ‘MCI_PARMS_UNION’ has no member named ‘caps’ ../../../../dlls/winmm/tests/mci.c:206: error: ‘MCI_PARMS_UNION’ has no member named ‘caps’ ../../../../dlls/winmm/tests/mci.c:208: error: ‘MCI_PARMS_UNION’ has no member named ‘caps’ ../../../../dlls/winmm/tests/mci.c:209: error: ‘MCI_PARMS_UNION’ has no member named ‘caps’ ../../../../dlls/winmm/tests/mci.c:212: error: ‘MCI_PARMS_UNION’ has no member named ‘caps’ ../../../../dlls/winmm/tests/mci.c:212: error: ‘MCI_PARMS_UNION’ has no member named ‘caps’ ../../../../dlls/winmm/tests/mci.c:214: error: ‘MCI_PARMS_UNION’ has no member named ‘caps’ ../../../../dlls/winmm/tests/mci.c:215: error: ‘MCI_PARMS_UNION’ has no member named ‘caps’ ../../../../dlls/winmm/tests/mci.c:218: error: ‘MCI_PARMS_UNION’ has no member named ‘caps’ ../../../../dlls/winmm/tests/mci.c:218: error: ‘MCI_PARMS_UNION’ has no member named ‘caps’ make: *** [mci.o] Fehler 1 -- Best Regards, André Hentschel
Re: Patch: Bug 19668 cmd /c doesn't wait before exiting
Hi Anh and Welcome to Wine! It seems your patch got mangled in transit, e.g. git apply says: fatal: corrupt patch at line 33 If you have a look at it at http://source.winehq.org/patches/ you see that the lines are wrapped. -- Best Regards, André Hentschel
Re: [1/2] dlls/crypt32: implement PFXImportCertStore()
Hi Juan,
Thanks for reviewing my patches. Here are my comments:
this attempt looks pretty incomplete. First off:
+ret = pPKCS12_verify_mac(pkcs12, password, len);
+if (ret == 0)
+ERR_(crypt)(failed to verify pkcs12 {%p} with password
\%s\ using func {%p}\n, pkcs12, password, pPKCS12_verify_mac);
+else
+TRACE_(crypt)(verified pkcs12 {%p} with password \%s\
using func {%p}\n, pkcs12, password, pPKCS12_verify_mac);
You accept the PKCS12 file even if the password is incorrect. This is
clearly wrong.
It is not accepted. If the verification fails, ERR is spewed out and the next
step (parse, below) will fail as well.
+/* extract private key and certificate */
+ret = pPKCS12_parse(pkcs12, password, pkey, cert, NULL);
You don't support more than a single certificate in the PKCS12 file.
This may be fine for the majority of uses, but at least a warning
indicating more certificates are present would be helpful.
Hmmm. How do you suggest I do that? From
http://www.openssl.org/docs/crypto/PKCS12_parse.html I get this:
BUGS
Only a single private key and corresponding certificate is returned by this
function. More complex PKCS#12
files with multiple private keys will only return the first match.
Also, a
PKCS12 file can contain more than just certificates, and the tests
ought at least to check this. For example, what about a PKCS12 file
with a CRL in it?
I have not seen, nor needed to implement this, so I'm not sure how to test for
it. Maybe add a comment to the test? Or a wine_todo test so we don't lose this
information?
OpenSSL also supports at least a couple of attributes associated with
the certificates. From the man page for PKCS12_parse:
The friendlyName and localKeyID attributes (if present) on each
certificate will be stored in the alias and keyid attributes of the
X509 structure.
The Crypto API also supports setting such attributes, and if you
aren't going to support these, at least the tests should cover them
(and marked todo_wine) so we know they're still not done.
Same answer. I guess I can update the test set with more wine_todo().
More questions:
+const WCHAR storeName[] = {'S', 't', 'o', 'r', 'e', ' ', 'N',
'a', 'm', 'e', 0};
+store = CertOpenStore(CERT_STORE_PROV_MEMORY, 0,
(HCRYPTPROV)NULL, CERT_STORE_CREATE_NEW_FLAG, storeName);
What's the purpose of the store name? Does the native implementation
do this? If not, it can be omitted. If so, the tests should check
it.
If you create a store with no name, you run the risk of it not being created
(if there is another store with no name).
I don't see a way from native to get the store name as it was created (only the
Localized Name, which may or may not be what we want).
Philippe
Re: [2/2] dlls/crypt32: unit test PFXImportCertStore()
Hi Juan,
Hi Philippe, in addition to the question I had on patch 1/2, I'd like
to know, what is the purpose of this test?
+static void test_createCertificateStore(void)
+{
+HCERTSTORE certStore = NULL;
+
+certStore = CertOpenStore(CERT_STORE_PROV_SYSTEM, 0, (HCRYPTPROV)NULL,
+CERT_SYSTEM_STORE_CURRENT_USER, LTest Store);
+ok(certStore != NULL, could not open the system certificate store\n);
+if (certStore)
+{
+CertCloseStore(certStore, 0);
+}
This is already covered by tests in store.c, and you're not using
Test Store elsewhere in the tests, so I don't see why this is
necessary.
I was doing test-driven development and wanted to start with a store, so I made
a test that made sure I could get a store. My environment is set up so that I
can only run the decode section of the tests (instead of running all the
tests all the time, which can be quite slow), hence its inclusion.
I have removed it from this patch for the next submission, and added the
wine_todo discussed in the 1/2 message.
For future reference, L string constants aren't portable, don't use
them in the tests.
Right. That's what I did in the implementation, forgot to do it in the test.
And, please use CertOpenStoreW if you're going to
use a wide-string parameter to it, or CertOpenStoreA if you're going
to use an ASCII string.
Sure. Won't that thunk down to the same thing, though?
Philippe
Re: [1/2] dlls/crypt32: implement PFXImportCertStore()
Hi Philippe, You accept the PKCS12 file even if the password is incorrect. This is clearly wrong. It is not accepted. If the verification fails, ERR is spewed out and the next step (parse, below) will fail as well. Is this how Windows fails? That is, with a parse error? Please add a test to cover this case. You don't support more than a single certificate in the PKCS12 file. This may be fine for the majority of uses, but at least a warning indicating more certificates are present would be helpful. Hmmm. How do you suggest I do that? From http://www.openssl.org/docs/crypto/PKCS12_parse.html I get this: BUGS Only a single private key and corresponding certificate is returned by this function. More complex PKCS#12 files with multiple private keys will only return the first match. Look at the 5th parameter of PKCS12_parse. It's true that OpenSSL will only return a single certificate with a private key, but not every certificate in a PKCS12 file need contain a private key. Also, a PKCS12 file can contain more than just certificates, and the tests ought at least to check this. For example, what about a PKCS12 file with a CRL in it? I have not seen, nor needed to implement this, so I'm not sure how to test for it. Maybe add a comment to the test? Or a wine_todo test so we don't lose this information? Test for it the way you should any Wine test: on Windows. Create a store with a CRL in it, export it to a PKCS12 file, and use that as your test case. The Crypto API also supports setting such attributes, and if you aren't going to support these, at least the tests should cover them (and marked todo_wine) so we know they're still not done. Same answer. I guess I can update the test set with more wine_todo(). Yes, I'd appreciate that. If you create a store with no name, you run the risk of it not being created (if there is another store with no name). Not for a memory store, it's just a linked list. --Juan
Re: [PATCH 6/8] msvcrt: Added _strtoi64 implementation (try4)
Piotr Caban pi...@codeweavers.com writes: --- dlls/msvcr80/msvcr80.spec |4 +- dlls/msvcr90/msvcr90.spec |4 +- dlls/msvcrt/msvcrt.h | 44 dlls/msvcrt/msvcrt.spec|4 +- dlls/msvcrt/string.c | 80 dlls/msvcrt/tests/string.c | 123 include/msvcrt/crtdefs.h | 52 ++ 7 files changed, 305 insertions(+), 6 deletions(-) This one doesn't build on Mingw either, because of crtdefs.h conflicts. -- Alexandre Julliard julli...@winehq.org
Re: [1/2] dlls/crypt32: implement PFXImportCertStore()
On 2010-03-25, at 12:14 PM, Juan Lang wrote: Hi Philippe, You accept the PKCS12 file even if the password is incorrect. This is clearly wrong. It is not accepted. If the verification fails, ERR is spewed out and the next step (parse, below) will fail as well. Is this how Windows fails? That is, with a parse error? Please add a test to cover this case. I don't have any password-protected certificates to test with, so I can't add such a test (it was not required for our implementation). You don't support more than a single certificate in the PKCS12 file. This may be fine for the majority of uses, but at least a warning indicating more certificates are present would be helpful. Hmmm. How do you suggest I do that? From http://www.openssl.org/docs/crypto/PKCS12_parse.html I get this: BUGS Only a single private key and corresponding certificate is returned by this function. More complex PKCS#12 files with multiple private keys will only return the first match. Look at the 5th parameter of PKCS12_parse. It's true that OpenSSL will only return a single certificate with a private key, but not every certificate in a PKCS12 file need contain a private key. So what you want is to import two new functions (sk_X509_new_null() and sk_X509_free()) and use them to create a STACK_OF(X509) whose sole purpose is to detect if there are more certificates, and then ERR or TRACE if there are, and then dispose? I added a FIXME comment. Also, a PKCS12 file can contain more than just certificates, and the tests ought at least to check this. For example, what about a PKCS12 file with a CRL in it? I have not seen, nor needed to implement this, so I'm not sure how to test for it. Maybe add a comment to the test? Or a wine_todo test so we don't lose this information? Test for it the way you should any Wine test: on Windows. Create a store with a CRL in it, export it to a PKCS12 file, and use that as your test case. I added the information as comments inside the test case. Mailing lists are harder to search than source code. The Crypto API also supports setting such attributes, and if you aren't going to support these, at least the tests should cover them (and marked todo_wine) so we know they're still not done. Same answer. I guess I can update the test set with more wine_todo(). Yes, I'd appreciate that. I did not realize that you wanted the actual set of test cases, disabled with todo_wine in front. I have added this information as comments inside the test case. If you create a store with no name, you run the risk of it not being created (if there is another store with no name). Not for a memory store, it's just a linked list. Ah. In that case, it does not really matter what the string is, right? I can remove it if you don't want it. Philippe
Re: [1/2] dlls/crypt32: implement PFXImportCertStore()
Whoops, forgot to cc wine-devel. I don't have any password-protected certificates to test with, so I can't add such a test (it was not required for our implementation). You can if you create one (on Windows.) So what you want is to import two new functions (sk_X509_new_null() and sk_X509_free()) and use them to create a STACK_OF(X509) whose sole purpose is to detect if there are more certificates, and then ERR or TRACE if there are, and then dispose? No, I suppose not. A test marked todo_wine would be better. Ah. In that case, it does not really matter what the string is, right? I can remove it if you don't want it. Please do ;) --Juan
Re: usbhub.sys: add stubbed usbhub.sys
On Wed, Mar 24, 2010 at 10:04 PM, Alexandre Julliard julli...@winehq.org wrote: Damjan Jovanovic damjan@gmail.com writes: On Wed, Mar 24, 2010 at 9:51 PM, Alexandre Julliard julli...@winehq.org wrote: Damjan Jovanovic damjan@gmail.com writes: usbhub.sys isn't accessed via exports and I don't think anything depends on usbhub.sys specifically - but on Windows it builds device objects for USB devices and does I/O to these, so it seems like the best place to do those in Wine too. I'm not convinced that splitting this stuff across modules is a good idea. There isn't much reason to replicate the Windows I/O stack layout because our devices work quite differently. -- Alexandre Julliard julli...@winehq.org So should I do everything in usbd.sys then? I expect that will be a lot easier. I'm happy to be proven wrong, if you find that usbhub.sys works better that's fine, I just don't want to add modules now to find out later that they can't be made to work. -- Alexandre Julliard julli...@winehq.org I don't know enough about writing kernel drivers yet to decide this (and several other things), so I think it's best I hack at it until I have it working, then I'll start submitting patches again. In the meanwhile, is the use of libusb-1.0 in Wine ok? Or should I use libusb-0.1 or some other library? Thank you Damjan Jovanovic
Re: usbhub.sys: add stubbed usbhub.sys
Damjan Jovanovic damjan@gmail.com writes: I don't know enough about writing kernel drivers yet to decide this (and several other things), so I think it's best I hack at it until I have it working, then I'll start submitting patches again. In the meanwhile, is the use of libusb-1.0 in Wine ok? Or should I use libusb-0.1 or some other library? I think 1.0 is fine, but I'd like to see some code using it first. -- Alexandre Julliard julli...@winehq.org
Re: 1/4 wined3d: Move argb_to_fmt to utils.c as a helper function to be used in more places.
Am Donnerstag 25 März 2010 23:02:33 schrieb Roderick Colenbrander: +DWORD color_convert_argb_to_fmt(DWORD color, WINED3DFORMAT destfmt) This only works with formats that are encoded in integers with 32 bit or less. Do we care about this limitation?
Re: 1/4 wined3d: Move argb_to_fmt to utils.c as a helper function to be used in more places.
On Thu, Mar 25, 2010 at 11:58 PM, Stefan Dösinger ste...@codeweavers.com wrote: Am Donnerstag 25 März 2010 23:02:33 schrieb Roderick Colenbrander: +DWORD color_convert_argb_to_fmt(DWORD color, WINED3DFORMAT destfmt) This only works with formats that are encoded in integers with 32 bit or less. Do we care about this limitation? At the moment this code is called to fall back to 'legacy' color fill / clear related calls and that's also what I intend to use it for in blit_shader. I'm not sure if we should handle 128-bit float support in the fallbacks. I agree that later on perhaps we could make this function more generic. The move to utils.c is a first step. The same can be said about the blit_shader fill_color which can later on also be extended if we want to use it from device.c but I don't think it is wise to change it all in the first iteration. Roderick
Re: 1/4 wined3d: Move argb_to_fmt to utils.c as a helper function to be used in more places.
On 25 March 2010 23:58, Stefan Dösinger ste...@codeweavers.com wrote: Am Donnerstag 25 März 2010 23:02:33 schrieb Roderick Colenbrander: +DWORD color_convert_argb_to_fmt(DWORD color, WINED3DFORMAT destfmt) This only works with formats that are encoded in integers with 32 bit or less. Do we care about this limitation? We do, but as far as I'm concerned not for this patch.
Re: 3/4 wined3d: add color_fill to blit_shader
On 25 March 2010 23:04, Roderick Colenbrander thunderbir...@gmail.com wrote:
This patch adds a first part of the new blit_shader infrastructure. In
a few days I plan to submit more parts of it which will extend the
blit_shader and rewrite parts in the end.
This is patch is pretty hacky.
+static const struct blit_shader *blitters[] =
+{
+ffp_blit,
+cpu_blit
+};
This doesn't make much sense here. You'll probably want to make it a
variable local to surface_select_blitter() as a start, but eventually
it should be stored in the device / adapter.
+const struct blit_shader *surface_select_blitter(const struct
wined3d_gl_info *gl_info, IWineD3DSurfaceImpl *src, IWineD3DSurfaceImpl *dst,
enum blit_operation op)
Why do we have both this and select_blit_implementation() now? Also, static.
+switch(op)
+{
+case BLIT_OP_COLOR_FILL:
+return TRUE;
+default:
+return FALSE;
+}
This is a silly way to write return op == BLIT_OP_COLOR_FILL;.
+const struct blit_shader cpu_blit = {
+NULL, /* alloc_private */
+NULL, /* free_private */
+NULL, /* set_shader */
+NULL, /* unset_shader */
+NULL, /* color_fixup_supported */
+cpu_blit_can_blit,
+cpu_blit_color_fill
};
You can't do that, callers don't check these for NULL.
+typedef enum blit_operation
+{
+BLIT_OP_COLOR_FILL=1,
+BLIT_OP_BLIT_SYSMEM_TO_DRAWABLE=2,
+BLIT_OP_BLIT_DRAWABLE_TO_TEXTURE=3
+} blit_operation;
Besides being unused, the distinction between SYSMEM_TO_DRAWABLE,
DRAWABLE_TO_TEXTURE, and other potential variants is an
implementation detail callers shouldn't care about. Also, we don't
care about the exact values of the enum elements, and the typedef only
obscures the type of blit_operation.
BOOL (*color_fixup_supported)(const struct wined3d_gl_info *gl_info,
struct color_fixup_desc fixup);
+BOOL (*can_blit)(const struct wined3d_gl_info *gl_info, blit_operation
op, IWineD3DSurfaceImpl *src_surface, IWineD3DSurfaceImpl *dst_surface);
The functionality of can_blit is a superset of color_fixup_supported.
+IWineD3DSurface_GetContainer((IWineD3DSurface*)dst_surface,
IID_IWineD3DSwapChain, (void **)dst_swapchain);
+if(dst_swapchain) IWineD3DSwapChain_Release((IWineD3DSwapChain *)
dst_swapchain);
That's three different styles of doing a cast, in two lines of code.
And code that's supposedly being rewritten at that. At this point I
don't even care about the exact style you're using (although I do have
a preference), but try to at least care about consistency. Same goes
for these:
+if(dst_swapchain) IWineD3DSwapChain_Release((IWineD3DSwapChain *)
dst_swapchain);
+switch(op)
+if ((IWineD3DSurface*)dst_surface !=
device-render_targets[0]
+const struct blit_shader *surface_select_blitter(const struct
wined3d_gl_info *gl_info, IWineD3DSurfaceImpl *src, IWineD3DSurfaceImpl *dst,
enum blit_operation op)
This line is a bit long for my taste. I think I have relatively large
screens, but this takes up over half a screen. Personally I think 120
characters is a reasonable upper limit, but there are probably plenty
of people that would be happier with 80.
Essentially, I think you're trying to take too large steps. E.g.
splitting of functionality into cpu_blit_color_fill(),
ffp_blit_color_fill(), etc. is ok, hacking it into struct blit_shader
isn't. Extending color_fixup_supported() into something like
blit_supported() could also be a reasonable patch, on its own, but
trying to do it all at the same time becomes hacky pretty quickly.
