[UPDATE] [programs/uninstaller] Check HKCU for uninstall entries too

[James Hawkins]

On 4/19/07, Tom Spear <speeddymon at 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 at gmail.com> wrote:
> > On 4/19/07, Tom Spear <speeddymon at 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