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

[Rob Walker]

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).

Test on Gentoo GNU/Linux.

Signed-off-by: Rob Walker 
---
 dlls/shell32/shellpath.c | 325 ++++++++++++++++++---------------------
 1 file changed, 151 insertions(+), 174 deletions(-)

diff --git a/dlls/shell32/shellpath.c b/dlls/shell32/shellpath.c
index a551e93aa8..c87c73846b 100644
--- a/dlls/shell32/shellpath.c
+++ b/dlls/shell32/shellpath.c
@@ -58,6 +58,7 @@
 WINE_DEFAULT_DEBUG_CHANNEL(shell);

static const BOOL is_win64 = sizeof(void *) > sizeof(int);
+static LONG register_shell_folders = 0;

/*
    ########## Combining and Constructing paths ##########
@@ -4098,6 +4099,15 @@ HRESULT WINAPI SHGetFolderPathAndSubDirW(
         goto end;
     }

+    /* Allow function create_homedir_symbolic_link create this directory
+       (or a HOME directory symlink) for us, later in the boot process. */
+    if (!InterlockedCompareExchange(&register_shell_folders, -1, -1) && (folder == CSIDL_DESKTOPDIRECTORY))
+    {
+        TRACE("Faking successful creation of system directory %s (%#x)\n", debugstr_w(szBuildPath), folder);
+        hr = S_OK;
+        goto end;
+    }
+
     /* create directory/directories */
     ret = SHCreateDirectoryExW(hwndOwner, szBuildPath, NULL);
     if (ret && ret != ERROR_ALREADY_EXISTS)
@@ -4107,7 +4117,7 @@ HRESULT WINAPI SHGetFolderPathAndSubDirW(
         goto end;
     }

-    TRACE("Created missing system directory %s\n", debugstr_w(szBuildPath));
+    TRACE("Created missing system directory %s (%#x)\n", debugstr_w(szBuildPath), folder);
 end:
     TRACE("returning 0x%08x (final path is %s)\n", hr, debugstr_w(szBuildPath));
     return hr;
@@ -4385,193 +4395,111 @@ static inline BOOL _SHAppendToUnixPath(char *szBasePath, LPCWSTR pwszSubPath) {
 }

/******************************************************************************
- * _SHCreateSymbolicLinks  [Internal]
- * 
- * Sets up symbol links for various shell folders to point into the users home
- * directory. We do an educated guess about what the user would probably want:
- * - If there is a 'My Documents' directory in $HOME, the user probably wants
- *   wine's 'My Documents' to point there. Furthermore, we imply that the user
- *   is a Windows lover and has no problem with wine creating 'My Pictures',
- *   'My Music' and 'My Videos' subfolders under '$HOME/My Documents', if those
- *   do not already exits. We put appropriate symbolic links in place for those,
- *   too.
- * - If there is no 'My Documents' directory in $HOME, we let 'My Documents'
- *   point directly to $HOME. We assume the user to be a unix hacker who does not
- *   want wine to create anything anywhere besides the .wine directory. So, if
- *   there already is a 'My Music' directory in $HOME, we symlink the 'My Music'
- *   shell folder to it. But if not, then we check XDG_MUSIC_DIR - "well known"
- *   directory, and try to link to that. If that fails, then we symlink to
- *   $HOME directly. The same holds fo 'My Pictures' and 'My Videos'.
- * - The Desktop shell folder is symlinked to XDG_DESKTOP_DIR. If that does not
- *   exist, then we try '$HOME/Desktop'. If that does not exist, then we leave
- *   it alone.
- * ('My Music',... above in fact means LoadString(IDS_MYMUSIC))
+ * create_homedir_symbolic_link  [Internal]
+ *
+ * Creates a symbolic link from the current Wineprefix to an appropriate
+ * HOME subdirectory (if one is found).
+ *
+ *  Creates 'XXXX' directory in Wineprefix.
+ *  Then create a 'My XXXX' symbolic link in Wineprefix:
+ *  1) If '$HOME/XXXX' (IDS directory) exists then target this.
+ *  2) If '$HOME/XXXX' (XDG_XXXX_DIR) exists then target this.
+ *  3) If '$HOME/XXXX' (MacOS XXXX media directory) exists then target this.
+ *
+ *  PARAMS
+ *  ids_dir     [I]  Windows Resource Identifier code for current Shell Folder.
+ *  csidl_dir   [I]  Constant Special Item ID List identifier for current Shell Folder.
+ *  xdg_dir     [I]  Full path of external Unix XDG directory corresponding to current Shell Folder.
+ *  ws_osx_dir  [I]  Fallback directory name to use, corresponding to current Shell Folder (OSX specific).
+ *
  */
