[PATCH v2] shellpath.c: Fix creation of HOME directory symbolic links
Bob Wya
bob.mt.wya at gmail.com
Fri Jun 8 18:45:23 CDT 2018
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.
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.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://www.winehq.org/pipermail/wine-devel/attachments/20180609/b4d8804d/attachment-0001.html>
More information about the wine-devel
mailing list