r58591 - small bug?

Here you can discuss ReactOS related topics.

Moderator: Moderator Team

Post Reply
User avatar
Black_Fox
Posts: 1584
Joined: Fri Feb 15, 2008 9:44 pm
Location: Czechia

r58591 - small bug?

Post by Black_Fox »

Code: Select all

--- trunk/reactos/tools/mkhive/registry.c	2013/03/23 11:26:10	58590
+++ trunk/reactos/tools/mkhive/registry.c	2013/03/23 17:59:35	58591
@@ -562,6 +562,8 @@
 	rc = RegQueryValueExW(hKey, lpValueNameW, lpReserved, lpType, lpData, lpcbData);
 	if (lpValueNameW)
 		free(lpValueNameW);
+	if (rc != ERROR_SUCCESS)
+		return rc;
 	return ERROR_UNSUCCESSFUL;
 } 
Sounds to me like "if it's not success, return it, otherwise also return not success" :)
fireball
Developer
Posts: 358
Joined: Tue Nov 30, 2004 10:40 pm
Location: Moscow, Russia
Contact:

Re: r58591 - small bug?

Post by fireball »

what's wrong with just returning rc? It would be the most obvious and proper solution, IMO.
Aleksey Bragin,
ReactOS Project Lead
User avatar
Black_Fox
Posts: 1584
Joined: Fri Feb 15, 2008 9:44 pm
Location: Czechia

Re: r58591 - small bug?

Post by Black_Fox »

Let me rewrite what I see into this equivalent code

Code: Select all

if (rc != ERROR_SUCCESS)
    return rc;
else // if (rc == ERROR_SUCCESS)
    return ERROR_UNSUCCESSFUL;
if rc equals ERROR_SUCCESS, surely we want to return ERROR_UNSUCCESSFUL? Maybe I missed something, but I still see it this way :)
hto
Developer
Posts: 2193
Joined: Sun Oct 01, 2006 3:43 pm

Post by hto »

Of course, it could just return rc.
User avatar
Black_Fox
Posts: 1584
Joined: Fri Feb 15, 2008 9:44 pm
Location: Czechia

Re: r58591 - small bug?

Post by Black_Fox »

Aha, now I see I probably misunderstood Aleksey's post. Sorry about that :oops:
vicmarcal
Test Team
Posts: 2733
Joined: Mon Jul 07, 2008 12:35 pm

Re: r58591 - small bug?

Post by vicmarcal »

Black_Fox wrote:Aha, now I see I probably misunderstood Aleksey's post. Sorry about that :oops:
Dont't feel bad, I dont get it neither.

So this part of the code...

Code: Select all

if (rc != ERROR_SUCCESS)
    return rc;
For me has sense: In case the API is failing(aka rc is not ERROR_SUCCESS), we are returning the proper ERROR.(Maybe the function needs which is the exactly system error in order to perform other operations)

but the...

Code: Select all

else 
    return ERROR_UNSUCCESSFUL;
...Doesnt have any sense for me.
This means:
In case the API is working (aka rc is ERROR_SUCCESS) we are returning it didn't success at all. This doesnt have sense.

Let's see from another POV: If the API is working, we are forcing it to return ERROR_UNSUCCESSFUL.
And from another POV(2nd part): When are we supposely to be returning ERROR_SUCCESS?We aren't, in any case.


So for me, the correct code would be:

Code: Select all

if (rc != ERROR_SUCCESS) 
      return rc;
else
      return ERROR_SUCCESS;
which is equivalent to:

Code: Select all

return rc;
User avatar
Black_Fox
Posts: 1584
Joined: Fri Feb 15, 2008 9:44 pm
Location: Czechia

Re: r58591 - small bug?

Post by Black_Fox »

Yes, exactly :) That's what both fireball and hto mean.

(I initially thought that fireball agrees with the original patch, but I only got him wrong)
vicmarcal
Test Team
Posts: 2733
Joined: Mon Jul 07, 2008 12:35 pm

Re: r58591 - small bug?

Post by vicmarcal »

Black_Fox wrote:Aha, now I see I probably misunderstood Aleksey's post. Sorry about that :oops:
Hahaha...I know understand Aleksey reply too.
I thought he was arguing against Black_Fox but seems he was agree with him.
User avatar
Black_Fox
Posts: 1584
Joined: Fri Feb 15, 2008 9:44 pm
Location: Czechia

Re: r58591 - small bug?

Post by Black_Fox »

Thanks to Johaness for fixing it in r58626 ;)
Post Reply

Who is online

Users browsing this forum: Google [Bot] and 41 guests