Hi Paul,
Paul Vriens wrote:
We already check and return FALSE a few lines up.
Found with the help of Coccinelle and the CocciCheck scripts.
Changelog Don't check result twice (Coccinelle)
--- a/dlls/rsaenh/tests/rsaenh.c +++ b/dlls/rsaenh/tests/rsaenh.c @@ -275,7 +275,6 @@ static BOOL derive_key(ALG_ID aiAlgid, HCRYPTKEY *phKey, DWORD len) return FALSE; } ok(result, "%08x\n", GetLastError()); - if (!result) return FALSE; result = CryptHashData(hHash, pbData, sizeof(pbData), 0); ok(result, "%08x\n", GetLastError()); if (!result) return FALSE; that is not the same "result"; it gets a new value after the first "return FALSE".
bye michael
On 01/12/2010 02:32 PM, Michael Stefaniuc wrote:
Hi Paul,
Paul Vriens wrote:
We already check and return FALSE a few lines up.
Found with the help of Coccinelle and the CocciCheck scripts.
Changelog Don't check result twice (Coccinelle)
--- a/dlls/rsaenh/tests/rsaenh.c +++ b/dlls/rsaenh/tests/rsaenh.c @@ -275,7 +275,6 @@ static BOOL derive_key(ALG_ID aiAlgid, HCRYPTKEY *phKey, DWORD len) return FALSE; } ok(result, "%08x\n", GetLastError());
- if (!result) return FALSE; result = CryptHashData(hHash, pbData, sizeof(pbData), 0); ok(result, "%08x\n", GetLastError()); if (!result) return FALSE;
that is not the same "result"; it gets a new value after the first "return FALSE".
Where?
The only thing between the "return FALSE;" and the "if (!result)" is an ok() statement.
Hello Paul,
Paul Vriens wrote:
On 01/12/2010 02:32 PM, Michael Stefaniuc wrote:
Paul Vriens wrote:
We already check and return FALSE a few lines up.
Found with the help of Coccinelle and the CocciCheck scripts.
Changelog Don't check result twice (Coccinelle)
--- a/dlls/rsaenh/tests/rsaenh.c +++ b/dlls/rsaenh/tests/rsaenh.c @@ -275,7 +275,6 @@ static BOOL derive_key(ALG_ID aiAlgid, HCRYPTKEY *phKey, DWORD len) return FALSE; } ok(result, "%08x\n", GetLastError());
- if (!result) return FALSE;
^^^ old result value
result = CryptHashData(hHash, pbData, sizeof(pbData), 0);
^^^ new result value
ok(result, "%08x\n", GetLastError()); if (!result) return FALSE;that is not the same "result"; it gets a new value after the first "return FALSE".
Where?
The only thing between the "return FALSE;" and the "if (!result)" is an ok() statement.
I see a "result = CryptHashData()" in between.
bye michael
On 01/12/2010 02:51 PM, Michael Stefaniuc wrote:
Hello Paul,
Paul Vriens wrote:
On 01/12/2010 02:32 PM, Michael Stefaniuc wrote:
Paul Vriens wrote:
We already check and return FALSE a few lines up.
Found with the help of Coccinelle and the CocciCheck scripts.
Changelog Don't check result twice (Coccinelle)
--- a/dlls/rsaenh/tests/rsaenh.c +++ b/dlls/rsaenh/tests/rsaenh.c @@ -275,7 +275,6 @@ static BOOL derive_key(ALG_ID aiAlgid, HCRYPTKEY *phKey, DWORD len) return FALSE; } ok(result, "%08x\n", GetLastError());
- if (!result) return FALSE;
^^^ old result valueresult = CryptHashData(hHash, pbData, sizeof(pbData), 0);^^^ new result valueok(result, "%08x\n", GetLastError()); if (!result) return FALSE;that is not the same "result"; it gets a new value after the first "return FALSE".
Where?
The only thing between the "return FALSE;" and the "if (!result)" is an ok() statement.
I see a "result = CryptHashData()" in between.
bye michael
But we are already returning a few lines above:
271 result = CryptCreateHash(hProv, CALG_MD2, 0, 0, &hHash); 272 if (!result) { 273 /* rsaenh compiled without OpenSSL */ 274 ok(GetLastError()==NTE_BAD_ALGID, "%08x\n", GetLastError()); 275 return FALSE; 276 }
We will not get here if result = 0;
277 ok(result, "%08x\n", GetLastError()); 278 if (!result) return FALSE;
So that last one is needless.
Paul Vriens wrote:
On 01/12/2010 02:51 PM, Michael Stefaniuc wrote:
Hello Paul,
Paul Vriens wrote:
On 01/12/2010 02:32 PM, Michael Stefaniuc wrote:
Paul Vriens wrote:
We already check and return FALSE a few lines up.
Found with the help of Coccinelle and the CocciCheck scripts.
Changelog Don't check result twice (Coccinelle)
--- a/dlls/rsaenh/tests/rsaenh.c +++ b/dlls/rsaenh/tests/rsaenh.c @@ -275,7 +275,6 @@ static BOOL derive_key(ALG_ID aiAlgid, HCRYPTKEY *phKey, DWORD len) return FALSE; } ok(result, "%08x\n", GetLastError());
- if (!result) return FALSE;
^^^ old result valueresult = CryptHashData(hHash, pbData, sizeof(pbData), 0);^^^ new result valueok(result, "%08x\n", GetLastError()); if (!result) return FALSE;that is not the same "result"; it gets a new value after the first "return FALSE".
Where?
The only thing between the "return FALSE;" and the "if (!result)" is an ok() statement.
I see a "result = CryptHashData()" in between.
bye michael
But we are already returning a few lines above:
271 result = CryptCreateHash(hProv, CALG_MD2, 0, 0, &hHash); 272 if (!result) { 273 /* rsaenh compiled without OpenSSL */ 274 ok(GetLastError()==NTE_BAD_ALGID, "%08x\n", GetLastError()); 275 return FALSE; 276 }
We will not get here if result = 0;
277 ok(result, "%08x\n", GetLastError()); 278 if (!result) return FALSE;
So that last one is needless.
Sorry about that, I should have looked at the code and not only at the patch. But the ok() line 277 is pretty pointless there as it won't produce an error ever. Well of course you might want to keep it to increase the "successful tests" counter.
bye michael
Michael Stefaniuc [mailto:mstefani@redhat.com]
But we are already returning a few lines above:
271 result = CryptCreateHash(hProv, CALG_MD2, 0, 0, &hHash); 272 if (!result) { 273 /* rsaenh compiled without OpenSSL */ 274 ok(GetLastError()==NTE_BAD_ALGID, "%08x\n",
GetLastError());
275 return FALSE; 276 }
We will not get here if result = 0;
277 ok(result, "%08x\n", GetLastError()); 278 if (!result) return FALSE;
So that last one is needless.
Sorry about that, I should have looked at the code and not only at the patch. But the ok() line 277 is pretty pointless there as it won't produce an error ever. Well of course you might want to keep it to increase the "successful tests" counter.
Actually that is not completely true. The first ok will usually produce an error except when the OpenSSL provider is not available and the second ok() will print out the success. It is equivalent to this:
272 if (!result) { 273 /* rsaenh compiled without OpenSSL */ 274 ok(GetLastError()==NTE_BAD_ALGID, "%08x\n", GetLastError()); 275 return FALSE; 276 } else { 277 ok(result, "%08x\n", GetLastError()); 278 if (!result) return FALSE; }
And could be rewritten like this without functional change:
272 ok((result || GetLastError()==NTE_BAD_ALGID), "%08x\n", GetLastError()); 273 if (!result) return FALSE;
But boolean logic is in general not as obvious in one glance.
Rolf Kalbermatter
-
Michael Stefaniuc -
Paul Vriens -
Rolf Kalbermatter