Re: [UPDATE] [programs/uninstaller] Check HKCU for uninstall entries too
On 4/19/07, Tom Spear <speeddymon(a)gmail.com> wrote:
Apparently the last patch had multiple patches in the file because I didnt delete it before creating the new one..
So here we go again.
Check HKCU as well as HKLM for uninstall entries, using a for loop
@@ -69,7 +69,7 @@ /** * Used to output program list when used with --list */ -static void ListUninstallPrograms(void) +static void ListUninstallPrograms() The parameter list was correct as is. If a function takes no parameters, the correct list is 'void'. Don't change parts of the code that don't have to do with your patch, especially when you don't know what you're changing. if (i < numentries) - UninstallProgram(); + for (x=0; x<2; x++) + UninstallProgram(rootkey); Why are you calling UninstallProgram twice with the same key? Besides that, using magic numbers (2) is bad coding. + HKEY keys[2]; + keys[0] = HKEY_CURRENT_USER; + keys[1] = HKEY_LOCAL_MACHINE; HKEY keys[] = { HKEY_CURRENT_USER, HKEY_LOCAL_MACHINE }; + for (x=0; x<2; x++) You're hardcoding this value to 2, which is another bad magic number. What if, hypothetically, more root keys are added to the list? for (x = 0; x < sizeof(keys); x++) + rootkey[0] = HKEY_CURRENT_USER; + rootkey[1] = HKEY_LOCAL_MACHINE; Same thing as above. + sizeOfSubKeyName = 255; What is 255? + HKEY rootkey[2]; + rootkey[0] = HKEY_CURRENT_USER; + rootkey[1] = HKEY_LOCAL_MACHINE; Same. + /* FIXME: UninstallProgram should figure out which root key to uninstall + * from, as opposed to just going with the first value in rootkey. The + * entry gets removed this way as well, but it is not apparent by this code. + */ + UninstallProgram(*rootkey); What is the point of adding rootkey if you're not going to use it? UninstallProgram(HKEY_X) is a lot shorter. The point is, don't add something until you're going to use it. On top of that, this new code is basically UninstallProgram(HKEY_CURRENT_USER) when the original code was (parameter added for convenience) UninstallProgram(HKEY_LOCAL_MACHINE). That's not right. -- James Hawkins
Try applying the patch before you comment on it. See below.. On 4/19/07, James Hawkins <truiken(a)gmail.com> wrote:
On 4/19/07, Tom Spear <speeddymon(a)gmail.com> wrote:
Apparently the last patch had multiple patches in the file because I didnt delete it before creating the new one..
So here we go again.
Check HKCU as well as HKLM for uninstall entries, using a for loop
@@ -69,7 +69,7 @@ /** * Used to output program list when used with --list */ -static void ListUninstallPrograms(void) +static void ListUninstallPrograms()
The parameter list was correct as is. If a function takes no parameters, the correct list is 'void'. Don't change parts of the code that don't have to do with your patch, especially when you don't know what you're changing.
That one was an oops, I'll change it back...
if (i < numentries) - UninstallProgram(); + for (x=0; x<2; x++) + UninstallProgram(rootkey);
Why are you calling UninstallProgram twice with the same key? Besides that, using magic numbers (2) is bad coding.
That is another oops. I should have double checked this as well. The use of the 'magic number' in this case is that there will never be more than 2 keys (EVER) to check. However I will make a constant array instead and check x against the number of values in this array instead.
+ HKEY keys[2]; + keys[0] = HKEY_CURRENT_USER; + keys[1] = HKEY_LOCAL_MACHINE;
HKEY keys[] = { HKEY_CURRENT_USER, HKEY_LOCAL_MACHINE };
+ for (x=0; x<2; x++)
You're hardcoding this value to 2, which is another bad magic number. What if, hypothetically, more root keys are added to the list?
see above
for (x = 0; x < sizeof(keys); x++)
+ rootkey[0] = HKEY_CURRENT_USER; + rootkey[1] = HKEY_LOCAL_MACHINE;
Same thing as above.
see above
+ sizeOfSubKeyName = 255;
What is 255?
I dont know, but if you look at the file (pre patch) that is already there, diff just deletes and readds it because it moved further down in the file. However, to answer your question, I checked the MS website: http://support.microsoft.com/kb/256986 <quote> The maximum size of a key name is 255 characters. The following table lists the data types that are currently defined and that are used by Windows. The maximum size of a value name is as follows: • Windows Server 2003 and Windows XP: 16,383 characters • Windows 2000: 260 ANSI characters or 16,383 Unicode characters • Windows Millennium Edition/Windows 98/Windows 95: 255 characters </quote>
+ HKEY rootkey[2]; + rootkey[0] = HKEY_CURRENT_USER; + rootkey[1] = HKEY_LOCAL_MACHINE;
Same. same
+ /* FIXME: UninstallProgram should figure out which root key to uninstall + * from, as opposed to just going with the first value in rootkey. The + * entry gets removed this way as well, but it is not apparent by this code. + */ + UninstallProgram(*rootkey);
What is the point of adding rootkey if you're not going to use it? UninstallProgram(HKEY_X) is a lot shorter. The point is, don't add something until you're going to use it. On top of that, this new code is basically UninstallProgram(HKEY_CURRENT_USER) when the original code was (parameter added for convenience) UninstallProgram(HKEY_LOCAL_MACHINE). That's not right.
Well, unfortunately there isnt a way to call UninstallProgram without a parameter if it is declared that way. So I could either hardcode it to HKEY_LOCAL_MACHINE, since it requires a parameter now, or I could leave it as *rootkey to be fixed later. I tried to find a way to for loop it, but because it is inside DlgProc, I get an error when compiling. So I left it alone.. As the comment says, the code works as is, but it is not apparent by the code. If someone has a suggestion on how to get that specific line of code to look proper (so you can tell if it is doing UninstallProgram on HKLM or HKCU, please feel free.. The reason we need the parameter added to UninstallProgram is because of: /* delete the application's uninstall entry */ RegOpenKeyExW(HKEY_LOCAL_MACHINE, PathUninstallW, 0, KEY_READ, &hkeyUninst); being in the original file. I think it's better to have whichever root key is being used passed to the RegOpenKeyExW..... To be honest I was more worried about someone not liking my use of the for loop in FetchUninstallInformation...
-- James Hawkins
-- Thanks Tom Check out this new 3D Instant Messenger called IMVU. It's the best I have seen yet! http://imvu.com/catalog/web_invitation.php?userId=1547373&from=power-email
On 4/19/07, Tom Spear <speeddymon(a)gmail.com> wrote:
Try applying the patch before you comment on it. See below..
I read all your reply comments, and nothing you said leads me to believe that I need to apply this patch to review it.
On 4/19/07, James Hawkins <truiken(a)gmail.com> wrote:
On 4/19/07, Tom Spear <speeddymon(a)gmail.com> wrote:
Apparently the last patch had multiple patches in the file because I didnt delete it before creating the new one..
So here we go again.
Check HKCU as well as HKLM for uninstall entries, using a for loop
@@ -69,7 +69,7 @@ /** * Used to output program list when used with --list */ -static void ListUninstallPrograms(void) +static void ListUninstallPrograms()
The parameter list was correct as is. If a function takes no parameters, the correct list is 'void'. Don't change parts of the code that don't have to do with your patch, especially when you don't know what you're changing.
That one was an oops, I'll change it back...
if (i < numentries) - UninstallProgram(); + for (x=0; x<2; x++) + UninstallProgram(rootkey);
Why are you calling UninstallProgram twice with the same key? Besides that, using magic numbers (2) is bad coding.
That is another oops. I should have double checked this as well. The use of the 'magic number' in this case is that there will never be more than 2 keys (EVER) to check. However I will make a constant array instead and check x against the number of values in this array instead.
Engineers at a certain software company decided that machines would not have more than 640k of RAM anytime soon, leading to at least a decade of headaches for developers. Case in point, putting self-imposed limits in your code is a bad design habit.
+ HKEY keys[2]; + keys[0] = HKEY_CURRENT_USER; + keys[1] = HKEY_LOCAL_MACHINE;
HKEY keys[] = { HKEY_CURRENT_USER, HKEY_LOCAL_MACHINE };
+ for (x=0; x<2; x++)
You're hardcoding this value to 2, which is another bad magic number. What if, hypothetically, more root keys are added to the list?
see above
for (x = 0; x < sizeof(keys); x++)
+ rootkey[0] = HKEY_CURRENT_USER; + rootkey[1] = HKEY_LOCAL_MACHINE;
Same thing as above.
see above
+ sizeOfSubKeyName = 255;
What is 255?
I dont know, but if you look at the file (pre patch) that is already there, diff just deletes and readds it because it moved further down in the file. However, to answer your question, I checked the MS website:
The old code is clearly in the patch, and no one implied that you wrote it. The comment still stands though. The 255 needs to be replaced with a const or define that clarifies what the value represents.
http://support.microsoft.com/kb/256986
<quote> The maximum size of a key name is 255 characters.
The following table lists the data types that are currently defined and that are used by Windows. The maximum size of a value name is as follows: • Windows Server 2003 and Windows XP: 16,383 characters • Windows 2000: 260 ANSI characters or 16,383 Unicode characters • Windows Millennium Edition/Windows 98/Windows 95: 255 characters </quote>
+ HKEY rootkey[2]; + rootkey[0] = HKEY_CURRENT_USER; + rootkey[1] = HKEY_LOCAL_MACHINE;
Same. same
+ /* FIXME: UninstallProgram should figure out which root key to uninstall + * from, as opposed to just going with the first value in rootkey. The + * entry gets removed this way as well, but it is not apparent by this code. + */ + UninstallProgram(*rootkey);
What is the point of adding rootkey if you're not going to use it? UninstallProgram(HKEY_X) is a lot shorter. The point is, don't add something until you're going to use it. On top of that, this new code is basically UninstallProgram(HKEY_CURRENT_USER) when the original code was (parameter added for convenience) UninstallProgram(HKEY_LOCAL_MACHINE). That's not right.
Well, unfortunately there isnt a way to call UninstallProgram without a parameter if it is declared that way.
Please reread my comment. rootkey is superfluous, not the parameter to UninstallProgram.
So I could either hardcode it to HKEY_LOCAL_MACHINE, since it requires a parameter now, or I could leave it as *rootkey to be fixed later.
Since rootkey shouldn't be in the patch (should only be added when it's used), the only option remaining is to call UninstallProgram(HKEY_LOCAL_MACHINE).
I tried to find a way to for loop it, but because it is inside DlgProc, I get an error when compiling. So I left it alone.. As the comment says, the code works as is, but it is not apparent by the code. If someone has a suggestion on how to get that specific line of code to look proper (so you can tell if it is doing UninstallProgram on HKLM or HKCU, please feel free..
The reason we need the parameter added to UninstallProgram is because of:
/* delete the application's uninstall entry */ RegOpenKeyExW(HKEY_LOCAL_MACHINE, PathUninstallW, 0, KEY_READ, &hkeyUninst);
being in the original file. I think it's better to have whichever root key is being used passed to the RegOpenKeyExW.....
I never had any objections with the new parameter to UninstallProgram. -- James Hawkins
I rewrote the patch based on your suggestions, and I'm having a little difficulty with certain things. I did a little more testing, and it turns out that the uninstaller I was using removes the local machine uninstall entry on its own. So I started futzing with the code some to try to figure a way to get the root key to be properly passed to UninstallProgram. The only thing I can come up with is some form of using entries[sel].key. I can only test at work though, so I have no way of knowing until tomorrow if that would even work. However, assuming that it will, is there a way to strip PathUninstallW from entries[sel].key so that the only thing left is the root key? If so then I can probably use something along the lines of (pseudocode): root_key = strip(entries[sel].key, PathUninstallW); UninstallProgram(root_key); Thanks for the help Tom On 4/19/07, James Hawkins <truiken(a)gmail.com> wrote:
On 4/19/07, Tom Spear <speeddymon(a)gmail.com> wrote:
Try applying the patch before you comment on it. See below..
I read all your reply comments, and nothing you said leads me to believe that I need to apply this patch to review it.
On 4/19/07, James Hawkins <truiken(a)gmail.com> wrote:
On 4/19/07, Tom Spear <speeddymon(a)gmail.com> wrote:
Apparently the last patch had multiple patches in the file because I didnt delete it before creating the new one..
So here we go again.
Check HKCU as well as HKLM for uninstall entries, using a for loop
@@ -69,7 +69,7 @@ /** * Used to output program list when used with --list */ -static void ListUninstallPrograms(void) +static void ListUninstallPrograms()
The parameter list was correct as is. If a function takes no parameters, the correct list is 'void'. Don't change parts of the code that don't have to do with your patch, especially when you don't know what you're changing.
That one was an oops, I'll change it back...
if (i < numentries) - UninstallProgram(); + for (x=0; x<2; x++) + UninstallProgram(rootkey);
Why are you calling UninstallProgram twice with the same key? Besides that, using magic numbers (2) is bad coding.
That is another oops. I should have double checked this as well. The use of the 'magic number' in this case is that there will never be more than 2 keys (EVER) to check. However I will make a constant array instead and check x against the number of values in this array instead.
Engineers at a certain software company decided that machines would not have more than 640k of RAM anytime soon, leading to at least a decade of headaches for developers. Case in point, putting self-imposed limits in your code is a bad design habit.
+ HKEY keys[2]; + keys[0] = HKEY_CURRENT_USER; + keys[1] = HKEY_LOCAL_MACHINE;
HKEY keys[] = { HKEY_CURRENT_USER, HKEY_LOCAL_MACHINE };
+ for (x=0; x<2; x++)
You're hardcoding this value to 2, which is another bad magic number. What if, hypothetically, more root keys are added to the list?
see above
for (x = 0; x < sizeof(keys); x++)
+ rootkey[0] = HKEY_CURRENT_USER; + rootkey[1] = HKEY_LOCAL_MACHINE;
Same thing as above.
see above
+ sizeOfSubKeyName = 255;
What is 255?
I dont know, but if you look at the file (pre patch) that is already there, diff just deletes and readds it because it moved further down in the file. However, to answer your question, I checked the MS website:
The old code is clearly in the patch, and no one implied that you wrote it. The comment still stands though. The 255 needs to be replaced with a const or define that clarifies what the value represents.
http://support.microsoft.com/kb/256986
<quote> The maximum size of a key name is 255 characters.
The following table lists the data types that are currently defined and that are used by Windows. The maximum size of a value name is as follows: • Windows Server 2003 and Windows XP: 16,383 characters • Windows 2000: 260 ANSI characters or 16,383 Unicode characters • Windows Millennium Edition/Windows 98/Windows 95: 255 characters </quote>
+ HKEY rootkey[2]; + rootkey[0] = HKEY_CURRENT_USER; + rootkey[1] = HKEY_LOCAL_MACHINE;
Same. same
+ /* FIXME: UninstallProgram should figure out which root key to uninstall + * from, as opposed to just going with the first value in rootkey. The + * entry gets removed this way as well, but it is not apparent by this code. + */ + UninstallProgram(*rootkey);
What is the point of adding rootkey if you're not going to use it? UninstallProgram(HKEY_X) is a lot shorter. The point is, don't add something until you're going to use it. On top of that, this new code is basically UninstallProgram(HKEY_CURRENT_USER) when the original code was (parameter added for convenience) UninstallProgram(HKEY_LOCAL_MACHINE). That's not right.
Well, unfortunately there isnt a way to call UninstallProgram without a parameter if it is declared that way.
Please reread my comment. rootkey is superfluous, not the parameter to UninstallProgram.
So I could either hardcode it to HKEY_LOCAL_MACHINE, since it requires a parameter now, or I could leave it as *rootkey to be fixed later.
Since rootkey shouldn't be in the patch (should only be added when it's used), the only option remaining is to call UninstallProgram(HKEY_LOCAL_MACHINE).
I tried to find a way to for loop it, but because it is inside DlgProc, I get an error when compiling. So I left it alone.. As the comment says, the code works as is, but it is not apparent by the code. If someone has a suggestion on how to get that specific line of code to look proper (so you can tell if it is doing UninstallProgram on HKLM or HKCU, please feel free..
The reason we need the parameter added to UninstallProgram is because of:
/* delete the application's uninstall entry */ RegOpenKeyExW(HKEY_LOCAL_MACHINE, PathUninstallW, 0, KEY_READ, &hkeyUninst);
being in the original file. I think it's better to have whichever root key is being used passed to the RegOpenKeyExW.....
I never had any objections with the new parameter to UninstallProgram.
-- James Hawkins
-- Thanks Tom Check out this new 3D Instant Messenger called IMVU. It's the best I have seen yet! http://imvu.com/catalog/web_invitation.php?userId=1547373&from=power-email
Well, I finally solved all of my issues with my patch (yay!). It will be ready to submit to wine-patches as soon as I reinstall wine to finish up my testing.. Something borked it and now it won't run even winecfg... Go figure. So anyways, with any luck, my testing will go without a hitch, and I can get my first patch ever done in C for the wine project committed. Wish me luck. Tom On 4/19/07, Tom Spear <speeddymon(a)gmail.com> wrote:
I rewrote the patch based on your suggestions, and I'm having a little difficulty with certain things. I did a little more testing, and it turns out that the uninstaller I was using removes the local machine uninstall entry on its own. So I started futzing with the code some to try to figure a way to get the root key to be properly passed to UninstallProgram. The only thing I can come up with is some form of using entries[sel].key. I can only test at work though, so I have no way of knowing until tomorrow if that would even work. However, assuming that it will, is there a way to strip PathUninstallW from entries[sel].key so that the only thing left is the root key? If so then I can probably use something along the lines of (pseudocode):
root_key = strip(entries[sel].key, PathUninstallW); UninstallProgram(root_key);
Thanks for the help
Tom
On 4/19/07, James Hawkins <truiken(a)gmail.com> wrote:
On 4/19/07, Tom Spear <speeddymon(a)gmail.com> wrote:
Try applying the patch before you comment on it. See below..
I read all your reply comments, and nothing you said leads me to believe that I need to apply this patch to review it.
On 4/19/07, James Hawkins <truiken(a)gmail.com> wrote:
On 4/19/07, Tom Spear <speeddymon(a)gmail.com> wrote:
Apparently the last patch had multiple patches in the file because I didnt delete it before creating the new one..
So here we go again.
Check HKCU as well as HKLM for uninstall entries, using a for loop
@@ -69,7 +69,7 @@ /** * Used to output program list when used with --list */ -static void ListUninstallPrograms(void) +static void ListUninstallPrograms()
The parameter list was correct as is. If a function takes no parameters, the correct list is 'void'. Don't change parts of the code that don't have to do with your patch, especially when you don't know what you're changing.
That one was an oops, I'll change it back...
if (i < numentries) - UninstallProgram(); + for (x=0; x<2; x++) + UninstallProgram(rootkey);
Why are you calling UninstallProgram twice with the same key? Besides that, using magic numbers (2) is bad coding.
That is another oops. I should have double checked this as well. The use of the 'magic number' in this case is that there will never be more than 2 keys (EVER) to check. However I will make a constant array instead and check x against the number of values in this array instead.
Engineers at a certain software company decided that machines would not have more than 640k of RAM anytime soon, leading to at least a decade of headaches for developers. Case in point, putting self-imposed limits in your code is a bad design habit.
+ HKEY keys[2]; + keys[0] = HKEY_CURRENT_USER; + keys[1] = HKEY_LOCAL_MACHINE;
HKEY keys[] = { HKEY_CURRENT_USER, HKEY_LOCAL_MACHINE };
+ for (x=0; x<2; x++)
You're hardcoding this value to 2, which is another bad magic number. What if, hypothetically, more root keys are added to the list?
see above
for (x = 0; x < sizeof(keys); x++)
+ rootkey[0] = HKEY_CURRENT_USER; + rootkey[1] = HKEY_LOCAL_MACHINE;
Same thing as above.
see above
+ sizeOfSubKeyName = 255;
What is 255?
I dont know, but if you look at the file (pre patch) that is already there, diff just deletes and readds it because it moved further down in the file. However, to answer your question, I checked the MS website:
The old code is clearly in the patch, and no one implied that you wrote it. The comment still stands though. The 255 needs to be replaced with a const or define that clarifies what the value represents.
http://support.microsoft.com/kb/256986
<quote> The maximum size of a key name is 255 characters.
The following table lists the data types that are currently defined and that are used by Windows. The maximum size of a value name is as follows: • Windows Server 2003 and Windows XP: 16,383 characters • Windows 2000: 260 ANSI characters or 16,383 Unicode characters • Windows Millennium Edition/Windows 98/Windows 95: 255 characters </quote>
+ HKEY rootkey[2]; + rootkey[0] = HKEY_CURRENT_USER; + rootkey[1] = HKEY_LOCAL_MACHINE;
Same. same
+ /* FIXME: UninstallProgram should figure out which root key to uninstall + * from, as opposed to just going with the first value in rootkey. The + * entry gets removed this way as well, but it is not apparent by this code. + */ + UninstallProgram(*rootkey);
What is the point of adding rootkey if you're not going to use it? UninstallProgram(HKEY_X) is a lot shorter. The point is, don't add something until you're going to use it. On top of that, this new code is basically UninstallProgram(HKEY_CURRENT_USER) when the original code was (parameter added for convenience) UninstallProgram(HKEY_LOCAL_MACHINE). That's not right.
Well, unfortunately there isnt a way to call UninstallProgram without a parameter if it is declared that way.
Please reread my comment. rootkey is superfluous, not the parameter to UninstallProgram.
So I could either hardcode it to HKEY_LOCAL_MACHINE, since it requires a parameter now, or I could leave it as *rootkey to be fixed later.
Since rootkey shouldn't be in the patch (should only be added when it's used), the only option remaining is to call UninstallProgram(HKEY_LOCAL_MACHINE).
I tried to find a way to for loop it, but because it is inside DlgProc, I get an error when compiling. So I left it alone.. As the comment says, the code works as is, but it is not apparent by the code. If someone has a suggestion on how to get that specific line of code to look proper (so you can tell if it is doing UninstallProgram on HKLM or HKCU, please feel free..
The reason we need the parameter added to UninstallProgram is because of:
/* delete the application's uninstall entry */ RegOpenKeyExW(HKEY_LOCAL_MACHINE, PathUninstallW, 0, KEY_READ, &hkeyUninst);
being in the original file. I think it's better to have whichever root key is being used passed to the RegOpenKeyExW.....
I never had any objections with the new parameter to UninstallProgram.
-- James Hawkins
-- Thanks
Tom
Check out this new 3D Instant Messenger called IMVU. It's the best I have seen yet!
http://imvu.com/catalog/web_invitation.php?userId=1547373&from=power-email
-- Thanks Tom Check out this new 3D Instant Messenger called IMVU. It's the best I have seen yet! http://imvu.com/catalog/web_invitation.php?userId=1547373&from=power-email
participants (2)
-
James Hawkins -
Tom Spear