Re: [PATCH v2] shellpath.c: Fix creation of HOME directory symbolic links

[Huw Davies]

On Sat, Jun 09, 2018 at 12:45:23AM +0100, Bob Wya wrote:
> On 8 June 2018 at 12:52, Huw Davies  wrote:
>      On Fri, Jun 08, 2018 at 10:58:23AM +0100, Rob Walker wrote:
>      > Fixes: https://bugs.winehq.org/show_bug.cgi?id=41668
>      >
>      > The Shell Folders, that Wine symlinks to the user's HOME directory,
>      are
>      > only re-created on each Wine boot if they: do not pre-exist or are
>      broken
>      > symbolic links.
>      >
>      > Handling the DESKTOP Shell Folder is a special case as this is
>      created twice
>      > (during a standard boot), early in the Wine boot process. This is
>      handled by
>      > 2 separate processes, loading shell32.dll, in sequence. This makes
>      it hard to determine
>      > if the DESKTOP folder was created before the Wine boot (either from
>      a previous Wine boot
>      > or by the end user).
>      >
>      > The final (implemented) solution determines the exact start time of
>      the Wine boot process
>      > (using the current system time and tick-count). If the DESKTOP
>      directory was last written
>      > after this Wine boot time, we can assume that "we" (Wine)
>      automatically created the
>      > directory. Only in this instance do we attempt to set a symlink to
>      the DESKTOP directory
>      > (subdirectory of HOME).
>  There's still too much going on here.
>
>  Do you really need to refactor the code to make your change?
>  If not, then just send in the change to the current code.
>  If you need to refactor, then do the refactoring first
>  (I could imagine taking 3-4 patches to do the refactoring[1])
>  then make the change as a final patch in the series.
>
>  The file-time / boot-time thing seems hacky, I'm not exactly
>  sure what you're trying do to, but this doesn't sound right.
>  Hopefully that will become clearer as you tidy things up.
>
>  Huw.
>
>  [1] For example move the creation of My Pictures/My Videos/etc
>  first, then move My Documents and finally Desktop.  These
>  final two are special cases in the current code, we'd need
>  to see that in any new code.
>
> Sorry I forgot to update the commit message! I'd updated the code (in the v2
> patch), for handling the
> special case of the User profile Desktop directory, without testing file times
> / boot times (the latter
> didn't work as GetTickTime64 was returning the host system uptime anyway).
>
> I have a number of concerns with the existing implementation of
> _SHCreateSymbolicLinks():
> - The function is far too long, in it's present form - it's currently 165
>
> lines long!
> -
>
> The comments should be far more terse and precise.
> -
>
> Using infinite while (1) loops to de-mark code blocks only serves to obscure
>
> the functionality of the code.

I'd agree, the current function isn't ideal.  But, unfortunately, you
can't just rip it out and start again.  Things need to be done incrementally.
> -
>
> 4487-        /* '$HOME' doesn't exist. Create 'My Pictures', 'My Videos' and
> 'My Music' subdirs
> 4488-         * in '%USERPROFILE%\My Documents' or fail silently if they
> already exist. */
> 4489-        pszHome = NULL;
> 4490-        strcpy(szPersonalTarget, pszPersonal);
> 4491-        for (i = 0; i < ARRAY_SIZE(aidsMyStuff); i++) {
> 4492-            strcpy(szMyStuffTarget, szPersonalTarget);
> 4493-            if (_SHAppendToUnixPath(szMyStuffTarget, MAKEINTRESOURCEW
> (aidsMyStuff[i])))
> 4494-                mkdir(szMyStuffTarget, 0777);
> 4495-        }
>
> Windows Vista (and newer) do not nest the User profile directories. Since Wine
> is targeting Windows 7 by default - this
> legacy behaviour should be removed. (As an end user, who used to use Windows
> XP, this was simply an annoying default
> layout anyway.)
> -
>
> 4447-                /* '$HOME/My Documents' exists. Create 'My Pictures',
> 4448-                 * 'My Videos' and 'My Music' subfolders or fail silently
> if
> 4449-                 * they already exist.
> 4450-                 */
> 4451-                for (i = 0; i < ARRAY_SIZE(aidsMyStuff); i++)
> 4452-                {
> 4453-                    strcpy(szMyStuffTarget, szPersonalTarget);
> 4454-                    if (_SHAppendToUnixPath(szMyStuffTarget,
> MAKEINTRESOURCEW(aidsMyStuff[i])))
> 4455-                        mkdir(szMyStuffTarget, 0777);
> 4456-                }
> 4457-                break;
>
> Ditto as per (4). This is an obsolete User Profile directory layout (pre-
> Vista).
>
> Also if "${HOME}/My Documents" exists then this block of code will start
> creating subdirectories in this
> folder. I don't think that Wine should be making the rather arbitrary
> assumption that a Wine user wants
> their "${HOME}" directory spammed with new subdirectories.
>
> I often read online, comments from Wine users, complaining about this sort of
> behaviour...
> -
>
> 4543-    /* Last but not least, the Desktop folder */
> 4544-    if (pszHome)
> 4545-        strcpy(szDesktopTarget, pszHome);
> 4546-    else
> 4547-        strcpy(szDesktopTarget, pszPersonal);
> 4548-    heap_free(pszPersonal);
> 4549-
> 4550-    xdg_desktop_dir = xdg_results ? xdg_results[num - 1] : NULL;
> 4551-    if (xdg_desktop_dir ||
> 4552-        (_SHAppendToUnixPath(szDesktopTarget, DesktopW) &&
> 4553-        !stat(szDesktopTarget, &statFolder) && S_ISDIR
> (statFolder.st_mode)))
> 4554-    {
> 4555-        hr = SHGetFolderPathW(NULL,
> CSIDL_DESKTOPDIRECTORY|CSIDL_FLAG_CREATE, NULL,
> 4556-                              SHGFP_TYPE_DEFAULT, wszTempPath);
> 4557-        if (SUCCEEDED(hr) && (pszDesktop = wine_get_unix_file_name
> (wszTempPath)))
> 4558-        {
> 4559-            remove(pszDesktop);
> 4560-            if (xdg_desktop_dir)
> 4561-                symlink(xdg_desktop_dir, pszDesktop);
> 4562-            else
> 4563-                symlink(szDesktopTarget, pszDesktop);
> 4564-            heap_free(pszDesktop);
> 4565-        }
> 4566-    }
>
> This code uses pszPersonal as a fallback target for when HOME is unset.
> This will be the equivalent of:
>
> "${WINEPREFIX}/dosdevices/c:/users/${USER}/My Documents"
>
> Say the XDG_DESKTOP_DIR check fails. Then Wine tries to find the directory:
>
> "${WINEPREFIX}/dosdevices/c:/users/${USER}/My Documents/Desktop"
>
> as a symlink target. Which never succeeds because Wine never creates this
> directory...
>
> This extra path appears to be redundant (HOME unset / XDG_DESKTOP_DIR unset).

So points 4 - 6 sound like they each deserve a separate patch.
> I can of course leave the variable names unchanged and the function name.
> I was "told off" for not using snake case on a previous Wine commit - so I
> assumed
> I had to follow this standard if I wanted to refactor existing code. :-)

In this case you're adding to existing code so it becomes a bit more
flexible.  Generally though keeping the patch as small as possible should
be your aim.  The reviewer needs to be able to understand what's going on;
including unrelated or needless changes makes that much harder.
> I'd like to tidy this function up and make it more readable (including the
> comments).
> Which I can do in staged patches, as you suggested.
>
> But obviously I'd like to feel that we are both on the "same page" about the
> changes
> I discussed (above). Before I email in a v3 staged patchset.

Go ahead and send in those patches, then people can give you meaningful feedback
about the actual implementation details.

Huw.