[PATCH v2] shellpath.c: Fix creation of HOME directory symbolic links
Huw Davies
huw at codeweavers.com
Mon Jun 11 03:49:12 CDT 2018
On Sat, Jun 09, 2018 at 12:45:23AM +0100, Bob Wya wrote:
>
>
> On 8 June 2018 at 12:52, Huw Davies <huw at codeweavers.com> 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.
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.
> 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).
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.
More information about the wine-devel
mailing list