Hi Johannes,
a few comments...
On Dec 17 19:05, Johannes Schindelin wrote:
> [...]
> diff --git a/winsup/cygwin/uinfo.cc b/winsup/cygwin/uinfo.cc
> index c9b3e09..a5d6270 100644
> --- a/winsup/cygwin/uinfo.cc
> +++ b/winsup/cygwin/uinfo.cc
> [...]
> +static size_t
> +fetch_env(LPCWSTR key, char *buf, size_t size)
^^^
space
> +{
> + WCHAR wbuf[32767];
Ok, there are a couple of problems here. First, since this buffer
is a filename buffer, use NT_MAX_PATH from winsup.h as buffer size.
But then again, please avoid allocating 64K buffers on the stack.
That's what tmp_pathbuf:w_get () is for.
> + DWORD max = sizeof wbuf / sizeof *wbuf;
> + DWORD len = GetEnvironmentVariableW (key, wbuf, max);
This call to GetEnvironmentVariableW looks gratuitous to me. Why don't
you simply call getenv? It did the entire job already, it avoids the
requirement for a local buffer, and in case of $HOME it even did the
Win32->POSIX path conversion. If there's a really good reason for using
GetEnvironmentVariableW it begs at least for a longish comment.
> +
> + if (!len || len >= max)
> + return 0;
> +
> + len = sys_wcstombs (buf, size, wbuf, len);
> + return len && len < size ? len : 0;
> +}
> +
> +static char *
> +fetch_home_env (void)
> +{
> + char home[32767];
> + size_t max = sizeof home / sizeof *home, len;
> +
> + if (fetch_env (L"HOME", home, max)
> + || ((len = fetch_env (L"HOMEDRIVE", home, max))
> + && fetch_env (L"HOMEPATH", home + len, max - len))
> + || fetch_env (L"USERPROFILE", home, max))
> + {
> + tmp_pathbuf tp;
> + cygwin_conv_path (CCP_WIN_A_TO_POSIX | CCP_ABSOLUTE,
> + home, tp.c_get(), NT_MAX_PATH);
^^^
space
> + return strdup(tp.c_get());
^^^ ^^^
space......s
Whoa, tp.c_get() twice to access the same space? That's a dirty trick
which may puzzle later readers of the code and heavily depends on
knowing the internals of tmp_pathbuf. Please use a variable and only
assign tp.c_get () once.
OTOH, the above's a case for a cygwin_create_path call, rather than
cygwin_conv_path+strdup. Also, if there's *really* a good reason to use
GetEnvironmentVariableW, you should collapse sys_wcstombs+cygwin_conv_path+
strdup into a single cygwin_create_path (CCP_WIN_W_TO_POSIX, ...).
> [...]
> @@ -1079,6 +1123,7 @@ cygheap_pwdgrp::get_shell (cyg_ldap *pldap, cygpsid &sid, PCWSTR dom,
> case NSS_SCHEME_FALLBACK:
> return NULL;
> case NSS_SCHEME_WINDOWS:
> + case NSS_SCHEME_ENV:
> break;
> case NSS_SCHEME_CYGWIN:
> if (pldap->fetch_ad_account (sid, false, dnsdomain))
You know that I don't exactly like the "env" idea, but if we implement
it anyway, wouldn't it make sense to add some kind of $SHELL handling as
well, for symmetry?
> [...]
> @@ -1487,6 +1497,16 @@ of each schema when used with db_home:
> for a detailed description.
>
>
> + env
> + Derives the home directory of the current user from the
> + environment variable HOME (falling back to
> + HOMEDRIVE\HOMEPATH and
> + USERPROFILE, in that order). This is faster
> + than the windows schema at the
> + expense of determining only the current user's home directory
> + correctly.
In both case of the documentation it might make sense to add a few words
along the lines of "This schema is skipped for any other account",
wouldn't it?
Thanks,
Corinna
--
Corinna Vinschen Please, send mails regarding Cygwin to
Cygwin Maintainer cygwin AT cygwin DOT com
Red Hat