-static void _SHCreateSymbolicLinks(void)
-{
-    UINT aidsMyStuff[] = { IDS_MYPICTURES, IDS_MYVIDEOS, IDS_MYMUSIC }, i;
-    const WCHAR* MyOSXStuffW[] = { PicturesW, MoviesW, MusicW };
-    int acsidlMyStuff[] = { CSIDL_MYPICTURES, CSIDL_MYVIDEO, CSIDL_MYMUSIC };
-    static const char * const xdg_dirs[] = { "PICTURES", "VIDEOS", "MUSIC", "DOCUMENTS", "DESKTOP" };
-    static const unsigned int num = ARRAY_SIZE(xdg_dirs);
-    WCHAR wszTempPath[MAX_PATH];
-    char szPersonalTarget[FILENAME_MAX], *pszPersonal;
-    char szMyStuffTarget[FILENAME_MAX], *pszMyStuff;
-    char szDesktopTarget[FILENAME_MAX], *pszDesktop;
-    struct stat statFolder;
-    const char *pszHome;
+void create_homedir_symbolic_link(UINT ids_dir,
+                                  int csidl_dir,
+                                  const char * xdg_dir,
+                                  const WCHAR * ws_osx_dir)
+{
+    static const char * env_homedir = NULL;
+    WCHAR ws_temp_path[MAX_PATH];
+    char home_target[FILENAME_MAX], * prefix_dir;
+    struct stat stat_folder, stat_home_folder;
     HRESULT hr;
-    char ** xdg_results;
-    char * xdg_desktop_dir;
-
-    /* Create all necessary profile sub-dirs up to 'My Documents' and get the unix path. */
-    hr = SHGetFolderPathW(NULL, CSIDL_PERSONAL|CSIDL_FLAG_CREATE, NULL,
-                          SHGFP_TYPE_DEFAULT, wszTempPath);
-    if (FAILED(hr)) return;
-    pszPersonal = wine_get_unix_file_name(wszTempPath);
-    if (!pszPersonal) return;
+    BOOL target_ok;

-    hr = XDG_UserDirLookup(xdg_dirs, num, &xdg_results);
-    if (FAILED(hr)) xdg_results = NULL;
-
-    pszHome = getenv("HOME");
-    if (pszHome && !stat(pszHome, &statFolder) && S_ISDIR(statFolder.st_mode))
+    hr = SHGetFolderPathW(NULL, csidl_dir, NULL,
+                SHGFP_TYPE_DEFAULT, ws_temp_path);
+    if (SUCCEEDED(hr))
     {
-        while (1)
-        {
-            /* Check if there's already a Wine-specific 'My Documents' folder */
-            strcpy(szPersonalTarget, pszHome);
-            if (_SHAppendToUnixPath(szPersonalTarget, MAKEINTRESOURCEW(IDS_PERSONAL)) &&
-                !stat(szPersonalTarget, &statFolder) && S_ISDIR(statFolder.st_mode))
-            {
-                /* '$HOME/My Documents' exists. Create 'My Pictures',
-                 * 'My Videos' and 'My Music' subfolders or fail silently if
-                 * they already exist.
-                 */
-                for (i = 0; i < ARRAY_SIZE(aidsMyStuff); i++)
-                {
-                    strcpy(szMyStuffTarget, szPersonalTarget);
-                    if (_SHAppendToUnixPath(szMyStuffTarget, MAKEINTRESOURCEW(aidsMyStuff[i])))
-                        mkdir(szMyStuffTarget, 0777);
-                }
-                break;
-            }
-
-            /* Try to point to the XDG Documents folder */
-            if (xdg_results && xdg_results[num-2] &&
-               !stat(xdg_results[num-2], &statFolder) &&
-               S_ISDIR(statFolder.st_mode))
-            {
-                strcpy(szPersonalTarget, xdg_results[num-2]);
-                break;
-            }
-
-            /* Or the hardcoded / OS X Documents folder */
-            strcpy(szPersonalTarget, pszHome);
-            if (_SHAppendToUnixPath(szPersonalTarget, DocumentsW) &&
-               !stat(szPersonalTarget, &statFolder) &&
-               S_ISDIR(statFolder.st_mode))
-                break;
-
-            /* As a last resort point to $HOME. */
-            strcpy(szPersonalTarget, pszHome);
-            break;
-        }
-
-        /* Replace 'My Documents' directory with a symlink or fail silently if not empty. */
-        remove(pszPersonal);
-        symlink(szPersonalTarget, pszPersonal);
+        TRACE("%s directory already exists\n",
+              debugstr_w(ws_temp_path));
+        return;
+    }
+    else if (hr == HRESULT_FROM_WIN32(ERROR_FILE_NOT_FOUND))
+    {
+        hr = SHGetFolderPathW(NULL, csidl_dir|CSIDL_FLAG_CREATE, NULL,
+                            SHGFP_TYPE_DEFAULT, ws_temp_path);
+        if (FAILED(hr)) return;
     }
     else
     {
-        /* '$HOME' doesn't exist. Create 'My Pictures', 'My Videos' and 'My Music' subdirs
-         * in '%USERPROFILE%\My Documents' or fail silently if they already exist. */
-        pszHome = NULL;
-        strcpy(szPersonalTarget, pszPersonal);
-        for (i = 0; i < ARRAY_SIZE(aidsMyStuff); i++) {
-            strcpy(szMyStuffTarget, szPersonalTarget);
-            if (_SHAppendToUnixPath(szMyStuffTarget, MAKEINTRESOURCEW(aidsMyStuff[i])))
-                mkdir(szMyStuffTarget, 0777);
-        }
+        ERR("Failed to get Wineprefix path corresponding to %d CSIDL\n",
+            csidl_dir);
+        return;
     }

-    /* Create symbolic links for 'My Pictures', 'My Videos' and 'My Music'. */
-    for (i=0; i < ARRAY_SIZE(aidsMyStuff); i++)
+    prefix_dir = wine_get_unix_file_name(ws_temp_path);
+    if (!prefix_dir)
     {
-        /* Create the current 'My Whatever' folder and get its unix path. */
-        hr = SHGetFolderPathW(NULL, acsidlMyStuff[i]|CSIDL_FLAG_CREATE, NULL,
-                              SHGFP_TYPE_DEFAULT, wszTempPath);
-        if (FAILED(hr)) continue;
-
-        pszMyStuff = wine_get_unix_file_name(wszTempPath);
-        if (!pszMyStuff) continue;
-        
-        while (1)
-        {
-            /* Check for the Wine-specific '$HOME/My Documents' subfolder */
-            strcpy(szMyStuffTarget, szPersonalTarget);
-            if (_SHAppendToUnixPath(szMyStuffTarget, MAKEINTRESOURCEW(aidsMyStuff[i])) &&
-                !stat(szMyStuffTarget, &statFolder) && S_ISDIR(statFolder.st_mode))
-                break;
-
-            /* Try the XDG_XXX_DIR folder */
-            if (xdg_results && xdg_results[i])
-            {
-                strcpy(szMyStuffTarget, xdg_results[i]);
-                break;
-            }
-
-            /* Or the OS X folder (these are never localized) */
-            if (pszHome)
-            {
-                strcpy(szMyStuffTarget, pszHome);
-                if (_SHAppendToUnixPath(szMyStuffTarget, MyOSXStuffW[i]) &&
-                    !stat(szMyStuffTarget, &statFolder) &&
-                    S_ISDIR(statFolder.st_mode))
-                    break;
-            }
+        ERR("Failed to get Unix Wineprefix directory for: %s\n",
+            debugstr_w(ws_temp_path));
+        return;
+    }

-            /* As a last resort point to the same location as 'My Documents' */
-            strcpy(szMyStuffTarget, szPersonalTarget);
-            break;
-        }
-        remove(pszMyStuff);
-        symlink(szMyStuffTarget, pszMyStuff);
-        heap_free(pszMyStuff);
+    if (!env_homedir) env_homedir = getenv("HOME");
+    if (!(env_homedir
+          && (stat(env_homedir, &stat_home_folder) != 1)
+          && S_ISDIR(stat_home_folder.st_mode)))
+    {
+        if (prefix_dir) heap_free(prefix_dir);
+        return;
     }

-    /* Last but not least, the Desktop folder */
-    if (pszHome)
-        strcpy(szDesktopTarget, pszHome);
+    /* Check for:
+     *    '$HOME/XXXX' (IDS directory)
+     * or '$HOME/XXXX' (XDG directory)
+     * or '$HOME/XXXX' (MacOS directory)
+     */
+    target_ok = FALSE;
+    strcpy(home_target, env_homedir);
+    if (_SHAppendToUnixPath(home_target, MAKEINTRESOURCEW(ids_dir))
+        && (stat(home_target, &stat_folder) != -1)
+        && S_ISDIR(stat_folder.st_mode))
+    {
+        target_ok = TRUE;
+    }
+    else if (xdg_dir)
+    {
+        strcpy(home_target, xdg_dir);
+        /* Only link to the XDG directory, if it does not point directly
+         * to the user's HOME directory (XDG specification fallback path). */
+        target_ok = TRUE;
+    }
     else
-        strcpy(szDesktopTarget, pszPersonal);
-    heap_free(pszPersonal);
-
-    xdg_desktop_dir = xdg_results ? xdg_results[num - 1] : NULL;
-    if (xdg_desktop_dir ||
-        (_SHAppendToUnixPath(szDesktopTarget, DesktopW) &&
-        !stat(szDesktopTarget, &statFolder) && S_ISDIR(statFolder.st_mode)))
     {
-        hr = SHGetFolderPathW(NULL, CSIDL_DESKTOPDIRECTORY|CSIDL_FLAG_CREATE, NULL,
-                              SHGFP_TYPE_DEFAULT, wszTempPath);
-        if (SUCCEEDED(hr) && (pszDesktop = wine_get_unix_file_name(wszTempPath))) 
-        {
-            remove(pszDesktop);
-            if (xdg_desktop_dir)
-                symlink(xdg_desktop_dir, pszDesktop);
-            else
-                symlink(szDesktopTarget, pszDesktop);
-            heap_free(pszDesktop);
-        }
+        strcpy(home_target, env_homedir);
+        target_ok = _SHAppendToUnixPath(home_target, ws_osx_dir)
+                    && (stat(home_target, &stat_folder) != -1)
+                    && S_ISDIR(stat_folder.st_mode);
     }

-    /* Free resources allocated by XDG_UserDirLookup() */
-    if (xdg_results)
+    if (target_ok)
     {
-        for (i = 0; i < num; i++)
-            heap_free(xdg_results[i]);
-        heap_free(xdg_results);
+        TRACE("Delete path: %s\n", debugstr_a(prefix_dir));
+        remove(prefix_dir);
+        TRACE("Symlink: %s -> %s\n", debugstr_a(prefix_dir), debugstr_a(home_target));
+        symlink(home_target, prefix_dir);
     }
+
+    if (prefix_dir) heap_free(prefix_dir);
 }

/******************************************************************************
@@ -6126,15 +6054,61 @@ static void register_system_knownfolders(void)
     }
 }

+/******************************************************************************
+ * create_homedir_symbolic_links  [Internal]
+ *
+ * Parse WINESYMLINK env variable, for each XDG directory argument. To test if symlinking is
+ * enabled for that XDG directory / Wine Profile Folder.
+ * Then calls the function create_homedir_symbolic_link to potentially symlink from a Shell Folder,
+ * in the current Wineprefix, to a subdirectory of the current user's HOME directory.
+ *
+ * PARAMS
+ *  xdg_dirnames   [I]  Pointer to an array of Unix XDG directory names
+ *                      (without "XDG_" prefix and "_DIR" suffix).
+ *  xdg_dir_count  [I]  Item count of array (above).
+ *
+ */
+static void create_homedir_symbolic_links(const char * const xdg_dirnames[], const UINT xdg_dir_count)
+{
+    char ** xdg_dirs_array;
+    char * xdg_dir;
+    HRESULT hr;
+    UINT i;
+
+    if (!xdg_dirnames) return;
+
+    hr = XDG_UserDirLookup(xdg_dirnames, xdg_dir_count, &xdg_dirs_array);
+    if (FAILED(hr)) xdg_dirs_array = NULL;
+
+    for (i = 0; i < xdg_dir_count; ++i)
+    {
+        xdg_dir = xdg_dirs_array ? xdg_dirs_array[i] : NULL;
+        if (!strcmp(xdg_dirnames[i],"DOCUMENTS"))
+            create_homedir_symbolic_link(IDS_PERSONAL, CSIDL_PERSONAL, xdg_dir, DocumentsW);
+        else if (!strcmp(xdg_dirnames[i],"PICTURES"))
+            create_homedir_symbolic_link(IDS_MYPICTURES, CSIDL_MYPICTURES, xdg_dir, PicturesW);
+        else if (!strcmp(xdg_dirnames[i],"VIDEOS"))
+            create_homedir_symbolic_link(IDS_MYVIDEOS, CSIDL_MYVIDEO, xdg_dir, MoviesW);
+        else if (!strcmp(xdg_dirnames[i],"MUSIC"))
+            create_homedir_symbolic_link(IDS_MYMUSIC, CSIDL_MYMUSIC, xdg_dir, MusicW);
+        else if (!strcmp(xdg_dirnames[i],"DESKTOP"))
+            create_homedir_symbolic_link(IDS_DESKTOPDIRECTORY, CSIDL_DESKTOPDIRECTORY, xdg_dir, DesktopW);
+        else
+            ERR("XDG directory name specifier invalid: %s\n", debugstr_a(xdg_dirnames[i]));
+        if (xdg_dir) heap_free(xdg_dirs_array[i]);
+    }
+    if (xdg_dirs_array) heap_free(xdg_dirs_array);
+}
+
 HRESULT SHELL_RegisterShellFolders(void)
 {
+    static const char * const xdg_dirnames[] = { "DOCUMENTS", "PICTURES", "VIDEOS", "MUSIC", "DESKTOP" };
+    const UINT xdg_dir_count = 5;
     HRESULT hr;

-    /* Set up '$HOME' targeted symlinks for 'My Documents', 'My Pictures',
-     * 'My Videos', 'My Music' and 'Desktop' in advance, so that the
-     * _SHRegister*ShellFolders() functions will find everything nice and clean
-     * and thus will not attempt to create them in the profile directory. */
-    _SHCreateSymbolicLinks();
+    /* Early setup of symlinks from specific User Shell Folders, in
+     * current Wineprefix, to subdirecties of the user's HOME directory. */
+    create_homedir_symbolic_links(xdg_dirnames, xdg_dir_count);

hr = _SHRegisterUserShellFolders(TRUE);
     if (SUCCEEDED(hr))
@@ -6147,5 +6121,8 @@ HRESULT SHELL_RegisterShellFolders(void)
         hr = set_folder_attributes();
     if (SUCCEEDED(hr))
         register_system_knownfolders();
+
+    (void) InterlockedExchange(&register_shell_folders, 1);
+
     return hr;
 }

[Huw Davies]

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.

[Bob Wya]

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():

1) The function is far too long, in it's present form - it's currently 165
lines long!

2) The comments should be far more terse and precise.

3) Using infinite while (1) loops to de-mark code blocks only serves to
obscure the functionality of the code.

4)

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.)

5)

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...

6)

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).

--------------------------------------------------------------------------------------------------------------------------------

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. :-)

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.

[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.